On 11/01/2024 19:37, Robert Haas wrote:
On Wed, Jan 10, 2024 at 4:25 PM Heikki Linnakangas <hlinn...@iki.fi> wrote:
The problem with CommitTransactionCommand (or rather
AbortCurrentTransaction() which has the same problem)
and ShowTransactionStateRec is that they get called in a state where
aborting can lead to a panic. If you add a "check_stack_depth()" to them
and try to reproducer scripts that Egor posted, you still get a panic.
Hmm, that's unfortunate. I'm not sure what to do about that. But I'd
still suggest looking for a goto-free approach.
Here's one goto-free attempt. It adds a local loop to where the
recursion was, so that if you have a chain of subtransactions that need
to be aborted in CommitTransactionCommand, they are aborted iteratively.
The TBLOCK_SUBCOMMIT case already had such a loop.
I added a couple of comments in the patch marked with "REVIEWER NOTE",
to explain why I changed some things. They are to be removed before
committing.
I'm not sure if this is better than a goto. In fact, even if we commit
this, I think I'd still prefer to replace the remaining recursive calls
with a goto. Recursion feels a weird to me, when we're unwinding the
states from the stack as we go.
Of course we could use a "for (;;) { ... continue }" construct around
the whole function, instead of a goto, but I don't think that's better
than a goto in this case.
--
Heikki Linnakangas
Neon (https://neon.tech)
From 44568e6feef79d604bbe168c0839ab2e03c99f8a Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakan...@iki.fi>
Date: Fri, 12 Jan 2024 17:07:25 +0200
Subject: [PATCH v2 1/1] Turn tail recursion into iteration in
AbortCurrentTransaction()
Usually the compiler will optimize away the tail recursion anyway, but
if it doesn't, you can drive the function into stack overflow. For
example:
(n=1000000; printf "BEGIN;"; for ((i=1;i<=$n;i++)); do printf "SAVEPOINT s$i;"; done; printf "ERROR; COMMIT;") | psql >/dev/null
The usual fix would be to add a check_stack_depth() to the function,
but that's not good in AbortCurrentTransaction(), as you are already
trying to clean up after a failure. ereporting there leads to a panic.
Fix CurrentTransactionCommand() in a similar fashion, although
ereporting would be less bad there.
Report by Egor Chindyaskin and Alexander Lakhin.
Discussion: https://www.postgresql.org/message-id/1672760457.940462079%40f306.i.mail.ru
---
src/backend/access/transam/xact.c | 141 ++++++++++++++++--------------
1 file changed, 75 insertions(+), 66 deletions(-)
diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index 464858117e0..329a139c991 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -3194,47 +3194,62 @@ CommitTransactionCommand(void)
CommitSubTransaction();
s = CurrentTransactionState; /* changed by pop */
} while (s->blockState == TBLOCK_SUBCOMMIT);
+ /*
+ * REVIEWER NOTE: We used to have duplicated code here, to handle
+ * the TBLOCK_END and TBLOCK_PREPARE cases exactly the same way
+ * that we do above. I replaced that with a recursive call to
+ * CommitTransactionCommand(), to make this consistent with the
+ * TBLOCK_SUBABORT_END and TBLOCK_SUBABORT_PENDING handling. This
+ * can only recurse once, so there's no risk of stack overflow.
+ */
/* If we had a COMMIT command, finish off the main xact too */
- if (s->blockState == TBLOCK_END)
- {
- Assert(s->parent == NULL);
- CommitTransaction();
- s->blockState = TBLOCK_DEFAULT;
- if (s->chain)
- {
- StartTransaction();
- s->blockState = TBLOCK_INPROGRESS;
- s->chain = false;
- RestoreTransactionCharacteristics(&savetc);
- }
- }
- else if (s->blockState == TBLOCK_PREPARE)
- {
- Assert(s->parent == NULL);
- PrepareTransaction();
- s->blockState = TBLOCK_DEFAULT;
- }
- else
+ if (s->blockState != TBLOCK_END && s->blockState != TBLOCK_PREPARE)
elog(ERROR, "CommitTransactionCommand: unexpected state %s",
BlockStateAsString(s->blockState));
+ CommitTransactionCommand();
break;
/*
- * The current already-failed subtransaction is ending due to a
- * ROLLBACK or ROLLBACK TO command, so pop it and recursively
- * examine the parent (which could be in any of several states).
+ * The current subtransaction is ending due to a ROLLBACK or
+ * ROLLBACK TO command. Pop it, as well as any of it parents that
+ * are also being rolled back.
*/
case TBLOCK_SUBABORT_END:
- CleanupSubTransaction();
- CommitTransactionCommand();
- break;
+ case TBLOCK_SUBABORT_PENDING:
+ do {
+ /*
+ * The difference between SUBABORT_END and SUBABORT_PENDING is
+ * whether the subtransaction has already been aborted and we
+ * just need to clean it up, or if we need to also abort it
+ * first.
+ */
+ if (s->blockState == TBLOCK_SUBABORT_PENDING)
+ AbortSubTransaction();
+ CleanupSubTransaction();
+ s = CurrentTransactionState; /* changed by pop */
+ } while (s->blockState == TBLOCK_SUBABORT_END ||
+ s->blockState == TBLOCK_SUBABORT_PENDING);
/*
- * As above, but it's not dead yet, so abort first.
+ * REVIEWER NOTE: the old comment said that the parent can be "in
+ * any of several states". I looked at the code that sets the
+ * state to TBLOCK_SUBABORT_END or TBLOCK_SUBABORT_PENDING, and
+ * came to the conclusion that these are the only possible states
+ * above the chain of TBLOCK_SUBABORT_END or
+ * TBLOCK_SUBABORT_PENDING subtransactions.
*/
- case TBLOCK_SUBABORT_PENDING:
- AbortSubTransaction();
- CleanupSubTransaction();
+ /*
+ * Recurse to roll back the top-level transaction too, or to
+ * restart at a savepoint in case of ROLLBACK TO.
+ */
+ if (s->blockState != TBLOCK_ABORT_PENDING &&
+ s->blockState != TBLOCK_ABORT &&
+ s->blockState != TBLOCK_SUBRESTART &&
+ s->blockState != TBLOCK_SUBABORT_RESTART)
+ {
+ elog(ERROR, "CommitTransactionCommand: unexpected state %s",
+ BlockStateAsString(s->blockState));
+ }
CommitTransactionCommand();
break;
@@ -3243,35 +3258,13 @@ CommitTransactionCommand(void)
* command. Abort and pop it, then start a new subtransaction
* with the same name.
*/
- case TBLOCK_SUBRESTART:
- {
- char *name;
- int savepointLevel;
-
- /* save name and keep Cleanup from freeing it */
- name = s->name;
- s->name = NULL;
- savepointLevel = s->savepointLevel;
-
- AbortSubTransaction();
- CleanupSubTransaction();
-
- DefineSavepoint(NULL);
- s = CurrentTransactionState; /* changed by push */
- s->name = name;
- s->savepointLevel = savepointLevel;
-
- /* This is the same as TBLOCK_SUBBEGIN case */
- Assert(s->blockState == TBLOCK_SUBBEGIN);
- StartSubTransaction();
- s->blockState = TBLOCK_SUBINPROGRESS;
- }
- break;
-
/*
- * Same as above, but the subtransaction had already failed, so we
- * don't need AbortSubTransaction.
+ * REVIEWER NOTE: I merged the TBLOCK_SUBRESTART and
+ * TBLOCK_SUBABORT_RESTART cases. Not strictly necessary, but it
+ * makes this more similar to how the TBLOCK_SUBABORT_END and
+ * TBLOCK_SUBABORT_PENDING cases are handled now.
*/
+ case TBLOCK_SUBRESTART:
case TBLOCK_SUBABORT_RESTART:
{
char *name;
@@ -3282,6 +3275,12 @@ CommitTransactionCommand(void)
s->name = NULL;
savepointLevel = s->savepointLevel;
+ /*
+ * In SUBABORT_RESTART state, the subtransaction had already
+ * failed, so we don't need AbortSubTransaction.
+ */
+ if (s->blockState == TBLOCK_SUBRESTART)
+ AbortSubTransaction();
CleanupSubTransaction();
DefineSavepoint(NULL);
@@ -3437,17 +3436,27 @@ AbortCurrentTransaction(void)
case TBLOCK_SUBCOMMIT:
case TBLOCK_SUBABORT_PENDING:
case TBLOCK_SUBRESTART:
- AbortSubTransaction();
- CleanupSubTransaction();
- AbortCurrentTransaction();
- break;
-
- /*
- * Same as above, except the Abort() was already done.
- */
case TBLOCK_SUBABORT_END:
case TBLOCK_SUBABORT_RESTART:
- CleanupSubTransaction();
+ /*
+ * If multiple subtransactions need to be cleaned up, iterate
+ * through them here. The recursion would handle them too, but
+ * this avoids running out of stack if there is a deep stack of
+ * aborted subtransactions, and the compiler isn't smart enough
+ * to optimize away the tail recursion.
+ */
+ do {
+ if (s->blockState == TBLOCK_SUBABORT_END || s->blockState == TBLOCK_SUBABORT_RESTART)
+ AbortSubTransaction();
+ CleanupSubTransaction();
+ } while(s->blockState == TBLOCK_SUBBEGIN ||
+ s->blockState == TBLOCK_SUBRELEASE ||
+ s->blockState == TBLOCK_SUBCOMMIT ||
+ s->blockState == TBLOCK_SUBABORT_PENDING ||
+ s->blockState == TBLOCK_SUBRESTART ||
+ s->blockState == TBLOCK_SUBABORT_END ||
+ s->blockState == TBLOCK_SUBABORT_RESTART);
+ /* Abort the top-level transaction, too */
AbortCurrentTransaction();
break;
}
--
2.39.2
