Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Commit6eb52da

Browse files
committed
Fix handling of savepoint commands within multi-statement Query strings.
Issuing a savepoint-related command in a Query message that containsmultiple SQL statements led to a FATAL exit with a complaint about"unexpected state STARTED". This is a shortcoming of commit4f896da,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 descriptionof the situation anyway.To fix, let's introduce into xact.c an explicit model of what happens formulti-statement Query strings. This is an "implicit transaction blockin progress" state, which for many purposes works like the normalTBLOCK_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 thetransaction to be cancelled without need for an explicit ROLLBACK command.Commit4f896da is reverted in toto, so that we go back to treating theindividual statements as "top level". We could have left it as-is, butthis allows sharpening the error message for PreventTransactionChaincalls inside functions.Except for getting a normal error instead of a FATAL exit for savepointcommands, this patch should result in no user-visible behavioral change(other than that one error message rewording). There are some thingswe might want to do in the line of changing the appearance or wording oferror and warning messages around this behavior, which would be muchsimpler to do now that it's an explicitly modeled state. But I haven'tdone them here.Although this fixes a long-standing bug, no backpatch. The consequencesof the bug don't seem severe enough to justify the risk that this commititself creates some new issue.Patch by me, but it owes something to previous investigation byTakayuki Tsunakawa, who also reported the bug in the first place.Also thanks to Michael Paquier for reviewing.Discussion:https://postgr.es/m/0A3221C70F24FB45833433255569204D1F6BE40D@G01JPEXMBYT05
1 parentbfea925 commit6eb52da

File tree

5 files changed

+320
-37
lines changed

5 files changed

+320
-37
lines changed

‎src/backend/access/transam/xact.c‎

Lines changed: 145 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -145,6 +145,7 @@ typedef enum TBlockState
145145
/* transaction block states */
146146
TBLOCK_BEGIN,/* starting transaction block */
147147
TBLOCK_INPROGRESS,/* live transaction */
148+
TBLOCK_IMPLICIT_INPROGRESS,/* live transaction after implicit BEGIN */
148149
TBLOCK_PARALLEL_INPROGRESS,/* live transaction inside parallel worker */
149150
TBLOCK_END,/* COMMIT received */
150151
TBLOCK_ABORT,/* failed xact, awaiting ROLLBACK */
@@ -2700,6 +2701,7 @@ StartTransactionCommand(void)
27002701
* previous CommitTransactionCommand.)
27012702
*/
27022703
caseTBLOCK_INPROGRESS:
2704+
caseTBLOCK_IMPLICIT_INPROGRESS:
27032705
caseTBLOCK_SUBINPROGRESS:
27042706
break;
27052707

@@ -2790,6 +2792,7 @@ CommitTransactionCommand(void)
27902792
* counter and return.
27912793
*/
27922794
caseTBLOCK_INPROGRESS:
2795+
caseTBLOCK_IMPLICIT_INPROGRESS:
27932796
caseTBLOCK_SUBINPROGRESS:
27942797
CommandCounterIncrement();
27952798
break;
@@ -3014,10 +3017,12 @@ AbortCurrentTransaction(void)
30143017
break;
30153018

30163019
/*
3017-
* if we aren't in a transaction block, we just do the basic abort
3018-
* & cleanup transaction.
3020+
* If we aren't in a transaction block, we just do the basic abort
3021+
* & cleanup transaction. For this purpose, we treat an implicit
3022+
* transaction block as if it were a simple statement.
30193023
*/
30203024
caseTBLOCK_STARTED:
3025+
caseTBLOCK_IMPLICIT_INPROGRESS:
30213026
AbortTransaction();
30223027
CleanupTransaction();
30233028
s->blockState=TBLOCK_DEFAULT;
@@ -3148,9 +3153,8 @@ AbortCurrentTransaction(void)
31483153
*completes). Subtransactions are verboten too.
31493154
*
31503155
*isTopLevel: passed down from ProcessUtility to determine whether we are
3151-
*inside a function or multi-query querystring. (We will always fail if
3152-
*this is false, but it's convenient to centralize the check here instead of
3153-
*making callers do it.)
3156+
*inside a function. (We will always fail if this is false, but it's
3157+
*convenient to centralize the check here instead of making callers do it.)
31543158
*stmtType: statement type name, for error messages.
31553159
*/
31563160
void
@@ -3183,8 +3187,7 @@ PreventTransactionChain(bool isTopLevel, const char *stmtType)
31833187
ereport(ERROR,
31843188
(errcode(ERRCODE_ACTIVE_SQL_TRANSACTION),
31853189
/* translator: %s represents an SQL statement name */
3186-
errmsg("%s cannot be executed from a function or multi-command string",
3187-
stmtType)));
3190+
errmsg("%s cannot be executed from a function",stmtType)));
31883191

