From 6eb52da3948dc8bc7c8a61cbacac14823b670c58 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Thu, 7 Sep 2017 09:49:55 -0400 Subject: Fix handling of savepoint commands within multi-statement Query strings. Issuing a savepoint-related command in a Query message that contains multiple SQL statements led to a FATAL exit with a complaint about "unexpected state STARTED". This is a shortcoming of commit 4f896dac1, which attempted to prevent such misbehaviors in multi-statement strings; its quick hack of marking the individual statements as "not top-level" does the wrong thing in this case, and isn't a very accurate description of the situation anyway. To fix, let's introduce into xact.c an explicit model of what happens for multi-statement Query strings. This is an "implicit transaction block in progress" state, which for many purposes works like the normal TBLOCK_INPROGRESS state --- in particular, IsTransactionBlock returns true, causing the desired result that PreventTransactionChain will throw error. But in case of error abort it works like TBLOCK_STARTED, allowing the transaction to be cancelled without need for an explicit ROLLBACK command. Commit 4f896dac1 is reverted in toto, so that we go back to treating the individual statements as "top level". We could have left it as-is, but this allows sharpening the error message for PreventTransactionChain calls inside functions. Except for getting a normal error instead of a FATAL exit for savepoint commands, this patch should result in no user-visible behavioral change (other than that one error message rewording). There are some things we might want to do in the line of changing the appearance or wording of error and warning messages around this behavior, which would be much simpler to do now that it's an explicitly modeled state. But I haven't done them here. Although this fixes a long-standing bug, no backpatch. The consequences of the bug don't seem severe enough to justify the risk that this commit itself creates some new issue. Patch by me, but it owes something to previous investigation by Takayuki Tsunakawa, who also reported the bug in the first place. Also thanks to Michael Paquier for reviewing. Discussion: https://postgr.es/m/0A3221C70F24FB45833433255569204D1F6BE40D@G01JPEXMBYT05 --- src/backend/tcop/postgres.c | 57 ++++++++++++++++++++++++++++----------------- 1 file changed, 35 insertions(+), 22 deletions(-) (limited to 'src/backend/tcop/postgres.c') diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c index 8d3fecf6d6a..c10d891260c 100644 --- a/src/backend/tcop/postgres.c +++ b/src/backend/tcop/postgres.c @@ -883,10 +883,9 @@ exec_simple_query(const char *query_string) ListCell *parsetree_item; bool save_log_statement_stats = log_statement_stats; bool was_logged = false; - bool isTopLevel; + bool use_implicit_block; char msec_str[32]; - /* * Report query to various monitoring facilities. */ @@ -947,13 +946,14 @@ exec_simple_query(const char *query_string) MemoryContextSwitchTo(oldcontext); /* - * We'll tell PortalRun it's a top-level command iff there's exactly one - * raw parsetree. If more than one, it's effectively a transaction block - * and we want PreventTransactionChain to reject unsafe commands. (Note: - * we're assuming that query rewrite cannot add commands that are - * significant to PreventTransactionChain.) + * For historical reasons, if multiple SQL statements are given in a + * single "simple Query" message, we execute them as a single transaction, + * unless explicit transaction control commands are included to make + * portions of the list be separate transactions. To represent this + * behavior properly in the transaction machinery, we use an "implicit" + * transaction block. */ - isTopLevel = (list_length(parsetree_list) == 1); + use_implicit_block = (list_length(parsetree_list) > 1); /* * Run through the raw parsetree(s) and process each one. @@ -1001,6 +1001,16 @@ exec_simple_query(const char *query_string) /* Make sure we are in a transaction command */ start_xact_command(); + /* + * If using an implicit transaction block, and we're not already in a + * transaction block, start an implicit block to force this statement + * to be grouped together with any following ones. (We must do this + * each time through the loop; otherwise, a COMMIT/ROLLBACK in the + * list would cause later statements to not be grouped.) + */ + if (use_implicit_block) + BeginImplicitTransactionBlock(); + /* If we got a cancel signal in parsing or prior command, quit */ CHECK_FOR_INTERRUPTS(); @@ -1098,7 +1108,7 @@ exec_simple_query(const char *query_string) */ (void) PortalRun(portal, FETCH_ALL, - isTopLevel, + true, /* always top level */ true, receiver, receiver, @@ -1108,15 +1118,7 @@ exec_simple_query(const char *query_string) PortalDrop(portal, false); - if (IsA(parsetree->stmt, TransactionStmt)) - { - /* - * If this was a transaction control statement, commit it. We will - * start a new xact command for the next command (if any). - */ - finish_xact_command(); - } - else if (lnext(parsetree_item) == NULL) + if (lnext(parsetree_item) == NULL) { /* * If this is the last parsetree of the query string, close down @@ -1124,9 +1126,18 @@ exec_simple_query(const char *query_string) * is so that any end-of-transaction errors are reported before * the command-complete message is issued, to avoid confusing * clients who will expect either a command-complete message or an - * error, not one and then the other. But for compatibility with - * historical Postgres behavior, we do not force a transaction - * boundary between queries appearing in a single query string. + * error, not one and then the other. Also, if we're using an + * implicit transaction block, we must close that out first. + */ + if (use_implicit_block) + EndImplicitTransactionBlock(); + finish_xact_command(); + } + else if (IsA(parsetree->stmt, TransactionStmt)) + { + /* + * If this was a transaction control statement, commit it. We will + * start a new xact command for the next command. */ finish_xact_command(); } @@ -1149,7 +1160,9 @@ exec_simple_query(const char *query_string) } /* end loop over parsetrees */ /* - * Close down transaction statement, if one is open. + * Close down transaction statement, if one is open. (This will only do + * something if the parsetree list was empty; otherwise the last loop + * iteration already did it.) */ finish_xact_command(); -- cgit v1.2.3