Fix statement rollback discarding the whole transaction on a syntax error (#203)

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)
 	{

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 {

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 ...; <statement>" 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);

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

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

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 ...; <statement>" 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=<count query failed>, tx=ABORTED\n");
+		SQLFreeStmt(s, SQL_CLOSE);
+		return;
+	}
+	if (SQLFetch(s) != SQL_SUCCESS)
+	{
+		printf(", marker=<fetch failed>\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;
 }