31893192
/* If we got past IsTransactionBlock test, should be in default state */
31903193
if (CurrentTransactionState->blockState!=TBLOCK_DEFAULT&&
@@ -3428,6 +3431,15 @@ BeginTransactionBlock(void)
34283431
s->blockState=TBLOCK_BEGIN;
34293432
break;
34303433

3434+
/*
3435+
* BEGIN converts an implicit transaction block to a regular one.
3436+
* (Note that we allow this even if we've already done some
3437+
* commands, which is a bit odd but matches historical practice.)
3438+
*/
3439+
caseTBLOCK_IMPLICIT_INPROGRESS:
3440+
s->blockState=TBLOCK_BEGIN;
3441+
break;
3442+
34313443
/*
34323444
* Already a transaction block in progress.
34333445
*/
@@ -3503,7 +3515,8 @@ PrepareTransactionBlock(char *gid)
35033515
* ignore case where we are not in a transaction;
35043516
* EndTransactionBlock already issued a warning.
35053517
*/
3506-
Assert(s->blockState==TBLOCK_STARTED);
3518+
Assert(s->blockState==TBLOCK_STARTED||
3519+
s->blockState==TBLOCK_IMPLICIT_INPROGRESS);
35073520
/* Don't send back a PREPARE result tag... */
35083521
result= false;
35093522
}
@@ -3541,6 +3554,18 @@ EndTransactionBlock(void)
35413554
result= true;
35423555
break;
35433556

3557+
/*
3558+
* In an implicit transaction block, commit, but issue a warning
3559+
* because there was no explicit BEGIN before this.
3560+
*/
3561+
caseTBLOCK_IMPLICIT_INPROGRESS:
3562+
ereport(WARNING,
3563+
(errcode(ERRCODE_NO_ACTIVE_SQL_TRANSACTION),
3564+
errmsg("there is no transaction in progress")));
3565+
s->blockState=TBLOCK_END;
3566+
result= true;
3567+
break;
3568+
35443569
/*
35453570
* We are in a failed transaction block. Tell
35463571
* CommitTransactionCommand it's time to exit the block.
@@ -3705,8 +3730,14 @@ UserAbortTransactionBlock(void)
37053730
* WARNING and go to abort state. The upcoming call to
37063731
* CommitTransactionCommand() will then put us back into the
37073732
* default state.
3733+
*
3734+
* We do the same thing with ABORT inside an implicit transaction,
3735+
* although in this case we might be rolling back actual database
3736+
* state changes. (It's debatable whether we should issue a
3737+
* WARNING in this case, but we have done so historically.)
37083738
*/
37093739
caseTBLOCK_STARTED:
3740+
caseTBLOCK_IMPLICIT_INPROGRESS:
37103741
ereport(WARNING,
37113742
(errcode(ERRCODE_NO_ACTIVE_SQL_TRANSACTION),
37123743
errmsg("there is no transaction in progress")));
@@ -3743,6 +3774,58 @@ UserAbortTransactionBlock(void)
37433774
}
37443775
}
37453776

