From fefd9a3fed275cecd9ed4091b00698deed39b92e Mon Sep 17 00:00:00 2001 From: Alexander Korotkov Date: Fri, 8 Mar 2024 13:00:40 +0200 Subject: [PATCH] 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 Author: Alexander Korotkov, Heikki Linnakangas --- src/backend/access/transam/xact.c | 151 +++++++++++++++++++++--------- 1 file changed, 106 insertions(+), 45 deletions(-) diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c index ccd3f4fc55..1bd507230f 100644 --- a/src/backend/access/transam/xact.c +++ b/src/backend/access/transam/xact.c @@ -341,6 +341,9 @@ static void CommitTransaction(void); static TransactionId RecordTransactionAbort(bool isSubXact); static void StartTransaction(void); +static void CommitTransactionCommandInternal(void); +static void AbortCurrentTransactionInternal(void); + static void StartSubTransaction(void); static void CommitSubTransaction(void); static void AbortSubTransaction(void); @@ -3041,16 +3044,58 @@ RestoreTransactionCharacteristics(const SavedTransactionCharacteristics *s) XactDeferrable = s->save_XactDeferrable; } - /* - * CommitTransactionCommand + * CommitTransactionCommand -- a wrapper function handling the + * loop over subtransactions to avoid a potentially dangerous recursion + * in CommitTransactionCommandInternal(). */ 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; + } +} + +/* + * CommitTransactionCommandInternal - a function doing all the material work + * regarding handling the commit transaction command except for loop over + * subtransactions. + */ +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); @@ -3234,25 +3279,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 @@ -3310,17 +3336,73 @@ CommitTransactionCommand(void) s->blockState = TBLOCK_SUBINPROGRESS; } break; + default: + /* Keep compiler quiet */ + break; } } /* - * AbortCurrentTransaction + * AbortCurrentTransaction -- a 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; + } +} + +/* + * AbortCurrentTransactionInternal - a function doing all the material work + * regarding handling the abort transaction command except for loop over + * subtransactions. + */ +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: @@ -3441,29 +3523,8 @@ 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(); - break; - - /* - * Same as above, except the Abort() was already done. - */ - case TBLOCK_SUBABORT_END: - case TBLOCK_SUBABORT_RESTART: - CleanupSubTransaction(); - AbortCurrentTransaction(); + default: + /* Keep compiler quiet */ break; } }