Thread: Re: [HACKERS] [bug fix] Savepoint-related statements terminates connection
Re: [HACKERS] [bug fix] Savepoint-related statements terminates connection
From
Michael Paquier
Date:
On Fri, Mar 31, 2017 at 9:58 PM, Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> wrote: > Then the question is why not to allow savepoints as well? For that we > have to fix transaction block state machine. I agree with this argument. I have been looking at the patch, and what it does is definitely incorrect. Any query string including multiple queries sent to the server is executed as a single transaction. So, while the current behavior of the server is definitely incorrect for savepoints in this case, the proposed patch does not fix anything but actually makes things worse. I think that instead of failing, savepoints should be able to work properly. As you say cursors are handled correctly, savepoints should fall under the same rules. -- Michael
Re: [HACKERS] [bug fix] Savepoint-related statements terminatesconnection
From
"Tsunakawa, Takayuki"
Date:
From: Michael Paquier [mailto:michael.paquier@gmail.com] > On Fri, Mar 31, 2017 at 9:58 PM, Ashutosh Bapat > <ashutosh.bapat@enterprisedb.com> wrote: > > Then the question is why not to allow savepoints as well? For that we > > have to fix transaction block state machine. > > I agree with this argument. I have been looking at the patch, and what it > does is definitely incorrect. Any query string including multiple queries > sent to the server is executed as a single transaction. So, while the current > behavior of the server is definitely incorrect for savepoints in this case, > the proposed patch does not fix anything but actually makes things worse. > I think that instead of failing, savepoints should be able to work properly. > As you say cursors are handled correctly, savepoints should fall under the > same rules. Yes, I'm in favor of your opinion. I'll put more thought into whether it's feasible with invasive code. Regards Takayuki Tsunakawa
On 17 May 2017 at 08:38, Tsunakawa, Takayuki <tsunakawa.takay@jp.fujitsu.com> wrote: > From: Michael Paquier [mailto:michael.paquier@gmail.com] >> On Fri, Mar 31, 2017 at 9:58 PM, Ashutosh Bapat >> <ashutosh.bapat@enterprisedb.com> wrote: >> > Then the question is why not to allow savepoints as well? For that we >> > have to fix transaction block state machine. >> >> I agree with this argument. I have been looking at the patch, and what it >> does is definitely incorrect. Any query string including multiple queries >> sent to the server is executed as a single transaction. So, while the current >> behavior of the server is definitely incorrect for savepoints in this case, >> the proposed patch does not fix anything but actually makes things worse. >> I think that instead of failing, savepoints should be able to work properly. >> As you say cursors are handled correctly, savepoints should fall under the >> same rules. > > Yes, I'm in favor of your opinion. I'll put more thought into whether it's feasible with invasive code. I'm not sure I see the use case for anyone using SAVEPOINTs in this context, so simply throwing a good error message is enough. Clearly nobody is using this, so lets just lock the door. I don't think fiddling with the transaction block state machine is anything anybody wants to do in back branches, at least without a better reason than this. Simpler version of original patch attached. -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
Re: [HACKERS] [bug fix] Savepoint-related statements terminates connection
From
Michael Paquier
Date:
On Fri, Sep 1, 2017 at 3:05 PM, Simon Riggs <simon@2ndquadrant.com> wrote: > I'm not sure I see the use case for anyone using SAVEPOINTs in this > context, so simply throwing a good error message is enough. > > Clearly nobody is using this, so lets just lock the door. I don't > think fiddling with the transaction block state machine is anything > anybody wants to do in back branches, at least without a better reason > than this. I don't think you can say that, per se the following recent report: https://www.postgresql.org/message-id/CAH2-V61vxNEnTfj2V-zd+mA-g6kQMJgd5SvXoU3JBvdzQH0Yfw@mail.gmail.com -- Michael
On 1 September 2017 at 08:09, Michael Paquier <michael.paquier@gmail.com> wrote: > On Fri, Sep 1, 2017 at 3:05 PM, Simon Riggs <simon@2ndquadrant.com> wrote: >> I'm not sure I see the use case for anyone using SAVEPOINTs in this >> context, so simply throwing a good error message is enough. >> >> Clearly nobody is using this, so lets just lock the door. I don't >> think fiddling with the transaction block state machine is anything >> anybody wants to do in back branches, at least without a better reason >> than this. > > I don't think you can say that, per se the following recent report: > https://www.postgresql.org/message-id/CAH2-V61vxNEnTfj2V-zd+mA-g6kQMJgd5SvXoU3JBvdzQH0Yfw@mail.gmail.com AIUI, nobody is saying this should work, we're just discussing how to produce an error message. We should fix it, but not spend loads of time on it. I've added tests to the recent patch to show it works. Any objection to me backpatching this, please say. -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
Simon Riggs <simon@2ndquadrant.com> writes:
> I've added tests to the recent patch to show it works.
I don't think those test cases prove anything (ie, they work fine
on an unpatched server). With a backslash maybe they would.
> Any objection to me backpatching this, please say.
This patch makes me itch. Why is it correct for these three checks,
and only these three checks out of the couple dozen uses of isTopLevel
in standard_ProcessUtility, to instead do something else?
regards, tom lane
On 1 September 2017 at 15:19, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Simon Riggs <simon@2ndquadrant.com> writes: >> I've added tests to the recent patch to show it works. > > I don't think those test cases prove anything (ie, they work fine > on an unpatched server). With a backslash maybe they would. > >> Any objection to me backpatching this, please say. > > This patch makes me itch. Why is it correct for these three checks, > and only these three checks out of the couple dozen uses of isTopLevel > in standard_ProcessUtility, to instead do something else? No problem, it was a quick fix, not a deep one. -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Simon Riggs <simon@2ndquadrant.com> writes:
> On 1 September 2017 at 15:19, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> This patch makes me itch. Why is it correct for these three checks,
>> and only these three checks out of the couple dozen uses of isTopLevel
>> in standard_ProcessUtility, to instead do something else?
> No problem, it was a quick fix, not a deep one.
My thought is that what we need to do is find a way for isTopLevel
to be false if we're processing a multi-command string. It looks
like exec_simple_query is already doing the right thing in terms
of what it tells PortalRun; why is that not propagating down to
ProcessUtility?
regards, tom lane
I wrote:
> My thought is that what we need to do is find a way for isTopLevel
> to be false if we're processing a multi-command string.
Nah, that's backwards, the problem is exactly that isTopLevel is
false if we're processing a multi-command string. That allows
DefineSavepoint to think that it's inside a function, and we don't
disallow savepoints inside functions. (Or at least, xact.c doesn't
enforce any such prohibition; it's up to spi.c and the individual PLs
to decide if they could support that.)
After contemplating my navel for awhile, I think that this case proves
that the quick hack embodied in commit 4f896dac1 is inadequate. Rather
than piling another quick hack on top and hoping that the result is OK,
I think it's time to bite the bullet and represent the behavior we want
explicitly in the transaction machinery. Accordingly, PFA a patch
that invents a notion of an "implicit" transaction block.
I also added a bunch of test cases exercising the behavior. Except
for the problem of FATAL exits for savepoint commands, all these
cases work exactly like they do in unpatched code. However, now that
we have an explicit representation, it'd be easy to tweak the behavior
if we want to. For instance, I'm not entirely sure whether we want
the behavior that COMMIT and ROLLBACK in this state print warnings.
Good luck changing that before; but now it'd be a straightforward
adjustment.
I'm inclined to complete the reversion of 4f896dac1 by also undoing
its error message text change in PreventTransactionChain,
- errmsg("%s cannot be executed from a function", stmtType)));
+ errmsg("%s cannot be executed from a function or multi-command string",
+ stmtType)));
but this patch doesn't include that change.
My feeling about this is that we don't need a back-patch. Throwing
FATAL rather than ERROR for a misplaced savepoint command is a bit
unpleasant, but it doesn't break other sessions, and the upshot is
really the same: don't do that.
regards, tom lane
diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index 5e7e812..ba4b2da 100644
*** a/src/backend/access/transam/xact.c
--- b/src/backend/access/transam/xact.c
*************** typedef enum TBlockState
*** 145,150 ****
--- 145,151 ----
/* 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 */
*************** StartTransactionCommand(void)
*** 2700,2705 ****
--- 2701,2707 ----
* previous CommitTransactionCommand.)
*/
case TBLOCK_INPROGRESS:
+ case TBLOCK_IMPLICIT_INPROGRESS:
case TBLOCK_SUBINPROGRESS:
break;
*************** CommitTransactionCommand(void)
*** 2790,2795 ****
--- 2792,2798 ----
* counter and return.
*/
case TBLOCK_INPROGRESS:
+ case TBLOCK_IMPLICIT_INPROGRESS:
case TBLOCK_SUBINPROGRESS:
CommandCounterIncrement();
break;
*************** AbortCurrentTransaction(void)
*** 3014,3023 ****
break;
/*
! * if we aren't in a transaction block, we just do the basic abort
! * & cleanup transaction.
*/
case TBLOCK_STARTED:
AbortTransaction();
CleanupTransaction();
s->blockState = TBLOCK_DEFAULT;
--- 3017,3028 ----
break;
/*
! * 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;
*************** BeginTransactionBlock(void)
*** 3429,3434 ****
--- 3434,3448 ----
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.
*/
case TBLOCK_INPROGRESS:
*************** PrepareTransactionBlock(char *gid)
*** 3503,3509 ****
* ignore case where we are not in a transaction;
* EndTransactionBlock already issued a warning.
*/
! Assert(s->blockState == TBLOCK_STARTED);
/* Don't send back a PREPARE result tag... */
result = false;
}
--- 3517,3524 ----
* ignore case where we are not in a transaction;
* EndTransactionBlock already issued a warning.
*/
! Assert(s->blockState == TBLOCK_STARTED ||
! s->blockState == TBLOCK_IMPLICIT_INPROGRESS);
/* Don't send back a PREPARE result tag... */
result = false;
}
*************** EndTransactionBlock(void)
*** 3542,3547 ****
--- 3557,3574 ----
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.
*/
*************** UserAbortTransactionBlock(void)
*** 3705,3712 ****
--- 3732,3745 ----
* 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")));
*************** UserAbortTransactionBlock(void)
*** 3744,3749 ****
--- 3777,3834 ----
}
/*
+ * 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.
*/
*************** DefineSavepoint(char *name)
*** 3780,3785 ****
--- 3865,3879 ----
s->name = MemoryContextStrdup(TopTransactionContext, name);
break;
+ case TBLOCK_IMPLICIT_INPROGRESS:
+ /* Disallow SAVEPOINT in an implicit transaction */
+ 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:
*************** ReleaseSavepoint(List *options)
*** 3834,3841 ****
switch (s->blockState)
{
/*
! * We can't rollback to a savepoint if there is no savepoint
! * defined.
*/
case TBLOCK_INPROGRESS:
ereport(ERROR,
--- 3928,3934 ----
switch (s->blockState)
{
/*
! * We can't release a savepoint if there is no savepoint defined.
*/
case TBLOCK_INPROGRESS:
ereport(ERROR,
*************** ReleaseSavepoint(List *options)
*** 3843,3848 ****
--- 3936,3950 ----
errmsg("no such savepoint")));
break;
+ case TBLOCK_IMPLICIT_INPROGRESS:
+ /* Disallow RELEASE SAVEPOINT in an implicit transaction */
+ 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.
*************** RollbackToSavepoint(List *options)
*** 3957,3962 ****
--- 4059,4073 ----
errmsg("no such savepoint")));
break;
+ case TBLOCK_IMPLICIT_INPROGRESS:
+ /* Disallow ROLLBACK TO SAVEPOINT in an implicit transaction */
+ 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.
*/
*************** RollbackToSavepoint(List *options)
*** 4046,4056 ****
/*
* 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.
*/
void
BeginInternalSubTransaction(char *name)
--- 4157,4168 ----
/*
* BeginInternalSubTransaction
* This is the same as DefineSavepoint except it allows TBLOCK_STARTED,
! * 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)
*************** BeginInternalSubTransaction(char *name)
*** 4076,4081 ****
--- 4188,4194 ----
{
case TBLOCK_STARTED:
case TBLOCK_INPROGRESS:
+ case TBLOCK_IMPLICIT_INPROGRESS:
case TBLOCK_END:
case TBLOCK_PREPARE:
case TBLOCK_SUBINPROGRESS:
*************** RollbackAndReleaseCurrentSubTransaction(
*** 4180,4185 ****
--- 4293,4299 ----
case TBLOCK_DEFAULT:
case TBLOCK_STARTED:
case TBLOCK_BEGIN:
+ case TBLOCK_IMPLICIT_INPROGRESS:
case TBLOCK_PARALLEL_INPROGRESS:
case TBLOCK_SUBBEGIN:
case TBLOCK_INPROGRESS:
*************** RollbackAndReleaseCurrentSubTransaction(
*** 4211,4216 ****
--- 4325,4331 ----
s = CurrentTransactionState; /* changed by pop */
AssertState(s->blockState == TBLOCK_SUBINPROGRESS ||
s->blockState == TBLOCK_INPROGRESS ||
+ s->blockState == TBLOCK_IMPLICIT_INPROGRESS ||
s->blockState == TBLOCK_STARTED);
}
*************** AbortOutOfAnyTransaction(void)
*** 4259,4264 ****
--- 4374,4380 ----
case TBLOCK_STARTED:
case TBLOCK_BEGIN:
case TBLOCK_INPROGRESS:
+ case TBLOCK_IMPLICIT_INPROGRESS:
case TBLOCK_PARALLEL_INPROGRESS:
case TBLOCK_END:
case TBLOCK_ABORT_PENDING:
*************** TransactionBlockStatusCode(void)
*** 4369,4374 ****
--- 4485,4491 ----
case TBLOCK_BEGIN:
case TBLOCK_SUBBEGIN:
case TBLOCK_INPROGRESS:
+ case TBLOCK_IMPLICIT_INPROGRESS:
case TBLOCK_PARALLEL_INPROGRESS:
case TBLOCK_SUBINPROGRESS:
case TBLOCK_END:
*************** BlockStateAsString(TBlockState blockStat
*** 5036,5041 ****
--- 5153,5160 ----
return "BEGIN";
case TBLOCK_INPROGRESS:
return "INPROGRESS";
+ case TBLOCK_IMPLICIT_INPROGRESS:
+ return "IMPLICIT_INPROGRESS";
case TBLOCK_PARALLEL_INPROGRESS:
return "PARALLEL_INPROGRESS";
case TBLOCK_END:
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 8d3fecf..b27ca24 100644
*** a/src/backend/tcop/postgres.c
--- b/src/backend/tcop/postgres.c
*************** exec_simple_query(const char *query_stri
*** 883,892 ****
ListCell *parsetree_item;
bool save_log_statement_stats = log_statement_stats;
bool was_logged = false;
- bool isTopLevel;
char msec_str[32];
-
/*
* Report query to various monitoring facilities.
*/
--- 883,890 ----
*************** exec_simple_query(const char *query_stri
*** 947,966 ****
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.)
- */
- isTopLevel = (list_length(parsetree_list) == 1);
-
- /*
* Run through the raw parsetree(s) and process each one.
*/
foreach(parsetree_item, parsetree_list)
{
RawStmt *parsetree = lfirst_node(RawStmt, parsetree_item);
bool snapshot_set = false;
const char *commandTag;
char completionTag[COMPLETION_TAG_BUFSIZE];
--- 945,956 ----
MemoryContextSwitchTo(oldcontext);
/*
* Run through the raw parsetree(s) and process each one.
*/
foreach(parsetree_item, parsetree_list)
{
RawStmt *parsetree = lfirst_node(RawStmt, parsetree_item);
+ bool is_last_tree = (lnext(parsetree_item) == NULL);
bool snapshot_set = false;
const char *commandTag;
char completionTag[COMPLETION_TAG_BUFSIZE];
*************** exec_simple_query(const char *query_stri
*** 1001,1006 ****
--- 991,1011 ----
/* Make sure we are in a transaction command */
start_xact_command();
+ /*
+ * 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 represent this behavior properly in the transaction
+ * machinery, we use an "implicit" transaction block. If this isn't
+ * the last statement, and we're not already in a transaction block,
+ * start an implicit transaction block to force this statement to be
+ * grouped together with the following ones. (Thus, putting a COMMIT
+ * midway through the list doesn't allow the following statements to
+ * execute normally; they'll get grouped together anyway.)
+ */
+ if (!is_last_tree)
+ BeginImplicitTransactionBlock();
+
/* If we got a cancel signal in parsing or prior command, quit */
CHECK_FOR_INTERRUPTS();
*************** exec_simple_query(const char *query_stri
*** 1098,1104 ****
*/
(void) PortalRun(portal,
FETCH_ALL,
! isTopLevel,
true,
receiver,
receiver,
--- 1103,1109 ----
*/
(void) PortalRun(portal,
FETCH_ALL,
! true,
true,
receiver,
receiver,
*************** exec_simple_query(const char *query_stri
*** 1108,1132 ****
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 this is the last parsetree of the query string, close down
! * transaction statement before reporting command-complete. This
! * 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.
*/
finish_xact_command();
}
--- 1113,1139 ----
PortalDrop(portal, false);
! if (is_last_tree)
{
/*
! * If this is the last parsetree of the query string, close down
! * transaction statement, and any implicit transaction block,
! * before reporting command-complete. This 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.
*/
+ 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();
}
*************** exec_simple_query(const char *query_stri
*** 1149,1155 ****
} /* end loop over parsetrees */
/*
! * Close down transaction statement, if one is open.
*/
finish_xact_command();
--- 1156,1164 ----
} /* end loop over parsetrees */
/*
! * Close down transaction statement, if one is open. (This will only do
! * something if the parsetree list was empty; otherwise the is_last_tree
! * case above covered it.)
*/
finish_xact_command();
diff --git a/src/include/access/xact.h b/src/include/access/xact.h
index ad5aad9..f2c10f9 100644
*** a/src/include/access/xact.h
--- b/src/include/access/xact.h
*************** extern void BeginTransactionBlock(void);
*** 352,357 ****
--- 352,359 ----
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);
diff --git a/src/test/regress/expected/transactions.out b/src/test/regress/expected/transactions.out
index d9b702d..bc0b15c 100644
*** a/src/test/regress/expected/transactions.out
--- b/src/test/regress/expected/transactions.out
*************** ERROR: portal "ctt" cannot be run
*** 659,664 ****
--- 659,742 ----
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
+ -- we disallow savepoint-related commands in implicit-transaction state
+ SELECT 1\; SAVEPOINT sp;
+ 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;
diff --git a/src/test/regress/sql/transactions.sql b/src/test/regress/sql/transactions.sql
index bf9cb05..3ebde73 100644
*** a/src/test/regress/sql/transactions.sql
--- b/src/test/regress/sql/transactions.sql
*************** DROP FUNCTION create_temp_tab();
*** 419,424 ****
--- 419,476 ----
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;
+
+ -- we disallow savepoint-related commands in implicit-transaction state
+ SELECT 1\; 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.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
I wrote:
> ... PFA a patch
> that invents a notion of an "implicit" transaction block.
On further consideration, I think the control logic I added in
exec_simple_query() is a shade bogus. I set it up to only force
an implicit transaction block when there are at least two statements
remaining to execute. However, that has the result of allowing, eg,
begin\; select 1\; commit\; vacuum;
Now in principle it's perfectly OK to allow that, since the vacuum
is alone in its transaction. But it feels more like an implementation
artifact than a good design. The existing code doesn't allow it,
and we might have a hard time duplicating this behavior if we ever
significantly rewrote the transaction infrastructure. Plus I'd hate
to have to explain it to users. I think we'd be better off enforcing
transaction block restrictions on every statement in a multi-command
string, regardless of the location of any COMMIT/ROLLBACK within the
string.
Hence, attached a v2 that does it like that. I also fully reverted
4f896dac1 by undoing its changes to PreventTransactionChain; other
than that, the changes in xact.c are the same as before.
regards, tom lane
diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index 5e7e812..8b33676 100644
*** a/src/backend/access/transam/xact.c
--- b/src/backend/access/transam/xact.c
*************** typedef enum TBlockState
*** 145,150 ****
--- 145,151 ----
/* 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 */
*************** StartTransactionCommand(void)
*** 2700,2705 ****
--- 2701,2707 ----
* previous CommitTransactionCommand.)
*/
case TBLOCK_INPROGRESS:
+ case TBLOCK_IMPLICIT_INPROGRESS:
case TBLOCK_SUBINPROGRESS:
break;
*************** CommitTransactionCommand(void)
*** 2790,2795 ****
--- 2792,2798 ----
* counter and return.
*/
case TBLOCK_INPROGRESS:
+ case TBLOCK_IMPLICIT_INPROGRESS:
case TBLOCK_SUBINPROGRESS:
CommandCounterIncrement();
break;
*************** AbortCurrentTransaction(void)
*** 3014,3023 ****
break;
/*
! * if we aren't in a transaction block, we just do the basic abort
! * & cleanup transaction.
*/
case TBLOCK_STARTED:
AbortTransaction();
CleanupTransaction();
s->blockState = TBLOCK_DEFAULT;
--- 3017,3028 ----
break;
/*
! * 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;
*************** AbortCurrentTransaction(void)
*** 3148,3156 ****
* 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.)
* stmtType: statement type name, for error messages.
*/
void
--- 3153,3160 ----
* completes). Subtransactions are verboten too.
*
* isTopLevel: passed down from ProcessUtility to determine whether we are
! * 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
*************** PreventTransactionChain(bool isTopLevel,
*** 3183,3190 ****
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)));
/* If we got past IsTransactionBlock test, should be in default state */
if (CurrentTransactionState->blockState != TBLOCK_DEFAULT &&
--- 3187,3193 ----
ereport(ERROR,
(errcode(ERRCODE_ACTIVE_SQL_TRANSACTION),
/* translator: %s represents an SQL statement name */
! 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 &&
*************** BeginTransactionBlock(void)
*** 3429,3434 ****
--- 3432,3446 ----
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.
*/
case TBLOCK_INPROGRESS:
*************** PrepareTransactionBlock(char *gid)
*** 3503,3509 ****
* ignore case where we are not in a transaction;
* EndTransactionBlock already issued a warning.
*/
! Assert(s->blockState == TBLOCK_STARTED);
/* Don't send back a PREPARE result tag... */
result = false;
}
--- 3515,3522 ----
* ignore case where we are not in a transaction;
* EndTransactionBlock already issued a warning.
*/
! Assert(s->blockState == TBLOCK_STARTED ||
! s->blockState == TBLOCK_IMPLICIT_INPROGRESS);
/* Don't send back a PREPARE result tag... */
result = false;
}
*************** EndTransactionBlock(void)
*** 3542,3547 ****
--- 3555,3572 ----
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.
*/
*************** UserAbortTransactionBlock(void)
*** 3705,3712 ****
--- 3730,3743 ----
* 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")));
*************** UserAbortTransactionBlock(void)
*** 3744,3749 ****
--- 3775,3832 ----
}
/*
+ * 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.
*/
*************** DefineSavepoint(char *name)
*** 3780,3785 ****
--- 3863,3877 ----
s->name = MemoryContextStrdup(TopTransactionContext, name);
break;
+ case TBLOCK_IMPLICIT_INPROGRESS:
+ /* Disallow SAVEPOINT in an implicit transaction */
+ 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:
*************** ReleaseSavepoint(List *options)
*** 3834,3841 ****
switch (s->blockState)
{
/*
! * We can't rollback to a savepoint if there is no savepoint
! * defined.
*/
case TBLOCK_INPROGRESS:
ereport(ERROR,
--- 3926,3932 ----
switch (s->blockState)
{
/*
! * We can't release a savepoint if there is no savepoint defined.
*/
case TBLOCK_INPROGRESS:
ereport(ERROR,
*************** ReleaseSavepoint(List *options)
*** 3843,3848 ****
--- 3934,3948 ----
errmsg("no such savepoint")));
break;
+ case TBLOCK_IMPLICIT_INPROGRESS:
+ /* Disallow RELEASE SAVEPOINT in an implicit transaction */
+ 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.
*************** RollbackToSavepoint(List *options)
*** 3957,3962 ****
--- 4057,4071 ----
errmsg("no such savepoint")));
break;
+ case TBLOCK_IMPLICIT_INPROGRESS:
+ /* Disallow ROLLBACK TO SAVEPOINT in an implicit transaction */
+ 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.
*/
*************** RollbackToSavepoint(List *options)
*** 4046,4056 ****
/*
* 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.
*/
void
BeginInternalSubTransaction(char *name)
--- 4155,4166 ----
/*
* BeginInternalSubTransaction
* This is the same as DefineSavepoint except it allows TBLOCK_STARTED,
! * 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)
*************** BeginInternalSubTransaction(char *name)
*** 4076,4081 ****
--- 4186,4192 ----
{
case TBLOCK_STARTED:
case TBLOCK_INPROGRESS:
+ case TBLOCK_IMPLICIT_INPROGRESS:
case TBLOCK_END:
case TBLOCK_PREPARE:
case TBLOCK_SUBINPROGRESS:
*************** RollbackAndReleaseCurrentSubTransaction(
*** 4180,4185 ****
--- 4291,4297 ----
case TBLOCK_DEFAULT:
case TBLOCK_STARTED:
case TBLOCK_BEGIN:
+ case TBLOCK_IMPLICIT_INPROGRESS:
case TBLOCK_PARALLEL_INPROGRESS:
case TBLOCK_SUBBEGIN:
case TBLOCK_INPROGRESS:
*************** RollbackAndReleaseCurrentSubTransaction(
*** 4211,4216 ****
--- 4323,4329 ----
s = CurrentTransactionState; /* changed by pop */
AssertState(s->blockState == TBLOCK_SUBINPROGRESS ||
s->blockState == TBLOCK_INPROGRESS ||
+ s->blockState == TBLOCK_IMPLICIT_INPROGRESS ||
s->blockState == TBLOCK_STARTED);
}
*************** AbortOutOfAnyTransaction(void)
*** 4259,4264 ****
--- 4372,4378 ----
case TBLOCK_STARTED:
case TBLOCK_BEGIN:
case TBLOCK_INPROGRESS:
+ case TBLOCK_IMPLICIT_INPROGRESS:
case TBLOCK_PARALLEL_INPROGRESS:
case TBLOCK_END:
case TBLOCK_ABORT_PENDING:
*************** TransactionBlockStatusCode(void)
*** 4369,4374 ****
--- 4483,4489 ----
case TBLOCK_BEGIN:
case TBLOCK_SUBBEGIN:
case TBLOCK_INPROGRESS:
+ case TBLOCK_IMPLICIT_INPROGRESS:
case TBLOCK_PARALLEL_INPROGRESS:
case TBLOCK_SUBINPROGRESS:
case TBLOCK_END:
*************** BlockStateAsString(TBlockState blockStat
*** 5036,5041 ****
--- 5151,5158 ----
return "BEGIN";
case TBLOCK_INPROGRESS:
return "INPROGRESS";
+ case TBLOCK_IMPLICIT_INPROGRESS:
+ return "IMPLICIT_INPROGRESS";
case TBLOCK_PARALLEL_INPROGRESS:
return "PARALLEL_INPROGRESS";
case TBLOCK_END:
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 8d3fecf..c10d891 100644
*** a/src/backend/tcop/postgres.c
--- b/src/backend/tcop/postgres.c
*************** exec_simple_query(const char *query_stri
*** 883,892 ****
ListCell *parsetree_item;
bool save_log_statement_stats = log_statement_stats;
bool was_logged = false;
! bool isTopLevel;
char msec_str[32];
-
/*
* Report query to various monitoring facilities.
*/
--- 883,891 ----
ListCell *parsetree_item;
bool save_log_statement_stats = log_statement_stats;
bool was_logged = false;
! bool use_implicit_block;
char msec_str[32];
/*
* Report query to various monitoring facilities.
*/
*************** exec_simple_query(const char *query_stri
*** 947,959 ****
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.)
*/
! isTopLevel = (list_length(parsetree_list) == 1);
/*
* Run through the raw parsetree(s) and process each one.
--- 946,959 ----
MemoryContextSwitchTo(oldcontext);
/*
! * 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.
*/
! use_implicit_block = (list_length(parsetree_list) > 1);
/*
* Run through the raw parsetree(s) and process each one.
*************** exec_simple_query(const char *query_stri
*** 1001,1006 ****
--- 1001,1016 ----
/* 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();
*************** exec_simple_query(const char *query_stri
*** 1098,1104 ****
*/
(void) PortalRun(portal,
FETCH_ALL,
! isTopLevel,
true,
receiver,
receiver,
--- 1108,1114 ----
*/
(void) PortalRun(portal,
FETCH_ALL,
! true, /* always top level */
true,
receiver,
receiver,
*************** exec_simple_query(const char *query_stri
*** 1108,1122 ****
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 this is the last parsetree of the query string, close down
--- 1118,1124 ----
PortalDrop(portal, false);
! if (lnext(parsetree_item) == NULL)
{
/*
* If this is the last parsetree of the query string, close down
*************** exec_simple_query(const char *query_stri
*** 1124,1132 ****
* 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.
*/
finish_xact_command();
}
--- 1126,1143 ----
* 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. 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();
}
*************** exec_simple_query(const char *query_stri
*** 1149,1155 ****
} /* end loop over parsetrees */
/*
! * Close down transaction statement, if one is open.
*/
finish_xact_command();
--- 1160,1168 ----
} /* end loop over parsetrees */
/*
! * 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();
diff --git a/src/include/access/xact.h b/src/include/access/xact.h
index ad5aad9..f2c10f9 100644
*** a/src/include/access/xact.h
--- b/src/include/access/xact.h
*************** extern void BeginTransactionBlock(void);
*** 352,357 ****
--- 352,359 ----
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);
diff --git a/src/test/regress/expected/transactions.out b/src/test/regress/expected/transactions.out
index d9b702d..a7fdcf4 100644
*** a/src/test/regress/expected/transactions.out
--- b/src/test/regress/expected/transactions.out
*************** ERROR: portal "ctt" cannot be run
*** 659,664 ****
--- 659,748 ----
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;
diff --git a/src/test/regress/sql/transactions.sql b/src/test/regress/sql/transactions.sql
index bf9cb05..82661ab 100644
*** a/src/test/regress/sql/transactions.sql
--- b/src/test/regress/sql/transactions.sql
*************** DROP FUNCTION create_temp_tab();
*** 419,424 ****
--- 419,478 ----
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.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [bug fix] Savepoint-related statements terminates connection
From
Michael Paquier
Date:
On Mon, Sep 4, 2017 at 7:20 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > I wrote: > On further consideration, I think the control logic I added in > exec_simple_query() is a shade bogus. I set it up to only force > an implicit transaction block when there are at least two statements > remaining to execute. However, that has the result of allowing, eg, > > begin\; select 1\; commit\; vacuum; > > Now in principle it's perfectly OK to allow that, since the vacuum > is alone in its transaction. But it feels more like an implementation > artifact than a good design. The existing code doesn't allow it, > and we might have a hard time duplicating this behavior if we ever > significantly rewrote the transaction infrastructure. Plus I'd hate > to have to explain it to users. I think we'd be better off enforcing > transaction block restrictions on every statement in a multi-command > string, regardless of the location of any COMMIT/ROLLBACK within the > string. > > Hence, attached a v2 that does it like that. I also fully reverted > 4f896dac1 by undoing its changes to PreventTransactionChain; other > than that, the changes in xact.c are the same as before. Hmm. While this patch looks to me in a better shape than what Simon's is proposing, thinking about CAH2-V61vxNEnTfj2V-zd+mA-g6kQMJgd5SvXoU3JBvdzQH0Yfw@mail.gmail.com which involved a migration Oracle->Postgres, I have been wondering if it is possible to still allow savepoints in those cases to ease the pain and surprise of some users. And while looking around, it seems to me that it is possible. Please find the attached to show my idea, based on Tom's v2. The use of a new transaction state like IMPLICIT_INPROGRESS is something that I got in mind upthread, but I have not shaped that into a fully-blown patch. All the following sequences are working as I would think they should (a couple of inserts done within each savepoint allowed me to check that the transactions happened correctly, though the set of regressions presented in v2 looks enough): BEGIN; SELECT 1; SAVEPOINT sp; RELEASE sp; SAVEPOINT sp; ROLLBACK TO SAVEPOINT sp; COMMIT; BEGIN; SELECT 1; SAVEPOINT sp; RELEASE sp; SAVEPOINT sp; ROLLBACK TO SAVEPOINT sp; ROLLBACK; SELECT 1; SAVEPOINT sp; RELEASE sp; SAVEPOINT sp; ROLLBACK TO SAVEPOINT sp; So sequences of multiple commands are working with the patch attached even if a BEGIN is not explicitly added. On HEAD or with v2, if BEGIN is not specified, savepoint commands cause a failure. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
Michael Paquier <michael.paquier@gmail.com> writes:
> Hmm. While this patch looks to me in a better shape than what Simon's
> is proposing, thinking about
> CAH2-V61vxNEnTfj2V-zd+mA-g6kQMJgd5SvXoU3JBvdzQH0Yfw@mail.gmail.com
> which involved a migration Oracle->Postgres, I have been wondering if
> it is possible to still allow savepoints in those cases to ease the
> pain and surprise of some users.
I don't want to go there, and was thinking we should expand the new
comment in DefineSavepoint to explain why not. It's certainly not that
much additional work to allow a savepoint so far as xact.c is concerned,
as your patch shows. The problem is that intra-string savepoints seem
inconsistent with exec_simple_query's behavior of abandoning the whole
query string upon error. If you do
insert ...\; savepoint\; insert ...\; release savepoint\; insert ...;
wouldn't you sort of expect that the savepoint commands mean to keep going
if the second insert fails? If they don't mean that, what do they mean?
Also, the main thing that we need xact.c's involvement for in the first
place is the fact that implicit transaction blocks, unlike regular ones,
auto-cancel on an error, leaving you outside a block not inside a failed
one. So I don't exactly see how savepoints would fit into that.
Now I do not think we can change exec_simple_query's behavior without big
compatibility problems --- to the extent that there's a justifiable
use-case for multi-query strings at all, a big part of it is the implied
"do B only if A succeeds" semantics. But if that's what happens, then
having savepoint commands in the string is just a can of worms from both
definitional and practical points of view. If an error happens, did it
happen before or after the savepoint, and what state is the session left
in? You can't easily tell because of the lack of reporting about
savepoint state. Right now, the only real issue after a failure is "are
we in a transaction block or not", which the server does return enough
info to distinguish.
Now admittedly, the same set of issues pops up if one uses an
explicit transaction block in a multi-query string:
begin\; insert ...\; savepoint\; insert ...\; release savepoint\; insert ...\; commit;
If one of the inserts fails, you don't really know which one unless you
were counting command-complete replies (which PQexec doesn't let you do).
But that behavior was there already, we aren't proposing to make it worse.
(I think this approach is also the correct workaround to give those
Oracle-conversion folk: their real problem is failure to convert from
Oracle's implicit-BEGIN behavior to our explicit-BEGIN.)
In short, -1 for relaxing the prohibition on SAVEPOINT.
regards, tom lane
Re: [HACKERS] [bug fix] Savepoint-related statements terminates connection
From
Michael Paquier
Date:
On Mon, Sep 4, 2017 at 11:15 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > I don't want to go there, and was thinking we should expand the new > comment in DefineSavepoint to explain why not. Okay. > It's certainly not that > much additional work to allow a savepoint so far as xact.c is concerned, > as your patch shows. The problem is that intra-string savepoints seem > inconsistent with exec_simple_query's behavior of abandoning the whole > query string upon error. If you do > > insert ...\; savepoint\; insert ...\; release savepoint\; insert ...; > > wouldn't you sort of expect that the savepoint commands mean to keep going > if the second insert fails? If they don't mean that, what do they mean? Hmm. I spent more time looking at my patch and I see what you are pointing out here. Using something like that with a second insert failing I would expect the first insert to be visible, but that's not the case: savepoint rs; insert into exists values (1); savepoint rs2; insert into not_exists values (1); rollback to savepoint rs2; commit;' So this approach makes things inconsistent. > Now admittedly, the same set of issues pops up if one uses an > explicit transaction block in a multi-query string: > > begin\; insert ...\; savepoint\; insert ...\; release savepoint\; insert ...\; commit; > > If one of the inserts fails, you don't really know which one unless you > were counting command-complete replies (which PQexec doesn't let you do). > But that behavior was there already, we aren't proposing to make it worse. > (I think this approach is also the correct workaround to give those > Oracle-conversion folk: their real problem is failure to convert from > Oracle's implicit-BEGIN behavior to our explicit-BEGIN.) Sure there is this workaround. -- Michael
Michael Paquier <michael.paquier@gmail.com> writes:
> On Mon, Sep 4, 2017 at 11:15 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I don't want to go there, and was thinking we should expand the new
>> comment in DefineSavepoint to explain why not.
> Okay.
Does anyone want to do further review on this patch? If so, I'll
set the CF entry back to "Needs Review".
regards, tom lane
On Mon, Sep 4, 2017 at 4:15 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Also, the main thing that we need xact.c's involvement for in the first > place is the fact that implicit transaction blocks, unlike regular ones, > auto-cancel on an error, leaving you outside a block not inside a failed > one. So I don't exactly see how savepoints would fit into that. I think this hits the nail on the head and should have a place in the official docs as I now realize I didn't grasp this distinction before I read this. My mental model was always "sending a bunch of semicolon separated queries without BEGIN/COMMIT/ROLLBACK; in one PQexec is like sending them one by one preceeded by a BEGIN; and followed by a COMMIT; except you only get the response from the last one". Also, explain what happens when there are BEGIN/ROLLBACK/COMMIT inside that multiquery string, that's still not completely clear to me and I don't want to reverse engineer it from your patch. > Now admittedly, the same set of issues pops up if one uses an > explicit transaction block in a multi-query string: > > begin\; insert ...\; savepoint\; insert ...\; release savepoint\; insert ...\; commit; According to my mental model described above, this would be exactly the same as without the begin; and commit; which is not the case so I think the distinction is worth explaining. I think the lack of a more detailed explanation about the stuff above confuses *a lot* of people, especially newcomers, and the confusion is only increased by what client drivers do on top (like issuing implicit BEGIN if configured in various modes specified by language-specific-DB-independent specs like Python's DBAPI or Java's JDBC) and one's background from other DBs that do it differently. Speaking of the above, psql also doesn't explicitly document how it groups lines of the file it's executing into PQexec calls. See below for a personal example of the confusions all this generates. I also encountered this FATAL a month ago in the context of "we have some (migration schema) queries in some files and want to orchestrate running them for testing". Initially we started with calling psql but then we needed some client side logic for some other stuff and switched to Python and Psycopg2. We did "read the whole file in a Python string" and then call Psycopg2's execute() on that string. Note that Psycopg2 only uses PQexec to issue queries. We had some SAVEPOINT statements in the file which lead to the backend stopping and the next Psycopg2 execute() on that connection saying Connection closed. It was already confusing why Psycopg2 behaves differently than psql (because we were issuing the whole file in one PQexec vs. psql splitting on ; and issuing multiple PQexecs and SAVEPOINTs working there) and the backend stopping only added to that confusion. Add on top of that "Should we put BEGIN; and COMMIT; in the file itself? Or is a single Psycopg2 execute() enough to have this schema migration be applied transactionally? Is there a difference between the two?". I searched the docs for existing explanations of multiquery strings and found these references but all of them are a bit hand wavy: - psql's reference explaining -c - libpq's PQexec explanation - the message flow document in the FE/BE protocol description
Catalin Iacob <iacobcatalin@gmail.com> writes:
> On Mon, Sep 4, 2017 at 4:15 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Also, the main thing that we need xact.c's involvement for in the first
>> place is the fact that implicit transaction blocks, unlike regular ones,
>> auto-cancel on an error, leaving you outside a block not inside a failed
>> one. So I don't exactly see how savepoints would fit into that.
> I think this hits the nail on the head and should have a place in the
> official docs as I now realize I didn't grasp this distinction before
> I read this.
Yeah, it seems like we have now made this behavior official enough that
it's time to document it better. My thought is to create a new subsection
in the FE/BE Protocol chapter that explains how multi-statement Query
messages are handled, and then to link to that from appropriate places
elsewhere. If anyone thinks the reference section would be better put
somewhere else than Protocol, please say where.
regards, tom lane
I wrote: > Yeah, it seems like we have now made this behavior official enough that > it's time to document it better. My thought is to create a new subsection > in the FE/BE Protocol chapter that explains how multi-statement Query > messages are handled, and then to link to that from appropriate places > elsewhere. If anyone thinks the reference section would be better put > somewhere else than Protocol, please say where. I've pushed up an attempt at this: https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=b976499480bdbab6d69a11e47991febe53865adc Feel free to suggest improvements. regards, tom lane
On 5 September 2017 at 10:22, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Michael Paquier <michael.paquier@gmail.com> writes: >> On Mon, Sep 4, 2017 at 11:15 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >>> I don't want to go there, and was thinking we should expand the new >>> comment in DefineSavepoint to explain why not. > >> Okay. > > Does anyone want to do further review on this patch? If so, I'll > set the CF entry back to "Needs Review". OK, I'll review Michael's patch (and confirm my patch is dead) -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 7 September 2017 at 11:07, Tom Lane <tgl@sss.pgh.pa.us> wrote: > I wrote: >> Yeah, it seems like we have now made this behavior official enough that >> it's time to document it better. My thought is to create a new subsection >> in the FE/BE Protocol chapter that explains how multi-statement Query >> messages are handled, and then to link to that from appropriate places >> elsewhere. If anyone thinks the reference section would be better put >> somewhere else than Protocol, please say where. > > I've pushed up an attempt at this: > > https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=b976499480bdbab6d69a11e47991febe53865adc > > Feel free to suggest improvements. Not so much an improvement as a follow-on thought: All of this applies to simple queries. At present we restrict using multi-statement requests in extended protocol, saying that we don't allow it because of a protocol restriction. The precise restriction is that we can't return more than one reply. The restriction is implemented via this test if (list_length(parsetree_list) > 1) ereport(ERROR, (errcode(ERRCODE_SYNTAX_ERROR), errmsg("cannot insert multiple commands into a prepared statement"))); at line 1277 of exec_parse_message() which is actually more restrictive than it needs to be. I would like to relax the restriction to allow this specific use case... SET work_mem = X; SET max_parallel_workers = 4;SELECT ... so we still have only one command (the last select), yet we have multiple GUC settings beforehand. Any reason to disallow that? -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Simon Riggs <simon@2ndquadrant.com> writes:
> On 5 September 2017 at 10:22, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Does anyone want to do further review on this patch? If so, I'll
>> set the CF entry back to "Needs Review".
> OK, I'll review Michael's patch (and confirm my patch is dead)
Not hearing anything, I already pushed my patch an hour or three ago.
regards, tom lane
Simon Riggs <simon@2ndquadrant.com> writes:
> I would like to relax the restriction to allow this specific use case...
> SET work_mem = X; SET max_parallel_workers = 4; SELECT ...
> so we still have only one command (the last select), yet we have
> multiple GUC settings beforehand.
On what basis do you claim that's only one command? It would return
multiple CommandCompletes, for starters, so that it breaks the protocol
just as effectively as any other loosening.
Moreover, I imagine the semantics you really want is that the SETs only
apply for the duration of the command. This wouldn't provide that
result either.
Haas' idea of some kind of syntactic extension, like "LET guc1 = x,
guc2 = y FOR statement" seems more feasible to me. I'm not necessarily
wedded to that particular syntax, but I think it has to look like
a single-statement construct of some kind.
regards, tom lane
On 7 September 2017 at 11:24, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Simon Riggs <simon@2ndquadrant.com> writes: >> On 5 September 2017 at 10:22, Tom Lane <tgl@sss.pgh.pa.us> wrote: >>> Does anyone want to do further review on this patch? If so, I'll >>> set the CF entry back to "Needs Review". > >> OK, I'll review Michael's patch (and confirm my patch is dead) > > Not hearing anything, I already pushed my patch an hour or three ago. Yes, I saw. Are you saying that doc commit is all we need? ISTM we still had an actual bug. -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
On 7 September 2017 at 11:31, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Simon Riggs <simon@2ndquadrant.com> writes: >> I would like to relax the restriction to allow this specific use case... >> SET work_mem = X; SET max_parallel_workers = 4; SELECT ... >> so we still have only one command (the last select), yet we have >> multiple GUC settings beforehand. > > On what basis do you claim that's only one command? It would return > multiple CommandCompletes, for starters, so that it breaks the protocol > just as effectively as any other loosening. > > Moreover, I imagine the semantics you really want is that the SETs only > apply for the duration of the command. This wouldn't provide that > result either. > Haas' idea of some kind of syntactic extension, like "LET guc1 = x, > guc2 = y FOR statement" seems more feasible to me. I'm not necessarily > wedded to that particular syntax, but I think it has to look like > a single-statement construct of some kind. Always happy to use a good idea... (any better way to re-locate that discussion?) 1. Allow SET to set multiple parameters... SET guc1 = x, guc2 = y This looks fairly straightforward 2. Allow SET to work only for a single command... SET guc1 = x, guc2 = y FOR query Don't see anything too bad about that... Requires a new GUC mode for "statement local" rather than "transaction local" -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Simon Riggs <simon@2ndquadrant.com> writes:
> On 7 September 2017 at 11:24, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Not hearing anything, I already pushed my patch an hour or three ago.
> Yes, I saw. Are you saying that doc commit is all we need? ISTM we
> still had an actual bug.
The originally reported bug is fixed. Not making any claims about
other bugs ...
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Simon Riggs <simon@2ndquadrant.com> writes: > On 7 September 2017 at 11:31, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> Haas' idea of some kind of syntactic extension, like "LET guc1 = x, >> guc2 = y FOR statement" seems more feasible to me. I'm not necessarily >> wedded to that particular syntax, but I think it has to look like >> a single-statement construct of some kind. > Always happy to use a good idea... (any better way to re-locate that > discussion?) https://www.postgresql.org/message-id/CA+TgmobgD_UZRs44cOutY1odNbR0C_HJSxvx_dMREvz-CwuiaQ@mail.gmail.com > Requires a new GUC mode for "statement local" rather than "transaction local" Yeah, something along that line. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
On Thu, Sep 7, 2017 at 8:07 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I've pushed up an attempt at this:
>
> https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=b976499480bdbab6d69a11e47991febe53865adc
>
> Feel free to suggest improvements.
Thank you, this helps a lot. Especially since some of the behavior is a bit surprising, for example stopping on error leading to ROLLBACK not being done and the retroactive upgrade of preceding commands in an implicit block to a transaction block when a BEGIN appears.
When reading this I also realized that the backend does send responses for every individual query in a multi-query request, it's only libpq's PQexec that throws away the intermediate results and only provides access to the last one. I always thought the backend did that. The docs hinted that it's the frontend ("psql only prints the last one", "PGresult describes the result of the last command") but to assure myself I looked with tcpdump.
It's a pity that the underlying protocol has 2 ways to do batching of queries but the official library hides both. I guess I should go review the "Batch/pipelining support for libpq" patch rather than complaining.
> I've pushed up an attempt at this:
>
> https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=b976499480bdbab6d69a11e47991febe53865adc
>
> Feel free to suggest improvements.
Thank you, this helps a lot. Especially since some of the behavior is a bit surprising, for example stopping on error leading to ROLLBACK not being done and the retroactive upgrade of preceding commands in an implicit block to a transaction block when a BEGIN appears.
When reading this I also realized that the backend does send responses for every individual query in a multi-query request, it's only libpq's PQexec that throws away the intermediate results and only provides access to the last one. I always thought the backend did that. The docs hinted that it's the frontend ("psql only prints the last one", "PGresult describes the result of the last command") but to assure myself I looked with tcpdump.
It's a pity that the underlying protocol has 2 ways to do batching of queries but the official library hides both. I guess I should go review the "Batch/pipelining support for libpq" patch rather than complaining.
Catalin Iacob <iacobcatalin@gmail.com> writes: > When reading this I also realized that the backend does send responses for > every individual query in a multi-query request, it's only libpq's PQexec > that throws away the intermediate results and only provides access to the > last one. If you want to see them all, you can use PQsendQuery/PQgetResult. https://www.postgresql.org/docs/current/static/libpq-async.html There's a case to be made that we should change psql to use these and print all the results not just the last one. I've not looked to see how much work that would be; but now that we're actually documenting how to script multi-command queries, it might be a good idea to fix it before too many people have scripts that rely on the current behavior. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [bug fix] Savepoint-related statements terminatesconnection
From
"Tsunakawa, Takayuki"
Date:
From: pgsql-hackers-owner@postgresql.org > [mailto:pgsql-hackers-owner@postgresql.org] On Behalf Of Tom Lane > The originally reported bug is fixed. Not making any claims about other > bugs ... I'm sorry I couldn't reply to you. I've recently been in a situation where I can't use my time for development. I thinkI'll be able to rejoin the community activity soon. I confirmed your patch fixed the problem. And the code looks perfect. Thank you very much. Regards Takayuki Tsunakawa -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers