rdelaage
/
postgresql
mirror of https://git.postgresql.org/git/postgresql.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

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
This commit is contained in:
Tom Lane 2017-09-07 09:49:55 -04:00
parent bfea92563c
commit 6eb52da394
5 changed files with 320 additions and 37 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

160
src/backend/access/transam/xact.c
View File

@ -145,6 +145,7 @@ typedef enum TBlockState
/* transaction block states */
TBLOCK_BEGIN, /* starting transaction block */
TBLOCK_INPROGRESS, /* live transaction */
TBLOCK_IMPLICIT_INPROGRESS, /* live transaction after implicit BEGIN */
TBLOCK_PARALLEL_INPROGRESS, /* live transaction inside parallel worker */
TBLOCK_END, /* COMMIT received */
TBLOCK_ABORT, /* failed xact, awaiting ROLLBACK */
@ -2700,6 +2701,7 @@ StartTransactionCommand(void)
* previous CommitTransactionCommand.)
*/
case TBLOCK_INPROGRESS:
case TBLOCK_IMPLICIT_INPROGRESS:
case TBLOCK_SUBINPROGRESS:
break;
@ -2790,6 +2792,7 @@ CommitTransactionCommand(void)
* counter and return.
*/
case TBLOCK_INPROGRESS:
case TBLOCK_IMPLICIT_INPROGRESS:
case TBLOCK_SUBINPROGRESS:
CommandCounterIncrement();
break;
@ -3014,10 +3017,12 @@ AbortCurrentTransaction(void)
break;
/*
* if we aren't in a transaction block, we just do the basic abort
* & cleanup transaction.
* If we aren't in a transaction block, we just do the basic abort
* & cleanup transaction. For this purpose, we treat an implicit
* transaction block as if it were a simple statement.
*/
case TBLOCK_STARTED:
case TBLOCK_IMPLICIT_INPROGRESS:
AbortTransaction();
CleanupTransaction();
s->blockState = TBLOCK_DEFAULT;
@ -3148,9 +3153,8 @@ AbortCurrentTransaction(void)
* completes). Subtransactions are verboten too.
*
* isTopLevel: passed down from ProcessUtility to determine whether we are
* inside a function or multi-query querystring. (We will always fail if
* this is false, but it's convenient to centralize the check here instead of
* making callers do it.)
* inside a function. (We will always fail if this is false, but it's
* convenient to centralize the check here instead of making callers do it.)
* stmtType: statement type name, for error messages.
*/
void
@ -3183,8 +3187,7 @@ PreventTransactionChain(bool isTopLevel, const char *stmtType)
ereport(ERROR,
(errcode(ERRCODE_ACTIVE_SQL_TRANSACTION),
/* translator: %s represents an SQL statement name */
errmsg("%s cannot be executed from a function or multi-command string",
stmtType)));
errmsg("%s cannot be executed from a function", stmtType)));
/* If we got past IsTransactionBlock test, should be in default state */
if (CurrentTransactionState->blockState != TBLOCK_DEFAULT &&
@ -3428,6 +3431,15 @@ BeginTransactionBlock(void)
s->blockState = TBLOCK_BEGIN;
break;
/*
* BEGIN converts an implicit transaction block to a regular one.
* (Note that we allow this even if we've already done some
* commands, which is a bit odd but matches historical practice.)
*/
case TBLOCK_IMPLICIT_INPROGRESS:
s->blockState = TBLOCK_BEGIN;
break;
/*
* Already a transaction block in progress.
*/
@ -3503,7 +3515,8 @@ PrepareTransactionBlock(char *gid)
* ignore case where we are not in a transaction;
* EndTransactionBlock already issued a warning.
*/
Assert(s->blockState == TBLOCK_STARTED);
Assert(s->blockState == TBLOCK_STARTED ||
s->blockState == TBLOCK_IMPLICIT_INPROGRESS);
/* Don't send back a PREPARE result tag... */
result = false;
}
@ -3541,6 +3554,18 @@ EndTransactionBlock(void)
result = true;
break;
/*
* In an implicit transaction block, commit, but issue a warning
* because there was no explicit BEGIN before this.
*/
case TBLOCK_IMPLICIT_INPROGRESS:
ereport(WARNING,
(errcode(ERRCODE_NO_ACTIVE_SQL_TRANSACTION),
errmsg("there is no transaction in progress")));
s->blockState = TBLOCK_END;
result = true;
break;
/*
* We are in a failed transaction block. Tell
* CommitTransactionCommand it's time to exit the block.
@ -3705,8 +3730,14 @@ UserAbortTransactionBlock(void)
* WARNING and go to abort state. The upcoming call to
* CommitTransactionCommand() will then put us back into the
* default state.
*
* We do the same thing with ABORT inside an implicit transaction,
* although in this case we might be rolling back actual database
* state changes. (It's debatable whether we should issue a
* WARNING in this case, but we have done so historically.)
*/
case TBLOCK_STARTED:
case TBLOCK_IMPLICIT_INPROGRESS:
ereport(WARNING,
(errcode(ERRCODE_NO_ACTIVE_SQL_TRANSACTION),
errmsg("there is no transaction in progress")));
@ -3743,6 +3774,58 @@ UserAbortTransactionBlock(void)
}
}
/*
* BeginImplicitTransactionBlock
* Start an implicit transaction block if we're not already in one.
*
* Unlike BeginTransactionBlock, this is called directly from the main loop
* in postgres.c, not within a Portal. So we can just change blockState
* without a lot of ceremony. We do not expect caller to do
* CommitTransactionCommand/StartTransactionCommand.
*/
void
BeginImplicitTransactionBlock(void)
{
TransactionState s = CurrentTransactionState;
/*
* If we are in STARTED state (that is, no transaction block is open),
* switch to IMPLICIT_INPROGRESS state, creating an implicit transaction
* block.
*
* For caller convenience, we consider all other transaction states as
* legal here; otherwise the caller would need its own state check, which
* seems rather pointless.
*/
if (s->blockState == TBLOCK_STARTED)
s->blockState = TBLOCK_IMPLICIT_INPROGRESS;
}
/*
* EndImplicitTransactionBlock
* End an implicit transaction block, if we're in one.
*
* Like EndTransactionBlock, we just make any needed blockState change here.
* The real work will be done in the upcoming CommitTransactionCommand().
*/
void
EndImplicitTransactionBlock(void)
{
TransactionState s = CurrentTransactionState;
/*
* If we are in IMPLICIT_INPROGRESS state, switch back to STARTED state,
* allowing CommitTransactionCommand to commit whatever happened during
* the implicit transaction block as though it were a single statement.
*
* For caller convenience, we consider all other transaction states as
* legal here; otherwise the caller would need its own state check, which
* seems rather pointless.
*/
if (s->blockState == TBLOCK_IMPLICIT_INPROGRESS)
s->blockState = TBLOCK_STARTED;
}
/*
* DefineSavepoint
* This executes a SAVEPOINT command.
@ -3780,6 +3863,28 @@ DefineSavepoint(char *name)
s->name = MemoryContextStrdup(TopTransactionContext, name);
break;
/*
* We disallow savepoint commands in implicit transaction blocks.
* There would be no great difficulty in allowing them so far as
* this module is concerned, but a savepoint seems inconsistent
* with exec_simple_query's behavior of abandoning the whole query
* string upon error. Also, the point of an implicit transaction
* block (as opposed to a regular one) is to automatically close
* after an error, so it's hard to see how a savepoint would fit
* into that.
*
* The error messages for this are phrased as if there were no
* active transaction block at all, which is historical but
* perhaps could be improved.
*/
case TBLOCK_IMPLICIT_INPROGRESS:
ereport(ERROR,
(errcode(ERRCODE_NO_ACTIVE_SQL_TRANSACTION),
/* translator: %s represents an SQL statement name */
errmsg("%s can only be used in transaction blocks",
"SAVEPOINT")));
break;
/* These cases are invalid. */
case TBLOCK_DEFAULT:
case TBLOCK_STARTED:
@ -3834,8 +3939,7 @@ ReleaseSavepoint(List *options)
switch (s->blockState)
{
/*
* We can't rollback to a savepoint if there is no savepoint
* defined.
* We can't release a savepoint if there is no savepoint defined.
*/
case TBLOCK_INPROGRESS:
ereport(ERROR,
@ -3843,6 +3947,15 @@ ReleaseSavepoint(List *options)
errmsg("no such savepoint")));
break;
case TBLOCK_IMPLICIT_INPROGRESS:
/* See comment about implicit transactions in DefineSavepoint */
ereport(ERROR,
(errcode(ERRCODE_NO_ACTIVE_SQL_TRANSACTION),
/* translator: %s represents an SQL statement name */
errmsg("%s can only be used in transaction blocks",
"RELEASE SAVEPOINT")));
break;
/*
* We are in a non-aborted subtransaction. This is the only valid
* case.
@ -3957,6 +4070,15 @@ RollbackToSavepoint(List *options)
errmsg("no such savepoint")));
break;
case TBLOCK_IMPLICIT_INPROGRESS:
/* See comment about implicit transactions in DefineSavepoint */
ereport(ERROR,
(errcode(ERRCODE_NO_ACTIVE_SQL_TRANSACTION),
/* translator: %s represents an SQL statement name */
errmsg("%s can only be used in transaction blocks",
"ROLLBACK TO SAVEPOINT")));
break;
/*
* There is at least one savepoint, so proceed.
*/
@ -4046,11 +4168,12 @@ RollbackToSavepoint(List *options)
/*
* BeginInternalSubTransaction
* This is the same as DefineSavepoint except it allows TBLOCK_STARTED,
* TBLOCK_END, and TBLOCK_PREPARE states, and therefore it can safely be
* used in functions that might be called when not inside a BEGIN block
* or when running deferred triggers at COMMIT/PREPARE time. Also, it
* automatically does CommitTransactionCommand/StartTransactionCommand
* instead of expecting the caller to do it.
* TBLOCK_IMPLICIT_INPROGRESS, TBLOCK_END, and TBLOCK_PREPARE states,
* and therefore it can safely be used in functions that might be called
* when not inside a BEGIN block or when running deferred triggers at
* COMMIT/PREPARE time. Also, it automatically does
* CommitTransactionCommand/StartTransactionCommand instead of expecting
* the caller to do it.
*/
void
BeginInternalSubTransaction(char *name)
@ -4076,6 +4199,7 @@ BeginInternalSubTransaction(char *name)
{
case TBLOCK_STARTED:
case TBLOCK_INPROGRESS:
case TBLOCK_IMPLICIT_INPROGRESS:
case TBLOCK_END:
case TBLOCK_PREPARE:
case TBLOCK_SUBINPROGRESS:
@ -4180,6 +4304,7 @@ RollbackAndReleaseCurrentSubTransaction(void)
case TBLOCK_DEFAULT:
case TBLOCK_STARTED:
case TBLOCK_BEGIN:
case TBLOCK_IMPLICIT_INPROGRESS:
case TBLOCK_PARALLEL_INPROGRESS:
case TBLOCK_SUBBEGIN:
case TBLOCK_INPROGRESS:
@ -4211,6 +4336,7 @@ RollbackAndReleaseCurrentSubTransaction(void)
s = CurrentTransactionState; /* changed by pop */
AssertState(s->blockState == TBLOCK_SUBINPROGRESS ||
s->blockState == TBLOCK_INPROGRESS ||
s->blockState == TBLOCK_IMPLICIT_INPROGRESS ||
s->blockState == TBLOCK_STARTED);
}
@ -4259,6 +4385,7 @@ AbortOutOfAnyTransaction(void)
case TBLOCK_STARTED:
case TBLOCK_BEGIN:
case TBLOCK_INPROGRESS:
case TBLOCK_IMPLICIT_INPROGRESS:
case TBLOCK_PARALLEL_INPROGRESS:
case TBLOCK_END:
case TBLOCK_ABORT_PENDING:
@ -4369,6 +4496,7 @@ TransactionBlockStatusCode(void)
case TBLOCK_BEGIN:
case TBLOCK_SUBBEGIN:
case TBLOCK_INPROGRESS:
case TBLOCK_IMPLICIT_INPROGRESS:
case TBLOCK_PARALLEL_INPROGRESS:
case TBLOCK_SUBINPROGRESS:
case TBLOCK_END:
@ -5036,6 +5164,8 @@ BlockStateAsString(TBlockState blockState)
return "BEGIN";
case TBLOCK_INPROGRESS:
return "INPROGRESS";
case TBLOCK_IMPLICIT_INPROGRESS:
return "IMPLICIT_INPROGRESS";
case TBLOCK_PARALLEL_INPROGRESS:
return "PARALLEL_INPROGRESS";
case TBLOCK_END:

