From 5e95bbf48e9c6a7a2d34d8a29a8bf789d3fb687c Mon Sep 17 00:00:00 2001 From: Dave Cramer Date: Wed, 26 Aug 2026 06:58:30 -0400 Subject: [PATCH 1/4] test: add failing regression test for issue #203 (statement rollback vs syntax error) With Protocol=7.4-2 (statement rollback), a syntax error currently rolls back the entire transaction instead of just the offending statement, silently discarding earlier work. This test inserts a row, triggers a syntax error, and asserts the row survives -- plus an execution-time error case that already works, as a regression guard. Fails on the current driver (row count 0 instead of 1 after the syntax error); the fix follows. --- test/expected/rollback-syntax-error.out | 8 ++ test/src/rollback-syntax-error-test.c | 127 ++++++++++++++++++++++++ test/tests | 1 + 3 files changed, 136 insertions(+) create mode 100644 test/expected/rollback-syntax-error.out create mode 100644 test/src/rollback-syntax-error-test.c diff --git a/test/expected/rollback-syntax-error.out b/test/expected/rollback-syntax-error.out new file mode 100644 index 00000000..b7f32145 --- /dev/null +++ b/test/expected/rollback-syntax-error.out @@ -0,0 +1,8 @@ +connected +Case 1: syntax error must roll back only the statement + expected error, SQLSTATE=42601 + rows visible after syntax error: 1 +Case 2: execution-time error rolls back only the statement + expected error, SQLSTATE=42P01 + rows visible after exec-time error: 2 +disconnecting diff --git a/test/src/rollback-syntax-error-test.c b/test/src/rollback-syntax-error-test.c new file mode 100644 index 00000000..c3cb51e4 --- /dev/null +++ b/test/src/rollback-syntax-error-test.c @@ -0,0 +1,127 @@ +/* + * Regression test for issue #203. + * + * With statement-level rollback (Protocol 7.4-2), a *syntax* error must roll + * back only the offending statement, leaving earlier work in the transaction + * intact -- exactly like an execution-time error already does. + * + * The driver used to bundle "SAVEPOINT ...; " into a single + * simple-query string. A syntax error fails that whole string at parse time, + * so the SAVEPOINT never executed and the driver fell back to rolling back the + * entire transaction -- silently discarding earlier work (see the issue: a + * table lock obtained earlier was lost). An execution-time error (e.g. a + * missing relation) did not hit this because the SAVEPOINT executed first. + * + * This test inserts a row, triggers each kind of error, and checks that the + * row is still visible afterwards (statement rolled back, transaction alive). + */ +#include +#include +#include + +#include "common.h" + +static HSTMT hstmt = SQL_NULL_HSTMT; + +/* Execute a statement expected to fail; print only its SQLSTATE (stable + * across server versions, unlike the message text). */ +static void +exec_expect_error(const char *sql) +{ + SQLRETURN rc; + + rc = SQLExecDirect(hstmt, (SQLCHAR *) sql, SQL_NTS); + if (SQL_SUCCEEDED(rc)) + { + printf(" ERROR: statement unexpectedly succeeded: %s\n", sql); + return; + } + { + SQLCHAR state[6]; + SQLINTEGER native; + SQLCHAR msg[512]; + SQLSMALLINT len; + + if (SQL_SUCCEEDED(SQLGetDiagRec(SQL_HANDLE_STMT, hstmt, 1, state, + &native, msg, sizeof(msg), &len))) + printf(" expected error, SQLSTATE=%s\n", state); + } + SQLFreeStmt(hstmt, SQL_CLOSE); +} + +static void +exec_ok(const char *sql) +{ + SQLRETURN rc; + + rc = SQLExecDirect(hstmt, (SQLCHAR *) sql, SQL_NTS); + CHECK_STMT_RESULT(rc, "SQLExecDirect failed", hstmt); + SQLFreeStmt(hstmt, SQL_CLOSE); +} + +/* Print how many rows are currently visible in rbtab. */ +static void +show_rowcount(const char *label) +{ + SQLRETURN rc; + SQLCHAR buf[32]; + SQLLEN ind; + + rc = SQLExecDirect(hstmt, (SQLCHAR *) "SELECT count(*) FROM rbtab", SQL_NTS); + CHECK_STMT_RESULT(rc, "count query failed", hstmt); + rc = SQLFetch(hstmt); + CHECK_STMT_RESULT(rc, "count fetch failed", hstmt); + rc = SQLGetData(hstmt, 1, SQL_C_CHAR, buf, sizeof(buf), &ind); + CHECK_STMT_RESULT(rc, "count getdata failed", hstmt); + printf(" %s: %s\n", label, (char *) buf); + SQLFreeStmt(hstmt, SQL_CLOSE); +} + +int +main(int argc, char **argv) +{ + SQLRETURN rc; + + test_connect_ext("Protocol=7.4-2"); + + rc = SQLAllocStmt(conn, &hstmt); + if (!SQL_SUCCEEDED(rc)) + { + print_diag("failed to allocate stmt handle", SQL_HANDLE_DBC, conn); + exit(1); + } + + rc = SQLSetConnectAttr(conn, SQL_ATTR_AUTOCOMMIT, + (SQLPOINTER) SQL_AUTOCOMMIT_OFF, SQL_IS_UINTEGER); + CHECK_STMT_RESULT(rc, "SQLSetConnectAttr failed", hstmt); + + /* A committed temp table gives us a clean, session-local slate. */ + exec_ok("CREATE TEMPORARY TABLE rbtab (i int4)"); + rc = SQLEndTran(SQL_HANDLE_DBC, conn, SQL_COMMIT); + CHECK_STMT_RESULT(rc, "SQLEndTran (create) failed", hstmt); + + /* + * Case 1: syntax error (parse-time). This is the issue #203 case. + * The row inserted before the error must survive. + */ + printf("Case 1: syntax error must roll back only the statement\n"); + exec_ok("INSERT INTO rbtab VALUES (100)"); + exec_expect_error("INSERT INTO rbtab VALUS (101)"); /* typo -> 42601 */ + show_rowcount("rows visible after syntax error"); + + /* + * Case 2: execution-time error (missing relation). Same expectation; + * this path already worked, so it guards against regressions. + */ + printf("Case 2: execution-time error rolls back only the statement\n"); + exec_ok("INSERT INTO rbtab VALUES (200)"); + exec_expect_error("INSERT INTO no_such_table_xyz VALUES (201)"); /* 42P01 */ + show_rowcount("rows visible after exec-time error"); + + rc = SQLEndTran(SQL_HANDLE_DBC, conn, SQL_ROLLBACK); + CHECK_STMT_RESULT(rc, "SQLEndTran (rollback) failed", hstmt); + + test_disconnect(); + + return 0; +} diff --git a/test/tests b/test/tests index 7331541f..46c104b6 100644 --- a/test/tests +++ b/test/tests @@ -50,6 +50,7 @@ TESTBINS = exe/connect-test \ exe/cte-test \ exe/errors-test \ exe/error-rollback-test \ + exe/rollback-syntax-error-test \ exe/diagnostic-test \ exe/numeric-test \ exe/large-object-test \ From d169f42b8e5a5b648b7a64d3e96d7be88e931da4 Mon Sep 17 00:00:00 2001 From: Dave Cramer Date: Wed, 26 Aug 2026 14:45:46 -0400 Subject: [PATCH 2/4] test: extend error-rollback-test with error-class matrix for issue #203 The existing error-rollback-test only exercised one error class (invalid integer input, 22P02) via SQLExecDirect, so it never caught the case where a *syntax* error (42601) silently aborts the whole transaction under statement-level rollback (Protocol=7.4-2). Add a matrix section that, under 7.4-2, inserts a marker row and then runs a failing statement, checking that the marker row survives (i.e. only the statement, not the transaction, was rolled back). Cases cover three error classes (42601 syntax, 42P01 undefined relation, 22P02 bad value) crossed with two execution paths (ExecDirect vs Prepare/Execute) and both UseServerSidePrepare settings. The syntax-error rows are the ones broken by the bug in the bundled 'SAVEPOINT ...; ' simple-query send: ExecDirect+SSP=0, Prepare+SSP=0 and ExecDirect+SSP=1 all report marker=0 on the current driver. Prepare+SSP=1 already works because Prepare has its own separate round-trip when server-side prepare is on. Uses SQLSTATE-only output for the new section so the expected file stays stable across PostgreSQL versions and locales. Existing protocol 0/1/2 blocks are unchanged to keep pre-existing expected lines identical. --- test/expected/error-rollback.out | 18 ++++ test/src/error-rollback-test.c | 157 +++++++++++++++++++++++++++++++ 2 files changed, 175 insertions(+) diff --git a/test/expected/error-rollback.out b/test/expected/error-rollback.out index 24cc4a90..7926e540 100644 --- a/test/expected/error-rollback.out +++ b/test/expected/error-rollback.out @@ -55,3 +55,21 @@ Result set: 6 7 disconnecting +Test for rollback protocol 2 error-class matrix (SSP=0) +connected + ExecDirect syntax : SQLSTATE=42601, marker=1, tx=alive + Prepare syntax : SQLSTATE=42601, marker=1, tx=alive + ExecDirect undef-rel : SQLSTATE=42P01, marker=1, tx=alive + Prepare undef-rel : SQLSTATE=42P01, marker=1, tx=alive + ExecDirect bad-value : SQLSTATE=22P02, marker=1, tx=alive + Prepare bad-value : SQLSTATE=22P02, marker=1, tx=alive +disconnecting +Test for rollback protocol 2 error-class matrix (SSP=1) +connected + ExecDirect syntax : SQLSTATE=42601, marker=1, tx=alive + Prepare syntax : SQLSTATE=42601, marker=1, tx=alive + ExecDirect undef-rel : SQLSTATE=42P01, marker=1, tx=alive + Prepare undef-rel : SQLSTATE=42P01, marker=1, tx=alive + ExecDirect bad-value : SQLSTATE=22P02, marker=1, tx=alive + Prepare bad-value : SQLSTATE=22P02, marker=1, tx=alive +disconnecting diff --git a/test/src/error-rollback-test.c b/test/src/error-rollback-test.c index 7c34df7f..bee8422f 100644 --- a/test/src/error-rollback-test.c +++ b/test/src/error-rollback-test.c @@ -141,6 +141,145 @@ error_rollback_print(void) print_result(hstmt); } +/* + * Helpers for the error-class matrix below. + * + * The existing tests above cover just one error class (invalid integer input, + * 22P02) via SQLExecDirect. With statement rollback (Protocol=7.4-2) the + * driver used to bundle "SAVEPOINT ...; " into a single simple- + * query string, which fails as a unit at parse time on grammar errors -- so + * a *syntax error* (42601) would silently roll back the whole transaction + * instead of just the offending statement (issue #203). The matrix below + * exercises three error classes across ExecDirect and Prepare/Execute, and + * asserts (via a marker row) that earlier work in the transaction survives. + * + * Errors always surface at SQLExecute time (not SQLPrepare) for the paths + * this driver takes, so we only print the SQLSTATE, not where it fired. + */ + +/* Extract the first SQLSTATE from a statement handle. */ +static void +get_state(HSTMT s, char *out, size_t outlen) +{ + SQLCHAR state[6] = {0}; + SQLINTEGER native; + SQLCHAR msg[256]; + SQLSMALLINT len; + + SQLGetDiagRec(SQL_HANDLE_STMT, s, 1, state, + &native, msg, sizeof(msg), &len); + snprintf(out, outlen, "%s", (char *) state); +} + +/* Run a statement expected to fail; print its SQLSTATE. A different + * SQLSTATE, or unexpected success, is flagged inline so `diff` catches it. */ +static void +expect_fail(HSTMT s, int use_prepare, const char *label, + const char *sql, const char *want_state) +{ + SQLRETURN rc; + char state[8] = {0}; + + if (use_prepare) + { + rc = SQLPrepare(s, (SQLCHAR *) sql, SQL_NTS); + if (SQL_SUCCEEDED(rc)) + rc = SQLExecute(s); + } + else + rc = SQLExecDirect(s, (SQLCHAR *) sql, SQL_NTS); + + if (SQL_SUCCEEDED(rc)) + { + printf("%s: UNEXPECTED SUCCESS\n", label); + SQLFreeStmt(s, SQL_CLOSE); + return; + } + get_state(s, state, sizeof(state)); + printf("%s: SQLSTATE=%s", label, state); + if (strcmp(state, want_state) != 0) + printf(" [MISMATCH want=%s]", want_state); + SQLFreeStmt(s, SQL_CLOSE); +} + +/* After a failed statement, verify the marker row inserted earlier in the + * same transaction is still visible (statement-level rollback worked) and + * that the transaction is still usable. */ +static void +check_survival(HSTMT s) +{ + SQLRETURN rc; + SQLCHAR buf[32]; + SQLLEN ind; + + rc = SQLExecDirect(s, (SQLCHAR *) + "SELECT count(*) FROM errortab WHERE i=100", + SQL_NTS); + if (!SQL_SUCCEEDED(rc)) + { + printf(", marker=, tx=ABORTED\n"); + SQLFreeStmt(s, SQL_CLOSE); + return; + } + if (SQLFetch(s) != SQL_SUCCESS) + { + printf(", marker=\n"); + SQLFreeStmt(s, SQL_CLOSE); + return; + } + SQLGetData(s, 1, SQL_C_CHAR, buf, sizeof(buf), &ind); + printf(", marker=%s", (char *) buf); + SQLFreeStmt(s, SQL_CLOSE); + + rc = SQLExecDirect(s, (SQLCHAR *) "SELECT 1", SQL_NTS); + printf(", tx=%s\n", SQL_SUCCEEDED(rc) ? "alive" : "ABORTED"); + SQLFreeStmt(s, SQL_CLOSE); +} + +/* Run one case: insert marker row, fail the given statement, then verify + * the marker row survives. Each case rolls back at the end so the next + * case starts clean. */ +static void +run_case(int use_prepare, const char *label, + const char *sql, const char *want_state) +{ + SQLRETURN rc; + + rc = SQLExecDirect(hstmt, (SQLCHAR *) + "INSERT INTO errortab VALUES (100)", SQL_NTS); + CHECK_STMT_RESULT(rc, "marker insert failed", hstmt); + SQLFreeStmt(hstmt, SQL_CLOSE); + + expect_fail(hstmt, use_prepare, label, sql, want_state); + check_survival(hstmt); + + rc = SQLEndTran(SQL_HANDLE_DBC, conn, SQL_ROLLBACK); + CHECK_STMT_RESULT(rc, "SQLEndTran (case) failed", hstmt); +} + +/* One matrix run: three error classes x two execution paths. */ +static void +run_matrix(const char *options, const char *header) +{ + printf("%s\n", header); + error_rollback_init((char *) options); + + run_case(0, " ExecDirect syntax ", + "INSERT INTO errortab VALUS (1)", "42601"); + run_case(1, " Prepare syntax ", + "INSERT INTO errortab VALUS (1)", "42601"); + run_case(0, " ExecDirect undef-rel ", + "INSERT INTO no_such_tbl VALUES (1)", "42P01"); + run_case(1, " Prepare undef-rel ", + "INSERT INTO no_such_tbl VALUES (1)", "42P01"); + run_case(0, " ExecDirect bad-value ", + "INSERT INTO errortab VALUES ('nope')", "22P02"); + run_case(1, " Prepare bad-value ", + "INSERT INTO errortab VALUES ('nope')", "22P02"); + + error_rollback_clean(); +} + int main(int argc, char **argv) { @@ -226,5 +365,23 @@ main(int argc, char **argv) /* Clean up */ error_rollback_clean(); + /* + * Error-class matrix under Protocol=7.4-2. + * + * Each case inserts a marker row, then runs a statement expected to fail + * with a particular SQLSTATE, then asserts the marker row is still there + * (statement rollback worked) and the transaction is still usable. + * + * The syntax-error (42601) rows are the ones that broke pre-issue-#203 + * fix: on ExecDirect+SSP=0, Prepare+SSP=0 and ExecDirect+SSP=1 the whole + * transaction was silently aborted, so the marker row disappeared. + * The other classes and Prepare+SSP=1 have always worked; they're + * included as regression guards. + */ + run_matrix("Protocol=7.4-2;UseServerSidePrepare=0", + "Test for rollback protocol 2 error-class matrix (SSP=0)"); + run_matrix("Protocol=7.4-2;UseServerSidePrepare=1", + "Test for rollback protocol 2 error-class matrix (SSP=1)"); + return 0; } From 7d6cc84399c3aef7ffe65e6fa10f511b6ba8dc43 Mon Sep 17 00:00:00 2001 From: Dave Cramer Date: Wed, 26 Aug 2026 16:19:39 -0400 Subject: [PATCH 3/4] Fix statement rollback losing the transaction on a syntax error (#203) With statement-level rollback (Protocol=7.4-2) the driver established the per-statement internal SAVEPOINT by prepending it to the user's statement and sending 'SAVEPOINT ...; ' as a single simple-query string (the SVPOPT_REDUCE_ROUNDTRIP optimization). PostgreSQL parses a simple-query string in full before executing any of it, so a statement that fails at *parse* time -- a syntax error (SQLSTATE 42601) -- fails the whole string, and the leading SAVEPOINT never runs. With no savepoint to roll back to, the driver fell back to aborting the entire transaction, silently discarding work done earlier in it (e.g. a table lock, as reported). Execution-time errors (missing relation, type-coercion, etc.) were unaffected because the SAVEPOINT executed before the failing statement was analyzed. Send the SAVEPOINT as its own round-trip so it is always in place before the user's statement is parsed. A syntax error then rolls back only that statement and the transaction stays usable. This costs one extra round-trip per rolled-back statement under 7.4-2. Covered by rollback-syntax-error-test and the error-class matrix added to error-rollback-test. --- execute.c | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/execute.c b/execute.c index b6b57d2a..6edc81c6 100644 --- a/execute.c +++ b/execute.c @@ -812,12 +812,19 @@ MYLOG(DETAIL_LOG_LEVEL, " %p->accessed=%d opt=%u in_progress=%u prev=%u\n", conn } if (need_savep) { - if (0 != (option & SVPOPT_REDUCE_ROUNDTRIP)) - { - conn->internal_op = PREPEND_IN_PROGRESS; - CC_set_accessed_db(conn); - return ret; - } + /* + * Establish the internal savepoint with its own round-trip + * instead of prepending it to the user's statement. Bundling + * "SAVEPOINT ...; " into a single simple-query string + * means a statement that fails at parse time (e.g. a syntax + * error) takes the SAVEPOINT down with it: the SAVEPOINT never + * executes, so statement-level rollback has no savepoint to roll + * back to and the whole transaction is discarded, silently losing + * work done earlier in the transaction (issue #203). Sending the + * SAVEPOINT separately guarantees it is in place before the + * statement is parsed, at the cost of one extra round-trip per + * rolled-back statement. + */ GenerateSvpCommand(conn, INTERNAL_SAVEPOINT_OPERATION, cmd, sizeof(cmd)); conn->internal_op = SAVEPOINT_IN_PROGRESS; res = CC_send_query(conn, cmd, NULL, 0, NULL); From d8e8f4aae42ad8b9ecce6cacfe8f9b5868338061 Mon Sep 17 00:00:00 2001 From: Dave Cramer Date: Wed, 26 Aug 2026 16:24:15 -0400 Subject: [PATCH 4/4] Remove now-dead prepend-savepoint machinery With the #203 fix, SetStatementSvp no longer defers the internal SAVEPOINT to be prepended onto the next query, so PREPEND_IN_PROGRESS is never set. Remove the unreachable prepend handling in CC_send_query_append (the prepend_savepoint local and its branch) and the PREPEND_IN_PROGRESS enumerator. No behavior change. --- connection.c | 16 ++-------------- connection.h | 1 - 2 files changed, 2 insertions(+), 15 deletions(-) diff --git a/connection.c b/connection.c index 54c069dc..2237d9ff 100644 --- a/connection.c +++ b/connection.c @@ -1875,7 +1875,7 @@ CC_send_query_append(ConnectionClass *self, const char *query, QueryInfo *qi, UD create_keyset = ((flag & CREATE_KEYSET) != 0), issue_begin = ((flag & GO_INTO_TRANSACTION) != 0 && !CC_is_in_trans(self)), rollback_on_error, query_rollback, end_with_commit, - read_only, prepend_savepoint = FALSE, + read_only, ignore_roundtrip_time = ((self->connInfo.extra_opts & BIT_IGNORE_ROUND_TRIP_TIME) != 0); char *ptr; @@ -1979,16 +1979,12 @@ CC_send_query_append(ConnectionClass *self, const char *query, QueryInfo *qi, UD } } - /* prepend internal savepoint command ? */ - if (PREPEND_IN_PROGRESS == self->internal_op) - prepend_savepoint = TRUE; - /* append all these together, to avoid round-trips */ query_len = strlen(query); MYLOG(0, "query_len=" FORMAT_SIZE_T "\n", query_len); initPQExpBuffer(&query_buf); - /* issue_begin, query_rollback and prepend_savepoint are exclusive */ + /* issue_begin and query_rollback are exclusive */ if (issue_begin) { appendPQExpBuffer(&query_buf, "%s;", bgncmd); @@ -1999,14 +1995,6 @@ CC_send_query_append(ConnectionClass *self, const char *query, QueryInfo *qi, UD appendPQExpBuffer(&query_buf, "%s %s;", svpcmd, per_query_svp); discard_next_savepoint = TRUE; } - else if (prepend_savepoint) - { - char prepend_cmd[128]; - - GenerateSvpCommand(self, INTERNAL_SAVEPOINT_OPERATION, prepend_cmd, sizeof(prepend_cmd)); - appendPQExpBuffer(&query_buf, "%s;", prepend_cmd); - self->internal_op = SAVEPOINT_IN_PROGRESS; - } appendPQExpBufferStr(&query_buf, query); if (appendq) { diff --git a/connection.h b/connection.h index d1f0a087..a74f32d4 100644 --- a/connection.h +++ b/connection.h @@ -521,7 +521,6 @@ int GenerateSvpCommand(ConnectionClass *conn, int type, char *cmd, int bufsize); /* Operations in progress */ enum { SAVEPOINT_IN_PROGRESS = 1 - ,PREPEND_IN_PROGRESS }; /* StatementSvp entry option */ enum {