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 { 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); 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/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/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; } 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 \