3777+
/*
3778+
* BeginImplicitTransactionBlock
3779+
*Start an implicit transaction block if we're not already in one.
3780+
*
3781+
* Unlike BeginTransactionBlock, this is called directly from the main loop
3782+
* in postgres.c, not within a Portal. So we can just change blockState
3783+
* without a lot of ceremony. We do not expect caller to do
3784+
* CommitTransactionCommand/StartTransactionCommand.
3785+
*/
3786+
void
3787+
BeginImplicitTransactionBlock(void)
3788+
{
3789+
TransactionStates=CurrentTransactionState;
3790+
3791+
/*
3792+
* If we are in STARTED state (that is, no transaction block is open),
3793+
* switch to IMPLICIT_INPROGRESS state, creating an implicit transaction
3794+
* block.
3795+
*
3796+
* For caller convenience, we consider all other transaction states as
3797+
* legal here; otherwise the caller would need its own state check, which
3798+
* seems rather pointless.
3799+
*/
3800+
if (s->blockState==TBLOCK_STARTED)
3801+
s->blockState=TBLOCK_IMPLICIT_INPROGRESS;
3802+
}
3803+
3804+
/*
3805+
* EndImplicitTransactionBlock
3806+
*End an implicit transaction block, if we're in one.
3807+
*
3808+
* Like EndTransactionBlock, we just make any needed blockState change here.
3809+
* The real work will be done in the upcoming CommitTransactionCommand().
3810+
*/
3811+
void
3812+
EndImplicitTransactionBlock(void)
3813+
{
3814+
TransactionStates=CurrentTransactionState;
3815+
3816+
/*
3817+
* If we are in IMPLICIT_INPROGRESS state, switch back to STARTED state,
3818+
* allowing CommitTransactionCommand to commit whatever happened during
3819+
* the implicit transaction block as though it were a single statement.
3820+
*
3821+
* For caller convenience, we consider all other transaction states as
3822+
* legal here; otherwise the caller would need its own state check, which
3823+
* seems rather pointless.
3824+
*/
3825+
if (s->blockState==TBLOCK_IMPLICIT_INPROGRESS)
3826+
s->blockState=TBLOCK_STARTED;
3827+
}
3828+
37463829
/*
37473830
* DefineSavepoint
37483831
*This executes a SAVEPOINT command.
@@ -3780,6 +3863,28 @@ DefineSavepoint(char *name)
37803863
s->name=MemoryContextStrdup(TopTransactionContext,name);
37813864
break;
37823865

3866+
/*
3867+
* We disallow savepoint commands in implicit transaction blocks.
3868+
* There would be no great difficulty in allowing them so far as
3869+
* this module is concerned, but a savepoint seems inconsistent
3870+
* with exec_simple_query's behavior of abandoning the whole query
3871+
* string upon error. Also, the point of an implicit transaction
3872+
* block (as opposed to a regular one) is to automatically close
3873+
* after an error, so it's hard to see how a savepoint would fit
3874+
* into that.
3875+
*
3876+
* The error messages for this are phrased as if there were no
3877+
* active transaction block at all, which is historical but
3878+
* perhaps could be improved.
3879+
*/
3880+
caseTBLOCK_IMPLICIT_INPROGRESS:
3881+
ereport(ERROR,
3882+
(errcode(ERRCODE_NO_ACTIVE_SQL_TRANSACTION),
3883+
/* translator: %s represents an SQL statement name */
3884+
errmsg("%s can only be used in transaction blocks",
3885+
"SAVEPOINT")));
3886+
break;
3887+
37833888
/* These cases are invalid. */
37843889
caseTBLOCK_DEFAULT:
37853890
caseTBLOCK_STARTED:
@@ -3834,15 +3939,23 @@ ReleaseSavepoint(List *options)
38343939
switch (s->blockState)
38353940
{
38363941
/*
3837-
* We can't rollback to a savepoint if there is no savepoint
3838-
* defined.
3942+
* We can't release a savepoint if there is no savepoint defined.
38393943
*/
38403944
caseTBLOCK_INPROGRESS:
38413945
ereport(ERROR,
38423946
(errcode(ERRCODE_S_E_INVALID_SPECIFICATION),
38433947
errmsg("no such savepoint")));
38443948
break;
38453949

3950+
caseTBLOCK_IMPLICIT_INPROGRESS:
3951+
/* See comment about implicit transactions in DefineSavepoint */
3952+
ereport(ERROR,
3953+
(errcode(ERRCODE_NO_ACTIVE_SQL_TRANSACTION),
3954+
/* translator: %s represents an SQL statement name */
3955+
errmsg("%s can only be used in transaction blocks",
3956+
"RELEASE SAVEPOINT")));
3957+
break;
3958+
38463959
/*
38473960
* We are in a non-aborted subtransaction. This is the only valid
38483961
* case.
@@ -3957,6 +4070,15 @@ RollbackToSavepoint(List *options)
39574070
errmsg("no such savepoint")));
39584071
break;
39594072

4073+
caseTBLOCK_IMPLICIT_INPROGRESS:
4074+
/* See comment about implicit transactions in DefineSavepoint */
4075+
ereport(ERROR,
4076+
(errcode(ERRCODE_NO_ACTIVE_SQL_TRANSACTION),
4077+
/* translator: %s represents an SQL statement name */
4078+
errmsg("%s can only be used in transaction blocks",
4079+
"ROLLBACK TO SAVEPOINT")));
4080+
break;
4081+
39604082
/*
39614083
* There is at least one savepoint, so proceed.
39624084
*/
@@ -4046,11 +4168,12 @@ RollbackToSavepoint(List *options)
40464168
/*
40474169
* BeginInternalSubTransaction
40484170
*This is the same as DefineSavepoint except it allows TBLOCK_STARTED,
4049-
*TBLOCK_END, and TBLOCK_PREPARE states, and therefore it can safely be
4050-
*used in functions that might be called when not inside a BEGIN block
4051-
*or when running deferred triggers at COMMIT/PREPARE time. Also, it
4052-
*automatically does CommitTransactionCommand/StartTransactionCommand
4053-
*instead of expecting the caller to do it.
4171+
*TBLOCK_IMPLICIT_INPROGRESS, TBLOCK_END, and TBLOCK_PREPARE states,
4172+
*and therefore it can safely be used in functions that might be called
4173+
*when not inside a BEGIN block or when running deferred triggers at
4174+
*COMMIT/PREPARE time. Also, it automatically does
4175+
*CommitTransactionCommand/StartTransactionCommand instead of expecting
4176+
*the caller to do it.
40544177
*/
40554178
void
40564179
BeginInternalSubTransaction(char*name)
@@ -4076,6 +4199,7 @@ BeginInternalSubTransaction(char *name)
40764199
{
40774200
caseTBLOCK_STARTED:
40784201
caseTBLOCK_INPROGRESS:
4202+
caseTBLOCK_IMPLICIT_INPROGRESS:
40794203
caseTBLOCK_END:
40804204
caseTBLOCK_PREPARE:
40814205
caseTBLOCK_SUBINPROGRESS:
@@ -4180,6 +4304,7 @@ RollbackAndReleaseCurrentSubTransaction(void)
41804304
caseTBLOCK_DEFAULT:
41814305
caseTBLOCK_STARTED:
41824306
caseTBLOCK_BEGIN:
4307+
caseTBLOCK_IMPLICIT_INPROGRESS:
41834308
caseTBLOCK_PARALLEL_INPROGRESS:
41844309
caseTBLOCK_SUBBEGIN:
41854310
caseTBLOCK_INPROGRESS:
@@ -4211,6 +4336,7 @@ RollbackAndReleaseCurrentSubTransaction(void)
42114336
s=CurrentTransactionState;/* changed by pop */
42124337
AssertState(s->blockState==TBLOCK_SUBINPROGRESS||
42134338
s->blockState==TBLOCK_INPROGRESS||
4339+
s->blockState==TBLOCK_IMPLICIT_INPROGRESS||
42144340
s->blockState==TBLOCK_STARTED);
42154341
}
42164342

@@ -4259,6 +4385,7 @@ AbortOutOfAnyTransaction(void)
42594385
caseTBLOCK_STARTED:
42604386
caseTBLOCK_BEGIN:
42614387
caseTBLOCK_INPROGRESS:
4388+
caseTBLOCK_IMPLICIT_INPROGRESS:
42624389
caseTBLOCK_PARALLEL_INPROGRESS:
42634390
caseTBLOCK_END:
42644391
caseTBLOCK_ABORT_PENDING:
@@ -4369,6 +4496,7 @@ TransactionBlockStatusCode(void)
43694496
caseTBLOCK_BEGIN:
43704497
caseTBLOCK_SUBBEGIN:
43714498
caseTBLOCK_INPROGRESS:
4499+
caseTBLOCK_IMPLICIT_INPROGRESS:
43724500
caseTBLOCK_PARALLEL_INPROGRESS:
43734501
caseTBLOCK_SUBINPROGRESS:
43744502
caseTBLOCK_END:
@@ -5036,6 +5164,8 @@ BlockStateAsString(TBlockState blockState)
50365164
return"BEGIN";
50375165
caseTBLOCK_INPROGRESS:
50385166
return"INPROGRESS";
5167+
caseTBLOCK_IMPLICIT_INPROGRESS:
5168+
return"IMPLICIT_INPROGRESS";
50395169
caseTBLOCK_PARALLEL_INPROGRESS:
50405170
return"PARALLEL_INPROGRESS";
50415171
caseTBLOCK_END:

0 commit comments

Comments
 (0)
[8]ページ先頭
©2009-2025 Movatter.jp