57
src/backend/tcop/postgres.c
View File

@ -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();

2
src/include/access/xact.h
View File

@ -352,6 +352,8 @@ extern void BeginTransactionBlock(void);
extern bool EndTransactionBlock(void);
extern bool PrepareTransactionBlock(char *gid);
extern void UserAbortTransactionBlock(void);
extern void BeginImplicitTransactionBlock(void);
extern void EndImplicitTransactionBlock(void);
extern void ReleaseSavepoint(List *options);
extern void DefineSavepoint(char *name);
extern void RollbackToSavepoint(List *options);

84
src/test/regress/expected/transactions.out
View File

@ -659,6 +659,90 @@ ERROR: portal "ctt" cannot be run
COMMIT;
DROP FUNCTION create_temp_tab();
DROP FUNCTION invert(x float8);
-- Test assorted behaviors around the implicit transaction block created
-- when multiple SQL commands are sent in a single Query message. These
-- tests rely on the fact that psql will not break SQL commands apart at a
-- backslash-quoted semicolon, but will send them as one Query.
create temp table i_table (f1 int);
-- psql will show only the last result in a multi-statement Query
SELECT 1\; SELECT 2\; SELECT 3;
?column?
----------
3
(1 row)
-- this implicitly commits:
insert into i_table values(1)\; select * from i_table;
f1
----
1
(1 row)
-- 1/0 error will cause rolling back the whole implicit transaction
insert into i_table values(2)\; select * from i_table\; select 1/0;
ERROR: division by zero
select * from i_table;
f1
----
1
(1 row)
rollback; -- we are not in a transaction at this point
WARNING: there is no transaction in progress
-- can use regular begin/commit/rollback within a single Query
begin\; insert into i_table values(3)\; commit;
rollback; -- we are not in a transaction at this point
WARNING: there is no transaction in progress
begin\; insert into i_table values(4)\; rollback;
rollback; -- we are not in a transaction at this point
WARNING: there is no transaction in progress
-- begin converts implicit transaction into a regular one that
-- can extend past the end of the Query
select 1\; begin\; insert into i_table values(5);
commit;
select 1\; begin\; insert into i_table values(6);
rollback;
-- commit in implicit-transaction state commits but issues a warning.
insert into i_table values(7)\; commit\; insert into i_table values(8)\; select 1/0;
WARNING: there is no transaction in progress
ERROR: division by zero
-- similarly, rollback aborts but issues a warning.
insert into i_table values(9)\; rollback\; select 2;
WARNING: there is no transaction in progress
?column?
----------
2
(1 row)
select * from i_table;
f1
----
1
3
5
7
(4 rows)
rollback; -- we are not in a transaction at this point
WARNING: there is no transaction in progress
-- implicit transaction block is still a transaction block, for e.g. VACUUM
SELECT 1\; VACUUM;
ERROR: VACUUM cannot run inside a transaction block
SELECT 1\; COMMIT\; VACUUM;
WARNING: there is no transaction in progress
ERROR: VACUUM cannot run inside a transaction block
-- we disallow savepoint-related commands in implicit-transaction state
SELECT 1\; SAVEPOINT sp;
ERROR: SAVEPOINT can only be used in transaction blocks
SELECT 1\; COMMIT\; SAVEPOINT sp;
WARNING: there is no transaction in progress
ERROR: SAVEPOINT can only be used in transaction blocks
ROLLBACK TO SAVEPOINT sp\; SELECT 2;
ERROR: ROLLBACK TO SAVEPOINT can only be used in transaction blocks
SELECT 2\; RELEASE SAVEPOINT sp\; SELECT 3;
ERROR: RELEASE SAVEPOINT can only be used in transaction blocks
-- but this is OK, because the BEGIN converts it to a regular xact
SELECT 1\; BEGIN\; SAVEPOINT sp\; ROLLBACK TO SAVEPOINT sp\; COMMIT;
-- Test for successful cleanup of an aborted transaction at session exit.
-- THIS MUST BE THE LAST TEST IN THIS FILE.
begin;

54
src/test/regress/sql/transactions.sql
View File

@ -419,6 +419,60 @@ DROP FUNCTION create_temp_tab();
DROP FUNCTION invert(x float8);
-- Test assorted behaviors around the implicit transaction block created
-- when multiple SQL commands are sent in a single Query message. These
-- tests rely on the fact that psql will not break SQL commands apart at a
-- backslash-quoted semicolon, but will send them as one Query.
create temp table i_table (f1 int);
-- psql will show only the last result in a multi-statement Query
SELECT 1\; SELECT 2\; SELECT 3;
-- this implicitly commits:
insert into i_table values(1)\; select * from i_table;
-- 1/0 error will cause rolling back the whole implicit transaction
insert into i_table values(2)\; select * from i_table\; select 1/0;
select * from i_table;
rollback; -- we are not in a transaction at this point
-- can use regular begin/commit/rollback within a single Query
begin\; insert into i_table values(3)\; commit;
rollback; -- we are not in a transaction at this point
begin\; insert into i_table values(4)\; rollback;
rollback; -- we are not in a transaction at this point
-- begin converts implicit transaction into a regular one that
-- can extend past the end of the Query
select 1\; begin\; insert into i_table values(5);
commit;
select 1\; begin\; insert into i_table values(6);
rollback;
-- commit in implicit-transaction state commits but issues a warning.
insert into i_table values(7)\; commit\; insert into i_table values(8)\; select 1/0;
-- similarly, rollback aborts but issues a warning.
insert into i_table values(9)\; rollback\; select 2;
select * from i_table;
rollback; -- we are not in a transaction at this point
-- implicit transaction block is still a transaction block, for e.g. VACUUM
SELECT 1\; VACUUM;
SELECT 1\; COMMIT\; VACUUM;
-- we disallow savepoint-related commands in implicit-transaction state
SELECT 1\; SAVEPOINT sp;
SELECT 1\; COMMIT\; SAVEPOINT sp;
ROLLBACK TO SAVEPOINT sp\; SELECT 2;
SELECT 2\; RELEASE SAVEPOINT sp\; SELECT 3;
-- but this is OK, because the BEGIN converts it to a regular xact
SELECT 1\; BEGIN\; SAVEPOINT sp\; ROLLBACK TO SAVEPOINT sp\; COMMIT;
-- Test for successful cleanup of an aborted transaction at session exit.
-- THIS MUST BE THE LAST TEST IN THIS FILE.