From 0d474daa36aa6dd83c64fb6ea4b0fc62a87030a6 Mon Sep 17 00:00:00 2001 From: Alexander Korotkov Date: Wed, 14 Feb 2024 13:16:41 +0200 Subject: [PATCH 1/2] Turn tail recursion into iteration in CommitTransactionCommand() 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 In order to get better readability and less changes to the existing code the recursion-replacing loop is implemented as a wrapper function. Report by Egor Chindyaskin and Alexander Lakhin. Discussion: https://postgr.es/m/1672760457.940462079%40f306.i.mail.ru --- src/backend/access/transam/xact.c | 147 ++++++++++++++++++++---------- 1 file changed, 101 insertions(+), 46 deletions(-) diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c index 464858117e0..fad1157f068 100644 --- a/src/backend/access/transam/xact.c +++ b/src/backend/access/transam/xact.c @@ -3028,14 +3028,18 @@ RestoreTransactionCharacteristics(const SavedTransactionCharacteristics *s) /* - * CommitTransactionCommand + * CommitTransactionCommandInternal */ -void -CommitTransactionCommand(void) +static void +CommitTransactionCommandInternal(void) { TransactionState s = CurrentTransactionState; SavedTransactionCharacteristics savetc; + /* This states are handled in CommitTransactionCommand() */ + Assert(s->blockState != TBLOCK_SUBABORT_END && + s->blockState != TBLOCK_SUBABORT_PENDING); + /* Must save in case we need to restore below */ SaveTransactionCharacteristics(&savetc); @@ -3219,25 +3223,6 @@ CommitTransactionCommand(void) BlockStateAsString(s->blockState)); 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). - */ - case TBLOCK_SUBABORT_END: - CleanupSubTransaction(); - CommitTransactionCommand(); - break; - - /* - * As above, but it's not dead yet, so abort first. - */ - case TBLOCK_SUBABORT_PENDING: - AbortSubTransaction(); - CleanupSubTransaction(); - CommitTransactionCommand(); - break; - /* * The current subtransaction is the target of a ROLLBACK TO * command. Abort and pop it, then start a new subtransaction @@ -3295,17 +3280,66 @@ CommitTransactionCommand(void) s->blockState = TBLOCK_SUBINPROGRESS; } break; + default: + /* Keep compiler quiet */ + break; } } /* - * AbortCurrentTransaction + * CommitTransactionCommand -- the wrapper function handling the + * loop over subtransactions to avoid potentially dangerous recursion in + * CommitTransactionCommandInternal(); */ void -AbortCurrentTransaction(void) +CommitTransactionCommand(void) +{ + while (true) + { + switch (CurrentTransactionState->blockState) + { + /* + * 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). + */ + case TBLOCK_SUBABORT_END: + CleanupSubTransaction(); + continue; + + /* + * As above, but it's not dead yet, so abort first. + */ + case TBLOCK_SUBABORT_PENDING: + AbortSubTransaction(); + CleanupSubTransaction(); + continue; + default: + break; + } + CommitTransactionCommandInternal(); + break; + } +} + +/* + * AbortCurrentTransactionInternal + */ +static void +AbortCurrentTransactionInternal(void) { TransactionState s = CurrentTransactionState; + /* This states are handled in AbortCurrentTransaction() */ + Assert(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); + switch (s->blockState) { case TBLOCK_DEFAULT: @@ -3426,30 +3460,51 @@ AbortCurrentTransaction(void) AbortSubTransaction(); s->blockState = TBLOCK_SUBABORT; break; - - /* - * If we failed while trying to create a subtransaction, clean up - * the broken subtransaction and abort the parent. The same - * applies if we get a failure while ending a subtransaction. - */ - case TBLOCK_SUBBEGIN: - case TBLOCK_SUBRELEASE: - case TBLOCK_SUBCOMMIT: - case TBLOCK_SUBABORT_PENDING: - case TBLOCK_SUBRESTART: - AbortSubTransaction(); - CleanupSubTransaction(); - AbortCurrentTransaction(); + default: + /* Keep compiler quiet */ break; + } +} - /* - * Same as above, except the Abort() was already done. - */ - case TBLOCK_SUBABORT_END: - case TBLOCK_SUBABORT_RESTART: - CleanupSubTransaction(); - AbortCurrentTransaction(); - break; +/* + * AbortCurrentTransaction -- the wrapper function handling the + * loop over subtransactions to avoid potentially dangerous recursion in + * AbortCurrentTransactionInternal(); + */ +void +AbortCurrentTransaction(void) +{ + while (true) + { + switch (CurrentTransactionState->blockState) + { + /* + * If we failed while trying to create a subtransaction, clean + * up the broken subtransaction and abort the parent. The + * same applies if we get a failure while ending a + * subtransaction. + */ + case TBLOCK_SUBBEGIN: + case TBLOCK_SUBRELEASE: + case TBLOCK_SUBCOMMIT: + case TBLOCK_SUBABORT_PENDING: + case TBLOCK_SUBRESTART: + AbortSubTransaction(); + CleanupSubTransaction(); + continue; + + /* + * Same as above, except the Abort() was already done. + */ + case TBLOCK_SUBABORT_END: + case TBLOCK_SUBABORT_RESTART: + CleanupSubTransaction(); + continue; + default: + break; + } + AbortCurrentTransactionInternal(); + break; } } -- 2.39.3 (Apple Git-145)