Skip to content

Don't savepoint-wrap internal queries at rollback level 0 (Protocol=7.4-0) (#208) - #209

Open
davecramer wants to merge 1 commit into
mainfrom
fix-208-no-savepoint-cleanup-level0
Open

Don't savepoint-wrap internal queries at rollback level 0 (Protocol=7.4-0) (#208)#209
davecramer wants to merge 1 commit into
mainfrom
fix-208-no-savepoint-cleanup-level0

Conversation

@davecramer

@davecramer davecramer commented Aug 30, 2026

Copy link
Copy Markdown
Contributor

Fixes #208 (scoped to the primary break; see "Not in this PR" below).

Problem

With Protocol=7.4-0 (rollback-on-error level 0), freeing a prepared statement inside a transaction still sends

SAVEPOINT _per_query_svp_;DEALLOCATE "_PLAN0x…";RELEASE _per_query_svp_

On a server without SAVEPOINT support (QuestDB, Materialize, ...) the SAVEPOINT is rejected and aborts the transaction, so a free-before-commit silently loses the transaction's rows.

Root cause

psqlODBC has two savepoint mechanisms:

  • per-statement _EXEC_SVP_… -- already gated by the rollback level (StartRollbackState), so 7.4-0 correctly disables it;
  • per-query _per_query_svp_ -- wraps internal driver queries such as the DEALLOCATE for a freed prepared statement. It was triggered solely by the ROLLBACK_ON_ERROR send flag and never consulted the rollback level.

So at level 0 the internal DEALLOCATE was still savepoint-wrapped.

Fix

Gate the per-query savepoint on the connection's rollback level in CC_send_query_append:

query_rollback = (rollback_on_error && !end_with_commit && PG_VERSION_GE(self, 8.0)
                  && self->connInfo.rollback_on_error != 0);

This is the single point every ROLLBACK_ON_ERROR send flows through (the DEALLOCATE sites in statement.c, connection.c, qresult.c). Levels default (-1), 1 and 2 are unchanged; only explicit 7.4-0 is affected -- internal queries are then sent bare.

Verification (PostgreSQL 18)

Wire log:

  • default level: SAVEPOINT _per_query_svp_;DEALLOCATE "…";RELEASE _per_query_svp_ (unchanged);
  • Protocol=7.4-0: bare DEALLOCATE "…".

Functional: free-before-commit commits both rows at each level; error-rollback-test still passes. (The break itself only reproduces on a savepoint-less server; PostgreSQL accepts both forms, so it can't be exercised by the PG-based regression suite.)

Not in this PR

The report's second point -- that a failing internal cleanup query is swallowed, so SQLEndTran(SQL_COMMIT) returns SQL_SUCCESS for a transaction the server rolled back -- is a separate, broader error-propagation concern touching the destructor/commit paths for all servers. This PR fixes the concrete data loss (the internal query no longer fails at level 0); surfacing internal-query errors is better tracked as a follow-up.

@davecramer
Don't wrap internal queries in a savepoint at rollback level 0 (#208)
922e08e 
The per-query savepoint (SAVEPOINT _per_query_svp_; <query>; RELEASE
_per_query_svp_) that protects internal driver queries -- notably the
DEALLOCATE issued when a prepared statement is freed inside a transaction
-- was applied whenever the ROLLBACK_ON_ERROR flag was set, without
regard to the connection's rollback-on-error level.

With Protocol=7.4-0 the user has explicitly asked for no automatic
savepoint handling, typically because the server has no SAVEPOINT support
(e.g. QuestDB, Materialize).  There the injected SAVEPOINT is rejected
and, inside a transaction, aborts it; freeing a prepared statement before
COMMIT then silently discarded the transaction's rows.

Gate the per-query savepoint on connInfo.rollback_on_error != 0 so that
at level 0 internal queries are sent bare.  Levels default(-1)/1/2 are
unchanged.  Verified on PostgreSQL 18: at the default level the DEALLOCATE
cleanup is still wrapped, and with Protocol=7.4-0 it is sent as a bare
DEALLOCATE.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment