Stack overflow issue

From 5c23f9cd9c16c605acdc36393e1a31e8c9b97a93 Mon Sep 17 00:00:00 2001
From: pgsql-guo <richard.guo@openpie.com>
Date: Wed, 24 Aug 2022 11:45:35 +0000
Subject: [PATCH v1] add check_stack_depth in more places

---
 src/backend/regex/regc_lex.c                         | 3 +++
 src/backend/snowball/libstemmer/stem_UTF_8_turkish.c | 3 +++
 src/backend/tsearch/spell.c                          | 4 ++++
 src/backend/utils/adt/geo_ops.c                      | 3 +++
 src/backend/utils/adt/like_support.c                 | 4 ++++
 5 files changed, 17 insertions(+)

diff --git a/src/backend/regex/regc_lex.c b/src/backend/regex/regc_lex.c
index 45727ffa01..1169e731a1 100644
--- a/src/backend/regex/regc_lex.c
+++ b/src/backend/regex/regc_lex.c
@@ -201,6 +201,9 @@ next(struct vars *v)
 {
 	chr			c;
 
+	/* since this function recurses, it could be driven to stack overflow */
+	check_stack_depth();
+
 	/* errors yield an infinite sequence of failures */
 	if (ISERR())
 		return 0;				/* the error has set nexttype to EOS */
diff --git a/src/backend/snowball/libstemmer/stem_UTF_8_turkish.c b/src/backend/snowball/libstemmer/stem_UTF_8_turkish.c
index 3d04032787..9254574525 100644
--- a/src/backend/snowball/libstemmer/stem_UTF_8_turkish.c
+++ b/src/backend/snowball/libstemmer/stem_UTF_8_turkish.c
@@ -1,6 +1,7 @@
 /* Generated by Snowball 2.2.0 - https://snowballstem.org/ */
 
 #include "header.h"
+#include "miscadmin.h"
 
 #ifdef __cplusplus
 extern "C" {
@@ -1156,6 +1157,8 @@ lab0:
 }
 
 static int r_stem_suffix_chain_before_ki(struct SN_env * z) {
+	/* since this function recurses, it could be driven to stack overflow */
+	check_stack_depth();
     z->ket = z->c;
     {   int ret = r_mark_ki(z);
         if (ret <= 0) return ret;
diff --git a/src/backend/tsearch/spell.c b/src/backend/tsearch/spell.c
index f07321b61d..edd2fbbd3a 100644
--- a/src/backend/tsearch/spell.c
+++ b/src/backend/tsearch/spell.c
@@ -63,6 +63,7 @@
 #include "postgres.h"
 
 #include "catalog/pg_collation.h"
+#include "miscadmin.h"
 #include "tsearch/dicts/spell.h"
 #include "tsearch/ts_locale.h"
 #include "utils/memutils.h"
@@ -2400,6 +2401,9 @@ SplitToVariants(IspellDict *Conf, SPNode *snode, SplitVar *orig, char *word, int
 	char	   *notprobed;
 	int			compoundflag = 0;
 
+	/* since this function recurses, it could be driven to stack overflow */
+	check_stack_depth();
+
 	notprobed = (char *) palloc(wordlen);
 	memset(notprobed, 1, wordlen);
 	var = CopyVar(orig, 1);
diff --git a/src/backend/utils/adt/geo_ops.c b/src/backend/utils/adt/geo_ops.c
index b79705f8b3..535301a218 100644
--- a/src/backend/utils/adt/geo_ops.c
+++ b/src/backend/utils/adt/geo_ops.c
@@ -3833,6 +3833,9 @@ lseg_inside_poly(Point *a, Point *b, POLYGON *poly, int start)
 	bool		res = true,
 				intersection = false;
 
+	/* since this function recurses, it could be driven to stack overflow */
+	check_stack_depth();
+
 	t.p[0] = *a;
 	t.p[1] = *b;
 	s.p[0] = poly->p[(start == 0) ? (poly->npts - 1) : (start - 1)];
diff --git a/src/backend/utils/adt/like_support.c b/src/backend/utils/adt/like_support.c
index 65a57fc3c4..2d3aaaaf6b 100644
--- a/src/backend/utils/adt/like_support.c
+++ b/src/backend/utils/adt/like_support.c
@@ -44,6 +44,7 @@
 #include "catalog/pg_statistic.h"
 #include "catalog/pg_type.h"
 #include "mb/pg_wchar.h"
+#include "miscadmin.h"
 #include "nodes/makefuncs.h"
 #include "nodes/nodeFuncs.h"
 #include "nodes/supportnodes.h"
@@ -1364,6 +1365,9 @@ regex_selectivity_sub(const char *patt, int pattlen, bool case_insensitive)
 	int			paren_pos = 0;	/* dummy init to keep compiler quiet */
 	int			pos;
 
+	/* since this function recurses, it could be driven to stack overflow */
+	check_stack_depth();
+
 	for (pos = 0; pos < pattlen; pos++)
 	{
 		if (patt[pos] == '(')
-- 
2.25.1

diff --git a/src/backend/snowball/dict_snowball.c b/src/backend/snowball/dict_snowball.c
index 68c9213f69..aaf4ff72b6 100644
--- a/src/backend/snowball/dict_snowball.c
+++ b/src/backend/snowball/dict_snowball.c
@@ -272,11 +272,25 @@ dsnowball_lexize(PG_FUNCTION_ARGS)
 	DictSnowball *d = (DictSnowball *) PG_GETARG_POINTER(0);
 	char	   *in = (char *) PG_GETARG_POINTER(1);
 	int32		len = PG_GETARG_INT32(2);
-	char	   *txt = lowerstr_with_len(in, len);
 	TSLexeme   *res = palloc0(sizeof(TSLexeme) * 2);
+	char	   *txt;
 
+	/*
+	 * Reject strings exceeding 1000 bytes, as they're surely not words in any
+	 * human language.  This restriction avoids wasting cycles on stuff like
+	 * base64-encoded data, and it protects us against possible inefficiency
+	 * or misbehavior in the stemmers (for example, the Turkish stemmer has an
+	 * indefinite recursion so it can crash on long-enough strings).
+	 */
+	if (len <= 0 || len > 1000)
+		PG_RETURN_POINTER(res);
+
+	txt = lowerstr_with_len(in, len);
+
+	/* txt is probably not zero-length now, but we'll check anyway */
 	if (*txt == '\0' || searchstoplist(&(d->stoplist), txt))
 	{
+		/* empty or stopword, so reject */
 		pfree(txt);
 	}
 	else

diff --git a/src/backend/snowball/dict_snowball.c b/src/backend/snowball/dict_snowball.c
index 68c9213f69..1d5dfff5a0 100644
--- a/src/backend/snowball/dict_snowball.c
+++ b/src/backend/snowball/dict_snowball.c
@@ -275,8 +275,24 @@ dsnowball_lexize(PG_FUNCTION_ARGS)
 	char	   *txt = lowerstr_with_len(in, len);
 	TSLexeme   *res = palloc0(sizeof(TSLexeme) * 2);
 
-	if (*txt == '\0' || searchstoplist(&(d->stoplist), txt))
+	/*
+	 * Do not pass strings exceeding 1000 bytes to the stemmer, as they're
+	 * surely not words in any human language.  This restriction avoids
+	 * wasting cycles on stuff like base64-encoded data, and it protects us
+	 * against possible inefficiency or misbehavior in the stemmer.  (For
+	 * example, the Turkish stemmer has an indefinite recursion, so it can
+	 * crash on long-enough strings.)  However, Snowball dictionaries are
+	 * defined to recognize all strings, so we can't reject the string as an
+	 * unknown word.
+	 */
+	if (len > 1000)
+	{
+		/* return the lexeme lowercased, but otherwise unmodified */
+		res->lexeme = txt;
+	}
+	else if (*txt == '\0' || searchstoplist(&(d->stoplist), txt))
 	{
+		/* empty or stopword, so report as stopword */
 		pfree(txt);
 	}
 	else

diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index d0e5bc26a7..8c1691fdd9 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -3024,6 +3024,9 @@ CommitTransactionCommand(void)
 	TransactionState s = CurrentTransactionState;
 	SavedTransactionCharacteristics savetc;
 
+	/* since this function recurses, it could be driven to stack overflow */
+	check_stack_depth();
+
 	SaveTransactionCharacteristics(&savetc);
 
 	switch (s->blockState)
@@ -5471,6 +5474,9 @@ ShowTransactionStateRec(const char *str, TransactionState s)
 {
 	StringInfoData buf;
 
+	/* since this function recurses, it could be driven to stack overflow */
+	check_stack_depth();
+
 	initStringInfo(&buf);
 
 	if (s->nChildXids > 0)
diff --git a/src/backend/catalog/dependency.c b/src/backend/catalog/dependency.c
index 85e2a902a2..7a48ad63ef 100644
--- a/src/backend/catalog/dependency.c
+++ b/src/backend/catalog/dependency.c
@@ -75,6 +75,7 @@
 #include "commands/trigger.h"
 #include "commands/typecmds.h"
 #include "funcapi.h"
+#include "miscadmin.h"
 #include "nodes/nodeFuncs.h"
 #include "parser/parsetree.h"
 #include "rewrite/rewriteRemove.h"
@@ -516,6 +517,11 @@ findDependentObjects(const ObjectAddress *object,
 	if (stack_address_present_add_flags(object, objflags, stack))
 		return;
 
+	/* since this function recurses, it could be driven to stack overflow,
+	 * because of the deep dependency tree, not only due to dependency loops.
+	 */
+	check_stack_depth();
+
 	/*
 	 * It's also possible that the target object has already been completely
 	 * processed and put into targetObjects.  If so, again we just add the
diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index a21de89874..e46002c932 100644
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -552,6 +552,9 @@ CheckAttributeType(const char *attname,
 	char		att_typtype = get_typtype(atttypid);
 	Oid			att_typelem;
 
+	/* since this function recurses, it could be driven to stack overflow */
+	check_stack_depth();
+
 	if (att_typtype == TYPTYPE_PSEUDO)
 	{
 		/*
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index fa4faff3c5..e194aff40e 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -6705,6 +6705,9 @@ ATExecAddColumn(List **wqueue, AlteredTableInfo *tab, Relation rel,
 	TupleDesc	tupdesc;
 	FormData_pg_attribute *aattr[] = {&attribute};
 
+	/* since this function recurses, it could be driven to stack overflow */
+	check_stack_depth();
+
 	/* At top level, permission check was done in ATPrepCmd, else do it */
 	if (recursing)
 		ATSimplePermissions((*cmd)->subtype, rel, ATT_TABLE | ATT_FOREIGN_TABLE);
@@ -8433,6 +8436,10 @@ ATExecDropColumn(List **wqueue, Relation rel, const char *colName,
 
 	/* Initialize addrs on the first invocation */
 	Assert(!recursing || addrs != NULL);
+
+	/* since this function recurses, it could be driven to stack overflow */
+	check_stack_depth();
+
 	if (!recursing)
 		addrs = new_object_addresses();
 
@@ -10895,6 +10902,9 @@ ATExecAlterConstrRecurse(Constraint *cmdcon, Relation conrel, Relation tgrel,
 	Oid			refrelid;
 	bool		changed = false;
 
+	/* since this function recurses, it could be driven to stack overflow */
+	check_stack_depth();
+
 	currcon = (Form_pg_constraint) GETSTRUCT(contuple);
 	conoid = currcon->oid;
 	refrelid = currcon->confrelid;
@@ -11896,6 +11906,9 @@ ATExecDropConstraint(Relation rel, const char *constrName,
 	bool		is_no_inherit_constraint = false;
 	char		contype;
 
+	/* since this function recurses, it could be driven to stack overflow */
+	check_stack_depth();
+
 	/* At top level, permission check was done in ATPrepCmd, else do it */
 	if (recursing)
 		ATSimplePermissions(AT_DropConstraint, rel, ATT_TABLE | ATT_FOREIGN_TABLE);
diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c
index bf3a7cae60..e92d0316aa 100644
--- a/src/backend/optimizer/util/clauses.c
+++ b/src/backend/optimizer/util/clauses.c
@@ -2281,6 +2281,10 @@ static Node *
 eval_const_expressions_mutator(Node *node,
 							   eval_const_expressions_context *context)
 {
+
+	/* since this function recurses, it could be driven to stack overflow */
+	check_stack_depth();
+
 	if (node == NULL)
 		return NULL;
 	switch (nodeTag(node))
diff --git a/src/backend/utils/adt/jsonpath_exec.c b/src/backend/utils/adt/jsonpath_exec.c
index ae27baacce..8095f957c9 100644
--- a/src/backend/utils/adt/jsonpath_exec.c
+++ b/src/backend/utils/adt/jsonpath_exec.c
@@ -1231,6 +1231,9 @@ executeBoolItem(JsonPathExecContext *cxt, JsonPathItem *jsp,
 	JsonPathBool res;
 	JsonPathBool res2;
 
+	/* since this function recurses, it could be driven to stack overflow */
+	check_stack_depth();
+
 	if (!canHaveNext && jspHasNext(jsp))
 		elog(ERROR, "boolean jsonpath item cannot have next item");
 
diff --git a/src/backend/utils/mmgr/mcxt.c b/src/backend/utils/mmgr/mcxt.c
index e12be1b9bd..d754b51113 100644
--- a/src/backend/utils/mmgr/mcxt.c
+++ b/src/backend/utils/mmgr/mcxt.c
@@ -223,6 +223,9 @@ MemoryContextDelete(MemoryContext context)
 	/* And not CurrentMemoryContext, either */
 	Assert(context != CurrentMemoryContext);
 
+	/* since this function recurses, it could be driven to stack overflow */
+	check_stack_depth();
+
 	/* save a function call in common case where there are no children */
 	if (context->firstchild != NULL)
 		MemoryContextDeleteChildren(context);
@@ -572,6 +575,9 @@ MemoryContextStatsInternal(MemoryContext context, int level,
 
 	AssertArg(MemoryContextIsValid(context));
 
+	/* since this function recurses, it could be driven to stack overflow */
+	check_stack_depth();
+
 	/* Examine the context itself */
 	context->methods->stats(context,
 							print ? MemoryContextStatsPrint : NULL,
@@ -737,6 +743,9 @@ MemoryContextCheck(MemoryContext context)
 
 	AssertArg(MemoryContextIsValid(context));
 
+	/* since this function recurses, it could be driven to stack overflow */
+	check_stack_depth();
+
 	context->methods->check(context);
 	for (child = context->firstchild; child != NULL; child = child->nextchild)
 		MemoryContextCheck(child);

diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index d85e313908..102d0e1574 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -3043,6 +3043,9 @@ CommitTransactionCommand(void)
 	TransactionState s = CurrentTransactionState;
 	SavedTransactionCharacteristics savetc;
 
+	/* since this function recurses, it could be driven to stack overflow */
+	check_stack_depth();
+
 	SaveTransactionCharacteristics(&savetc);
 
 	switch (s->blockState)
@@ -5500,6 +5503,9 @@ ShowTransactionStateRec(const char *str, TransactionState s)
 {
 	StringInfoData buf;
 
+	/* since this function recurses, it could be driven to stack overflow */
+	check_stack_depth();
+
 	initStringInfo(&buf);
 
 	if (s->nChildXids > 0)
diff --git a/src/backend/catalog/dependency.c b/src/backend/catalog/dependency.c
index 7acf654bf8..58a1d70b8a 100644
--- a/src/backend/catalog/dependency.c
+++ b/src/backend/catalog/dependency.c
@@ -76,6 +76,7 @@
 #include "commands/trigger.h"
 #include "commands/typecmds.h"
 #include "funcapi.h"
+#include "miscadmin.h"
 #include "nodes/nodeFuncs.h"
 #include "parser/parsetree.h"
 #include "rewrite/rewriteRemove.h"
@@ -524,6 +525,11 @@ findDependentObjects(const ObjectAddress *object,
 	if (stack_address_present_add_flags(object, objflags, stack))
 		return;
 
+	/* since this function recurses, it could be driven to stack overflow,
+	 * because of the deep dependency tree, not only due to dependency loops.
+	 */
+	check_stack_depth();
+
 	/*
 	 * It's also possible that the target object has already been completely
 	 * processed and put into targetObjects.  If so, again we just add the
diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index 4f006820b8..a88550913d 100644
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -552,6 +552,9 @@ CheckAttributeType(const char *attname,
 	char		att_typtype = get_typtype(atttypid);
 	Oid			att_typelem;
 
+	/* since this function recurses, it could be driven to stack overflow */
+	check_stack_depth();
+
 	if (att_typtype == TYPTYPE_PSEUDO)
 	{
 		/*
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 7c697a285b..f22da79c65 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -6684,6 +6684,9 @@ ATExecAddColumn(List **wqueue, AlteredTableInfo *tab, Relation rel,
 	TupleDesc	tupdesc;
 	FormData_pg_attribute *aattr[] = {&attribute};
 
+	/* since this function recurses, it could be driven to stack overflow */
+	check_stack_depth();
+
 	/* At top level, permission check was done in ATPrepCmd, else do it */
 	if (recursing)
 		ATSimplePermissions((*cmd)->subtype, rel, ATT_TABLE | ATT_FOREIGN_TABLE);
@@ -8383,6 +8386,10 @@ ATExecDropColumn(List **wqueue, Relation rel, const char *colName,
 
 	/* Initialize addrs on the first invocation */
 	Assert(!recursing || addrs != NULL);
+
+	/* since this function recurses, it could be driven to stack overflow */
+	check_stack_depth();
+
 	if (!recursing)
 		addrs = new_object_addresses();
 
@@ -10839,6 +10846,9 @@ ATExecAlterConstrRecurse(Constraint *cmdcon, Relation conrel, Relation tgrel,
 	Oid			refrelid;
 	bool		changed = false;
 
+	/* since this function recurses, it could be driven to stack overflow */
+	check_stack_depth();
+
 	currcon = (Form_pg_constraint) GETSTRUCT(contuple);
 	conoid = currcon->oid;
 	refrelid = currcon->confrelid;
@@ -11839,6 +11849,9 @@ ATExecDropConstraint(Relation rel, const char *constrName,
 	bool		is_no_inherit_constraint = false;
 	char		contype;
 
+	/* since this function recurses, it could be driven to stack overflow */
+	check_stack_depth();
+
 	/* At top level, permission check was done in ATPrepCmd, else do it */
 	if (recursing)
 		ATSimplePermissions(AT_DropConstraint, rel, ATT_TABLE | ATT_FOREIGN_TABLE);
diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c
index aa584848cf..77a5eb526c 100644
--- a/src/backend/optimizer/util/clauses.c
+++ b/src/backend/optimizer/util/clauses.c
@@ -2318,6 +2318,10 @@ static Node *
 eval_const_expressions_mutator(Node *node,
 							   eval_const_expressions_context *context)
 {
+
+	/* since this function recurses, it could be driven to stack overflow */
+	check_stack_depth();
+
 	if (node == NULL)
 		return NULL;
 	switch (nodeTag(node))
diff --git a/src/backend/utils/adt/jsonpath_exec.c b/src/backend/utils/adt/jsonpath_exec.c
index b561f0e7e8..dc7ab387ea 100644
--- a/src/backend/utils/adt/jsonpath_exec.c
+++ b/src/backend/utils/adt/jsonpath_exec.c
@@ -1232,6 +1232,9 @@ executeBoolItem(JsonPathExecContext *cxt, JsonPathItem *jsp,
 	JsonPathBool res;
 	JsonPathBool res2;
 
+	/* since this function recurses, it could be driven to stack overflow */
+	check_stack_depth();
+
 	if (!canHaveNext && jspHasNext(jsp))
 		elog(ERROR, "boolean jsonpath item cannot have next item");
 
diff --git a/src/backend/utils/mmgr/mcxt.c b/src/backend/utils/mmgr/mcxt.c
index 0b00802df7..cc06c00a49 100644
--- a/src/backend/utils/mmgr/mcxt.c
+++ b/src/backend/utils/mmgr/mcxt.c
@@ -392,6 +392,9 @@ MemoryContextDelete(MemoryContext context)
 	/* And not CurrentMemoryContext, either */
 	Assert(context != CurrentMemoryContext);
 
+	/* since this function recurses, it could be driven to stack overflow */
+	check_stack_depth();
+
 	/* save a function call in common case where there are no children */
 	if (context->firstchild != NULL)
 		MemoryContextDeleteChildren(context);
@@ -750,6 +753,9 @@ MemoryContextStatsInternal(MemoryContext context, int level,
 
 	Assert(MemoryContextIsValid(context));
 
+	/* since this function recurses, it could be driven to stack overflow */
+	check_stack_depth();
+
 	/* Examine the context itself */
 	context->methods->stats(context,
 							print ? MemoryContextStatsPrint : NULL,
@@ -915,6 +921,9 @@ MemoryContextCheck(MemoryContext context)
 
 	Assert(MemoryContextIsValid(context));
 
+	/* since this function recurses, it could be driven to stack overflow */
+	check_stack_depth();
+
 	context->methods->check(context);
 	for (child = context->firstchild; child != NULL; child = child->nextchild)
 		MemoryContextCheck(child);

diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index fd1a7ac5d5..318f620ed5 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -2812,6 +2812,9 @@ CommitTransactionCommand(void)
 {
 	TransactionState s = CurrentTransactionState;
 
+	/* since this function recurses, it could be driven to stack overflow */
+	check_stack_depth();
+
 	switch (s->blockState)
 	{
 			/*
@@ -5194,6 +5197,9 @@ ShowTransactionStateRec(const char *str, TransactionState s)
 {
 	StringInfoData buf;
 
+	/* since this function recurses, it could be driven to stack overflow */
+	check_stack_depth();
+
 	initStringInfo(&buf);
 
 	if (s->nChildXids > 0)
diff --git a/src/backend/catalog/dependency.c b/src/backend/catalog/dependency.c
index f8510a1483..d1bbc11d3a 100644
--- a/src/backend/catalog/dependency.c
+++ b/src/backend/catalog/dependency.c
@@ -74,6 +74,7 @@
 #include "nodes/nodeFuncs.h"
 #include "parser/parsetree.h"
 #include "rewrite/rewriteRemove.h"
+#include "miscadmin.h"
 #include "storage/lmgr.h"
 #include "utils/fmgroids.h"
 #include "utils/guc.h"
@@ -489,6 +490,11 @@ findDependentObjects(const ObjectAddress *object,
 	if (stack_address_present_add_flags(object, objflags, stack))
 		return;
 
+	/* since this function recurses, it could be driven to stack overflow,
+	 * because of the deep dependency tree, not only due to dependency loops.
+	 */
+	check_stack_depth();
+
 	/*
 	 * It's also possible that the target object has already been completely
 	 * processed and put into targetObjects.  If so, again we just add the
diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index 846c530154..2f243bcf5e 100644
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -513,6 +513,9 @@ CheckAttributeType(const char *attname,
 	char		att_typtype = get_typtype(atttypid);
 	Oid			att_typelem;
 
+	/* since this function recurses, it could be driven to stack overflow */
+	check_stack_depth();
+
 	if (att_typtype == TYPTYPE_PSEUDO)
 	{
 		/*
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index f56219973e..4cf08a1e04 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -5578,6 +5578,9 @@ ATExecAddColumn(List **wqueue, AlteredTableInfo *tab, Relation rel,
 	AclResult	aclresult;
 	ObjectAddress address;
 
+	/* since this function recurses, it could be driven to stack overflow */
+	check_stack_depth();
+
 	/* At top level, permission check was done in ATPrepCmd, else do it */
 	if (recursing)
 		ATSimplePermissions(rel, ATT_TABLE | ATT_FOREIGN_TABLE);
@@ -7004,6 +7007,9 @@ ATExecDropColumn(List **wqueue, Relation rel, const char *colName,
 	ObjectAddress object;
 	bool		is_expr;
 
+	/* since this function recurses, it could be driven to stack overflow */
+	check_stack_depth();
+
 	/* At top level, permission check was done in ATPrepCmd, else do it */
 	if (recursing)
 		ATSimplePermissions(rel, ATT_TABLE | ATT_FOREIGN_TABLE);
@@ -8563,6 +8569,9 @@ ATExecAlterConstrRecurse(Constraint *cmdcon, Relation conrel, Relation tgrel,
 	Oid			conoid;
 	bool		changed = false;
 
+	/* since this function recurses, it could be driven to stack overflow */
+	check_stack_depth();
+
 	currcon = (Form_pg_constraint) GETSTRUCT(contuple);
 	conoid = HeapTupleGetOid(contuple);
 
@@ -9552,6 +9561,9 @@ ATExecDropConstraint(Relation rel, const char *constrName,
 	bool		is_no_inherit_constraint = false;
 	char		contype;
 
+	/* since this function recurses, it could be driven to stack overflow */
+	check_stack_depth();
+
 	/* At top level, permission check was done in ATPrepCmd, else do it */
 	if (recursing)
 		ATSimplePermissions(rel, ATT_TABLE | ATT_FOREIGN_TABLE);
diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c
index edb56ab3a4..67b3a1f4bf 100644
--- a/src/backend/optimizer/util/clauses.c
+++ b/src/backend/optimizer/util/clauses.c
@@ -2650,6 +2650,10 @@ static Node *
 eval_const_expressions_mutator(Node *node,
 							   eval_const_expressions_context *context)
 {
+
+	/* since this function recurses, it could be driven to stack overflow */
+	check_stack_depth();
+
 	if (node == NULL)
 		return NULL;
 	switch (nodeTag(node))
diff --git a/src/backend/utils/mmgr/mcxt.c b/src/backend/utils/mmgr/mcxt.c
index 22da98c19d..800c1ffac0 100644
--- a/src/backend/utils/mmgr/mcxt.c
+++ b/src/backend/utils/mmgr/mcxt.c
@@ -216,6 +216,9 @@ MemoryContextDelete(MemoryContext context)
 	/* And not CurrentMemoryContext, either */
 	Assert(context != CurrentMemoryContext);
 
+	/* since this function recurses, it could be driven to stack overflow */
+	check_stack_depth();
+
 	/* save a function call in common case where there are no children */
 	if (context->firstchild != NULL)
 		MemoryContextDeleteChildren(context);
@@ -516,6 +519,9 @@ MemoryContextStatsInternal(MemoryContext context, int level,
 
 	AssertArg(MemoryContextIsValid(context));
 
+	/* since this function recurses, it could be driven to stack overflow */
+	check_stack_depth();
+
 	/* Examine the context itself */
 	context->methods->stats(context,
 							print ? MemoryContextStatsPrint : NULL,
@@ -646,6 +652,9 @@ MemoryContextCheck(MemoryContext context)
 
 	AssertArg(MemoryContextIsValid(context));
 
+	/* since this function recurses, it could be driven to stack overflow */
+	check_stack_depth();
+
 	context->methods->check(context);
 	for (child = context->firstchild; child != NULL; child = child->nextchild)
 		MemoryContextCheck(child);

diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index b304a85d08..387e8acd27 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -2913,6 +2913,9 @@ CommitTransactionCommand(void)
 {
 	TransactionState s = CurrentTransactionState;
 
+	/* since this function recurses, it could be driven to stack overflow */
+	check_stack_depth();
+
 	if (s->chain)
 		SaveTransactionCharacteristics();
 
@@ -5361,6 +5364,9 @@ ShowTransactionStateRec(const char *str, TransactionState s)
 {
 	StringInfoData buf;
 
+	/* since this function recurses, it could be driven to stack overflow */
+	check_stack_depth();
+
 	initStringInfo(&buf);
 
 	if (s->nChildXids > 0)
diff --git a/src/backend/catalog/dependency.c b/src/backend/catalog/dependency.c
index 46ced9bb60..3115476632 100644
--- a/src/backend/catalog/dependency.c
+++ b/src/backend/catalog/dependency.c
@@ -73,6 +73,7 @@
 #include "commands/sequence.h"
 #include "commands/trigger.h"
 #include "commands/typecmds.h"
+#include "miscadmin.h"
 #include "nodes/nodeFuncs.h"
 #include "parser/parsetree.h"
 #include "rewrite/rewriteRemove.h"
@@ -509,6 +510,11 @@ findDependentObjects(const ObjectAddress *object,
 	if (stack_address_present_add_flags(object, objflags, stack))
 		return;
 
+	/* since this function recurses, it could be driven to stack overflow,
+	 * because of the deep dependency tree, not only due to dependency loops.
+	 */
+	check_stack_depth();
+
 	/*
 	 * It's also possible that the target object has already been completely
 	 * processed and put into targetObjects.  If so, again we just add the
diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index ceccbdb8fe..395e5edd71 100644
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -595,6 +595,9 @@ CheckAttributeType(const char *attname,
 	char		att_typtype = get_typtype(atttypid);
 	Oid			att_typelem;
 
+	/* since this function recurses, it could be driven to stack overflow */
+	check_stack_depth();
+
 	if (att_typtype == TYPTYPE_PSEUDO)
 	{
 		/*
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index a57fe61187..1dc23f23db 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -5900,4 +5900,7 @@ ATExecAddColumn(List **wqueue, AlteredTableInfo *tab, Relation rel,
 
+	/* since this function recurses, it could be driven to stack overflow */
+	check_stack_depth();
+
 	/* At top level, permission check was done in ATPrepCmd, else do it */
 	if (recursing)
 		ATSimplePermissions(rel, ATT_TABLE | ATT_FOREIGN_TABLE);
@@ -7460,6 +7463,10 @@ ATExecDropColumn(List **wqueue, Relation rel, const char *colName,
 
 	/* Initialize addrs on the first invocation */
 	Assert(!recursing || addrs != NULL);
+
+	/* since this function recurses, it could be driven to stack overflow */
+	check_stack_depth();
+
 	if (!recursing)
 		addrs = new_object_addresses();
 
@@ -9587,6 +9594,9 @@ ATExecAlterConstrRecurse(Constraint *cmdcon, Relation conrel, Relation tgrel,
 	Oid			refrelid;
 	bool		changed = false;
 
+	/* since this function recurses, it could be driven to stack overflow */
+	check_stack_depth();
+
 	currcon = (Form_pg_constraint) GETSTRUCT(contuple);
 	conoid = currcon->oid;
 	refrelid = currcon->confrelid;
@@ -10556,6 +10566,9 @@ ATExecDropConstraint(Relation rel, const char *constrName,
 	bool		is_no_inherit_constraint = false;
 	char		contype;
 
+	/* since this function recurses, it could be driven to stack overflow */
+	check_stack_depth();
+
 	/* At top level, permission check was done in ATPrepCmd, else do it */
 	if (recursing)
 		ATSimplePermissions(rel, ATT_TABLE | ATT_FOREIGN_TABLE);
diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c
index 946e232cca..726b30a4e2 100644
--- a/src/backend/optimizer/util/clauses.c
+++ b/src/backend/optimizer/util/clauses.c
@@ -2392,6 +2392,10 @@ static Node *
 eval_const_expressions_mutator(Node *node,
 							   eval_const_expressions_context *context)
 {
+
+	/* since this function recurses, it could be driven to stack overflow */
+	check_stack_depth();
+
 	if (node == NULL)
 		return NULL;
 	switch (nodeTag(node))
diff --git a/src/backend/utils/adt/jsonpath_exec.c b/src/backend/utils/adt/jsonpath_exec.c
index 37b08f1742..e115b03a3d 100644
--- a/src/backend/utils/adt/jsonpath_exec.c
+++ b/src/backend/utils/adt/jsonpath_exec.c
@@ -1160,6 +1160,9 @@ executeBoolItem(JsonPathExecContext *cxt, JsonPathItem *jsp,
 	JsonPathBool res;
 	JsonPathBool res2;
 
+	/* since this function recurses, it could be driven to stack overflow */
+	check_stack_depth();
+
 	if (!canHaveNext && jspHasNext(jsp))
 		elog(ERROR, "boolean jsonpath item cannot have next item");
 
diff --git a/src/backend/utils/mmgr/mcxt.c b/src/backend/utils/mmgr/mcxt.c
index b07be12236..16ecf0538c 100644
--- a/src/backend/utils/mmgr/mcxt.c
+++ b/src/backend/utils/mmgr/mcxt.c
@@ -216,6 +216,9 @@ MemoryContextDelete(MemoryContext context)
 	/* And not CurrentMemoryContext, either */
 	Assert(context != CurrentMemoryContext);
 
+	/* since this function recurses, it could be driven to stack overflow */
+	check_stack_depth();
+
 	/* save a function call in common case where there are no children */
 	if (context->firstchild != NULL)
 		MemoryContextDeleteChildren(context);
@@ -516,6 +519,9 @@ MemoryContextStatsInternal(MemoryContext context, int level,
 
 	AssertArg(MemoryContextIsValid(context));
 
+	/* since this function recurses, it could be driven to stack overflow */
+	check_stack_depth();
+
 	/* Examine the context itself */
 	context->methods->stats(context,
 							print ? MemoryContextStatsPrint : NULL,
@@ -646,6 +652,9 @@ MemoryContextCheck(MemoryContext context)
 
 	AssertArg(MemoryContextIsValid(context));
 
+	/* since this function recurses, it could be driven to stack overflow */
+	check_stack_depth();
+
 	context->methods->check(context);
 	for (child = context->firstchild; child != NULL; child = child->nextchild)
 		MemoryContextCheck(child);

diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index d85e313908..102d0e1574 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -3043,6 +3043,9 @@ CommitTransactionCommand(void)
 	TransactionState s = CurrentTransactionState;
 	SavedTransactionCharacteristics savetc;
 
+	/* since this function recurses, it could be driven to stack overflow */
+	check_stack_depth();
+
 	SaveTransactionCharacteristics(&savetc);
 
 	switch (s->blockState)
@@ -5500,6 +5503,9 @@ ShowTransactionStateRec(const char *str, TransactionState s)
 {
 	StringInfoData buf;
 
+	/* since this function recurses, it could be driven to stack overflow */
+	check_stack_depth();
+
 	initStringInfo(&buf);
 
 	if (s->nChildXids > 0)
diff --git a/src/backend/catalog/dependency.c b/src/backend/catalog/dependency.c
index 7acf654bf8..58a1d70b8a 100644
--- a/src/backend/catalog/dependency.c
+++ b/src/backend/catalog/dependency.c
@@ -76,6 +76,7 @@
 #include "commands/trigger.h"
 #include "commands/typecmds.h"
 #include "funcapi.h"
+#include "miscadmin.h"
 #include "nodes/nodeFuncs.h"
 #include "parser/parsetree.h"
 #include "rewrite/rewriteRemove.h"
@@ -524,6 +525,11 @@ findDependentObjects(const ObjectAddress *object,
 	if (stack_address_present_add_flags(object, objflags, stack))
 		return;
 
+	/* since this function recurses, it could be driven to stack overflow,
+	 * because of the deep dependency tree, not only due to dependency loops.
+	 */
+	check_stack_depth();
+
 	/*
 	 * It's also possible that the target object has already been completely
 	 * processed and put into targetObjects.  If so, again we just add the
diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index 4f006820b8..a88550913d 100644
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -552,6 +552,9 @@ CheckAttributeType(const char *attname,
 	char		att_typtype = get_typtype(atttypid);
 	Oid			att_typelem;
 
+	/* since this function recurses, it could be driven to stack overflow */
+	check_stack_depth();
+
 	if (att_typtype == TYPTYPE_PSEUDO)
 	{
 		/*
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 7c697a285b..f22da79c65 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -6684,6 +6684,9 @@ ATExecAddColumn(List **wqueue, AlteredTableInfo *tab, Relation rel,
 	TupleDesc	tupdesc;
 	FormData_pg_attribute *aattr[] = {&attribute};
 
+	/* since this function recurses, it could be driven to stack overflow */
+	check_stack_depth();
+
 	/* At top level, permission check was done in ATPrepCmd, else do it */
 	if (recursing)
 		ATSimplePermissions((*cmd)->subtype, rel, ATT_TABLE | ATT_FOREIGN_TABLE);
@@ -8383,6 +8386,10 @@ ATExecDropColumn(List **wqueue, Relation rel, const char *colName,
 
 	/* Initialize addrs on the first invocation */
 	Assert(!recursing || addrs != NULL);
+
+	/* since this function recurses, it could be driven to stack overflow */
+	check_stack_depth();
+
 	if (!recursing)
 		addrs = new_object_addresses();
 
@@ -10839,6 +10846,9 @@ ATExecAlterConstrRecurse(Constraint *cmdcon, Relation conrel, Relation tgrel,
 	Oid			refrelid;
 	bool		changed = false;
 
+	/* since this function recurses, it could be driven to stack overflow */
+	check_stack_depth();
+
 	currcon = (Form_pg_constraint) GETSTRUCT(contuple);
 	conoid = currcon->oid;
 	refrelid = currcon->confrelid;
@@ -11839,6 +11849,9 @@ ATExecDropConstraint(Relation rel, const char *constrName,
 	bool		is_no_inherit_constraint = false;
 	char		contype;
 
+	/* since this function recurses, it could be driven to stack overflow */
+	check_stack_depth();
+
 	/* At top level, permission check was done in ATPrepCmd, else do it */
 	if (recursing)
 		ATSimplePermissions(AT_DropConstraint, rel, ATT_TABLE | ATT_FOREIGN_TABLE);
diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c
index aa584848cf..77a5eb526c 100644
--- a/src/backend/optimizer/util/clauses.c
+++ b/src/backend/optimizer/util/clauses.c
@@ -2318,6 +2318,10 @@ static Node *
 eval_const_expressions_mutator(Node *node,
 							   eval_const_expressions_context *context)
 {
+
+	/* since this function recurses, it could be driven to stack overflow */
+	check_stack_depth();
+
 	if (node == NULL)
 		return NULL;
 	switch (nodeTag(node))
diff --git a/src/backend/utils/adt/jsonpath_exec.c b/src/backend/utils/adt/jsonpath_exec.c
index b561f0e7e8..dc7ab387ea 100644
--- a/src/backend/utils/adt/jsonpath_exec.c
+++ b/src/backend/utils/adt/jsonpath_exec.c
@@ -1232,6 +1232,9 @@ executeBoolItem(JsonPathExecContext *cxt, JsonPathItem *jsp,
 	JsonPathBool res;
 	JsonPathBool res2;
 
+	/* since this function recurses, it could be driven to stack overflow */
+	check_stack_depth();
+
 	if (!canHaveNext && jspHasNext(jsp))
 		elog(ERROR, "boolean jsonpath item cannot have next item");
 
diff --git a/src/backend/utils/mmgr/mcxt.c b/src/backend/utils/mmgr/mcxt.c
index 0b00802df7..cc06c00a49 100644
--- a/src/backend/utils/mmgr/mcxt.c
+++ b/src/backend/utils/mmgr/mcxt.c
@@ -392,6 +392,9 @@ MemoryContextDelete(MemoryContext context)
 	/* And not CurrentMemoryContext, either */
 	Assert(context != CurrentMemoryContext);
 
+	/* since this function recurses, it could be driven to stack overflow */
+	check_stack_depth();
+
 	/* save a function call in common case where there are no children */
 	if (context->firstchild != NULL)
 		MemoryContextDeleteChildren(context);
@@ -750,6 +753,9 @@ MemoryContextStatsInternal(MemoryContext context, int level,
 
 	Assert(MemoryContextIsValid(context));
 
+	/* since this function recurses, it could be driven to stack overflow */
+	check_stack_depth();
+
 	/* Examine the context itself */
 	context->methods->stats(context,
 							print ? MemoryContextStatsPrint : NULL,
@@ -915,6 +921,9 @@ MemoryContextCheck(MemoryContext context)
 
 	Assert(MemoryContextIsValid(context));
 
+	/* since this function recurses, it could be driven to stack overflow */
+	check_stack_depth();
+
 	context->methods->check(context);
 	for (child = context->firstchild; child != NULL; child = child->nextchild)
 		MemoryContextCheck(child);

From 543e6569a3f4730f3105379cc946cc6b54525179 Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakangas@iki.fi>
Date: Fri, 24 Nov 2023 14:13:10 +0200
Subject: [PATCH 1/4] 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

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 | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index 536edb3792..e16b73a9f1 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -3033,12 +3033,15 @@ RestoreTransactionCharacteristics(const SavedTransactionCharacteristics *s)
 void
 CommitTransactionCommand(void)
 {
-	TransactionState s = CurrentTransactionState;
+	TransactionState s;
 	SavedTransactionCharacteristics savetc;
 
+recurse:
+	s = CurrentTransactionState;
 	/* Must save in case we need to restore below */
 	SaveTransactionCharacteristics(&savetc);
 
+	s = CurrentTransactionState;
 	switch (s->blockState)
 	{
 			/*
@@ -3226,8 +3229,7 @@ CommitTransactionCommand(void)
 			 */
 		case TBLOCK_SUBABORT_END:
 			CleanupSubTransaction();
-			CommitTransactionCommand();
-			break;
+			goto recurse;
 
 			/*
 			 * As above, but it's not dead yet, so abort first.
@@ -3235,8 +3237,7 @@ CommitTransactionCommand(void)
 		case TBLOCK_SUBABORT_PENDING:
 			AbortSubTransaction();
 			CleanupSubTransaction();
-			CommitTransactionCommand();
-			break;
+			goto recurse;
 
 			/*
 			 * The current subtransaction is the target of a ROLLBACK TO
-- 
2.39.2

From 1da20313b6252346fbc3f46cdfde612532e98c84 Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakangas@iki.fi>
Date: Fri, 24 Nov 2023 15:06:53 +0200
Subject: [PATCH 2/4] Avoid stack overflow in ShowTransactionStateRec()

The function recurses, but didn't perform stack-depth checks. It's
just a debugging aid, so instead of the usual check_stack_depth()
call, stop the printing if we'd risk stack overflow.

Here's an example of how to test this:

    (n=1000000; printf "BEGIN;"; for ((i=1;i<=$n;i++)); do printf "SAVEPOINT s$i;"; done; printf "SET log_min_messages = 'DEBUG5'; SAVEPOINT sp;") | psql >/dev/null

In the passing, swap building the list of child XIDs and recursing to
parent. That saves memory while recursing, reducing the risk of out of
memory errors with lots of subtransactions. The saving is not very
significant in practice, but this order seems more logical anyway.

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 | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index e16b73a9f1..53584f7dc6 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -5507,8 +5507,22 @@ ShowTransactionStateRec(const char *str, TransactionState s)
 {
 	StringInfoData buf;
 
-	initStringInfo(&buf);
+	if (s->parent)
+	{
+		/*
+		 * Since this function recurses, it could be driven to stack overflow.
+		 * This is just a debugging aid, so we can leave out some details
+		 * instead of erroring out with check_stack_depth().
+		 */
+		if (stack_is_too_deep())
+			ereport(DEBUG5,
+					(errmsg_internal("%s(%d): parent omitted to avoid stack overflow",
+									 str, s->nestingLevel)));
+		else
+			ShowTransactionStateRec(str, s->parent);
+	}
 
+	initStringInfo(&buf);
 	if (s->nChildXids > 0)
 	{
 		int			i;
@@ -5517,10 +5531,6 @@ ShowTransactionStateRec(const char *str, TransactionState s)
 		for (i = 1; i < s->nChildXids; i++)
 			appendStringInfo(&buf, " %u", s->childXids[i]);
 	}
-
-	if (s->parent)
-		ShowTransactionStateRec(str, s->parent);
-
 	ereport(DEBUG5,
 			(errmsg_internal("%s(%d) name: %s; blockState: %s; state: %s, xid/subid/cid: %u/%u/%u%s%s",
 							 str, s->nestingLevel,
@@ -5532,7 +5542,6 @@ ShowTransactionStateRec(const char *str, TransactionState s)
 							 (unsigned int) currentCommandId,
 							 currentCommandIdUsed ? " (used)" : "",
 							 buf.data)));
-
 	pfree(buf.data);
 }
 
-- 
2.39.2

From be7a1be658479de72ee694e41a8ce0f07b8b40ad Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakangas@iki.fi>
Date: Fri, 24 Nov 2023 17:03:28 +0200
Subject: [PATCH 3/4] Avoid recursion in MemoryContext functions.

You might run out of stack space with recursion, which is not nice in
functions that might be used e.g. at cleanup after transaction
abort. MemoryContext contains pointer to parent and siblings, so we
can traverse a tree of contexts iteratively, without using
stack. Refactor the functions to do that.

MemoryContextStats() still recurses, but it now has a limit to how
deep it recurses. Once the limit is reached, it prints just a summary
of the rest of the hierarchy, similar to how it summarizes contexts
with lots of children. That seems good anyway, because a context dump
with hundreds of nested contexts isn't very readable.

Report by Egor Chindyaskin and Alexander Lakhin.

Discussion: https://www.postgresql.org/message-id/1672760457.940462079%40f306.i.mail.ru
---
 src/backend/utils/mmgr/mcxt.c | 244 +++++++++++++++++++++++-----------
 src/include/utils/memutils.h  |   3 +-
 2 files changed, 167 insertions(+), 80 deletions(-)

diff --git a/src/backend/utils/mmgr/mcxt.c b/src/backend/utils/mmgr/mcxt.c
index 9fc83f11f6..2e9563d8c1 100644
--- a/src/backend/utils/mmgr/mcxt.c
+++ b/src/backend/utils/mmgr/mcxt.c
@@ -149,9 +149,10 @@ MemoryContext CurTransactionContext = NULL;
 /* This is a transient link to the active portal's memory context: */
 MemoryContext PortalContext = NULL;
 
+static void MemoryContextDeleteOnly(MemoryContext context);
 static void MemoryContextCallResetCallbacks(MemoryContext context);
 static void MemoryContextStatsInternal(MemoryContext context, int level,
-									   bool print, int max_children,
+									   int max_level, int max_children,
 									   MemoryContextCounters *totals,
 									   bool print_to_stderr);
 static void MemoryContextStatsPrint(MemoryContext context, void *passthru,
@@ -260,6 +261,41 @@ BogusGetChunkSpace(void *pointer)
 	return 0;					/* keep compiler quiet */
 }
 
+/*
+ * MemoryContextTraverse
+ *		Helper function to traverse all descendants of a memory context
+ *		without recursion.
+ *
+ * Recursion could lead to out-of-stack errors with deep context hierarchies,
+ * which would be unpleasant in error cleanup code paths.
+ *
+ * To process 'context' and all its descendants, use a loop like this:
+ *
+ *     <process 'context'>
+ *     for (MemoryContext curr = context->firstchild;
+ *          curr != NULL;
+ *          curr = MemoryContextTraverse(curr, context))
+ *     {
+ *         <process 'curr'>
+ *     }
+ *
+ * This visits all the contexts in preorder.
+ */
+static MemoryContext
+MemoryContextTraverse(MemoryContext curr, MemoryContext top)
+{
+	if (curr->firstchild != NULL)
+		return curr->firstchild;
+
+	while (curr->nextchild == NULL)
+	{
+		curr = curr->parent;
+		if (curr == top)
+			return NULL;
+	}
+	return curr->nextchild;
+}
+
 
 /*****************************************************************************
  *	  EXPORTED ROUTINES														 *
@@ -379,14 +415,13 @@ MemoryContextResetOnly(MemoryContext context)
 void
 MemoryContextResetChildren(MemoryContext context)
 {
-	MemoryContext child;
-
 	Assert(MemoryContextIsValid(context));
 
-	for (child = context->firstchild; child != NULL; child = child->nextchild)
+	for (MemoryContext curr = context->firstchild;
+		 curr != NULL;
+		 curr = MemoryContextTraverse(curr, context))
 	{
-		MemoryContextResetChildren(child);
-		MemoryContextResetOnly(child);
+		MemoryContextResetOnly(curr);
 	}
 }
 
@@ -401,16 +436,53 @@ MemoryContextResetChildren(MemoryContext context)
  */
 void
 MemoryContextDelete(MemoryContext context)
+{
+	MemoryContext curr;
+
+	/*
+	 * Delete subcontexts from the bottom up.
+	 *
+	 * Note: Do not use recursion here.  A "stack depth limit exceeded" error
+	 * would be unpleasant if we're already in the process of cleaning up from
+	 * transaction abort.  We also cannot use MemoryContextTraverse() here
+	 * because we modify the tree as we go.
+	 */
+	curr = context;
+	for (;;)
+	{
+		MemoryContext parent;
+
+		/* Descend down until we find a leaf context with no children */
+		while (curr->firstchild != NULL)
+			curr = curr->firstchild;
+
+		/*
+		 * We're now at a leaf with no children. Free it and continue from the
+		 * parent.  Or if this was the original node, we're all done.
+		 */
+		parent = curr->parent;
+		MemoryContextDeleteOnly(curr);
+
+		if (curr == context)
+			break;
+		curr = parent;
+	}
+}
+
+/*
+ * Subroutine of MemoryContextDelete, to delete a context that has no
+ * children.
+ */
+static void
+MemoryContextDeleteOnly(MemoryContext context)
 {
 	Assert(MemoryContextIsValid(context));
 	/* We had better not be deleting TopMemoryContext ... */
 	Assert(context != TopMemoryContext);
 	/* And not CurrentMemoryContext, either */
 	Assert(context != CurrentMemoryContext);
-
-	/* save a function call in common case where there are no children */
-	if (context->firstchild != NULL)
-		MemoryContextDeleteChildren(context);
+	/* All the children should've been deleted already */
+	Assert(context->firstchild == NULL);
 
 	/*
 	 * It's not entirely clear whether 'tis better to do this before or after
@@ -676,12 +748,12 @@ MemoryContextMemAllocated(MemoryContext context, bool recurse)
 
 	if (recurse)
 	{
-		MemoryContext child;
-
-		for (child = context->firstchild;
-			 child != NULL;
-			 child = child->nextchild)
-			total += MemoryContextMemAllocated(child, true);
+		for (MemoryContext curr = context->firstchild;
+			 curr != NULL;
+			 curr = MemoryContextTraverse(curr, context))
+		{
+			total += curr->mem_allocated;
+		}
 	}
 
 	return total;
@@ -698,8 +770,8 @@ MemoryContextMemAllocated(MemoryContext context, bool recurse)
 void
 MemoryContextStats(MemoryContext context)
 {
-	/* A hard-wired limit on the number of children is usually good enough */
-	MemoryContextStatsDetail(context, 100, true);
+	/* Hard-wired limits are usually good enough */
+	MemoryContextStatsDetail(context, 100, 100, true);
 }
 
 /*
@@ -711,14 +783,15 @@ MemoryContextStats(MemoryContext context)
  * with fprintf(stderr), otherwise use ereport().
  */
 void
-MemoryContextStatsDetail(MemoryContext context, int max_children,
+MemoryContextStatsDetail(MemoryContext context,
+						 int max_level, int max_children,
 						 bool print_to_stderr)
 {
 	MemoryContextCounters grand_totals;
 
 	memset(&grand_totals, 0, sizeof(grand_totals));
 
-	MemoryContextStatsInternal(context, 0, true, max_children, &grand_totals, print_to_stderr);
+	MemoryContextStatsInternal(context, 0, max_level, max_children, &grand_totals, print_to_stderr);
 
 	if (print_to_stderr)
 		fprintf(stderr,
@@ -756,78 +829,89 @@ MemoryContextStatsDetail(MemoryContext context, int max_children,
  */
 static void
 MemoryContextStatsInternal(MemoryContext context, int level,
-						   bool print, int max_children,
+						   int max_level, int max_children,
 						   MemoryContextCounters *totals,
 						   bool print_to_stderr)
 {
-	MemoryContextCounters local_totals;
 	MemoryContext child;
 	int			ichild;
 
 	Assert(MemoryContextIsValid(context));
 
 	/* Examine the context itself */
-	context->methods->stats(context,
-							print ? MemoryContextStatsPrint : NULL,
-							(void *) &level,
-							totals, print_to_stderr);
+	{
+		context->methods->stats(context,
+								MemoryContextStatsPrint,
+								(void *) &level,
+								totals,
+								print_to_stderr);
+	}
 
 	/*
-	 * Examine children.  If there are more than max_children of them, we do
-	 * not print the rest explicitly, but just summarize them.
+	 * Examine children.
+	 *
+	 * If we are past the recursion depth limit or already running low on
+	 * stack, do not print them explicitly but just summarize them. Similarly,
+	 * if there are more than max_children of them, we do not print the rest
+	 * explicitly, but just summarize them.
 	 */
-	memset(&local_totals, 0, sizeof(local_totals));
-
-	for (child = context->firstchild, ichild = 0;
-		 child != NULL;
-		 child = child->nextchild, ichild++)
+	ichild = 0;
+	child = context->firstchild;
+	if (level < max_level && !stack_is_too_deep())
 	{
-		if (ichild < max_children)
+		for (; child != NULL && ichild < max_children;
+			 child = child->nextchild)
+		{
 			MemoryContextStatsInternal(child, level + 1,
-									   print, max_children,
+									   max_level, max_children,
 									   totals,
 									   print_to_stderr);
-		else
-			MemoryContextStatsInternal(child, level + 1,
-									   false, max_children,
-									   &local_totals,
-									   print_to_stderr);
+			ichild++;
+		}
 	}
 
-	/* Deal with excess children */
-	if (ichild > max_children)
+	if (child != NULL)
 	{
-		if (print)
+		/* Summarize the rest of the children, avoiding recursion. */
+		MemoryContextCounters local_totals;
+
+		memset(&local_totals, 0, sizeof(local_totals));
+
+		while (child != NULL)
 		{
-			if (print_to_stderr)
-			{
-				int			i;
-
-				for (i = 0; i <= level; i++)
-					fprintf(stderr, "  ");
-				fprintf(stderr,
-						"%d more child contexts containing %zu total in %zu blocks; %zu free (%zu chunks); %zu used\n",
-						ichild - max_children,
-						local_totals.totalspace,
-						local_totals.nblocks,
-						local_totals.freespace,
-						local_totals.freechunks,
-						local_totals.totalspace - local_totals.freespace);
-			}
-			else
-				ereport(LOG_SERVER_ONLY,
-						(errhidestmt(true),
-						 errhidecontext(true),
-						 errmsg_internal("level: %d; %d more child contexts containing %zu total in %zu blocks; %zu free (%zu chunks); %zu used",
-										 level,
-										 ichild - max_children,
-										 local_totals.totalspace,
-										 local_totals.nblocks,
-										 local_totals.freespace,
-										 local_totals.freechunks,
-										 local_totals.totalspace - local_totals.freespace)));
+			child->methods->stats(child, NULL, NULL, &local_totals, print_to_stderr);
+			child = MemoryContextTraverse(child, context);
+			ichild++;
 		}
 
+		if (print_to_stderr)
+		{
+			int			i;
+
+			for (i = 0; i <= level; i++)
+				fprintf(stderr, "  ");
+			fprintf(stderr,
+					"%d more child contexts containing %zu total in %zu blocks; %zu free (%zu chunks); %zu used\n",
+					ichild - max_children,
+					local_totals.totalspace,
+					local_totals.nblocks,
+					local_totals.freespace,
+					local_totals.freechunks,
+					local_totals.totalspace - local_totals.freespace);
+		}
+		else
+			ereport(LOG_SERVER_ONLY,
+					(errhidestmt(true),
+					 errhidecontext(true),
+					 errmsg_internal("level: %d; %d more child contexts containing %zu total in %zu blocks; %zu free (%zu chunks); %zu used",
+									 level,
+									 ichild - max_children,
+									 local_totals.totalspace,
+									 local_totals.nblocks,
+									 local_totals.freespace,
+									 local_totals.freechunks,
+									 local_totals.totalspace - local_totals.freespace)));
+
 		if (totals)
 		{
 			totals->nblocks += local_totals.nblocks;
@@ -847,8 +931,7 @@ MemoryContextStatsInternal(MemoryContext context, int level,
  */
 static void
 MemoryContextStatsPrint(MemoryContext context, void *passthru,
-						const char *stats_string,
-						bool print_to_stderr)
+						const char *stats_string, bool print_to_stderr)
 {
 	int			level = *(int *) passthru;
 	const char *name = context->name;
@@ -927,13 +1010,15 @@ MemoryContextStatsPrint(MemoryContext context, void *passthru,
 void
 MemoryContextCheck(MemoryContext context)
 {
-	MemoryContext child;
-
 	Assert(MemoryContextIsValid(context));
-
 	context->methods->check(context);
-	for (child = context->firstchild; child != NULL; child = child->nextchild)
-		MemoryContextCheck(child);
+
+	for (MemoryContext curr = context->firstchild;
+		 curr != NULL;
+		 curr = MemoryContextTraverse(curr, context))
+	{
+		curr->methods->check(curr);
+	}
 }
 #endif
 
@@ -1212,14 +1297,15 @@ ProcessLogMemoryContextInterrupt(void)
 	/*
 	 * When a backend process is consuming huge memory, logging all its memory
 	 * contexts might overrun available disk space. To prevent this, we limit
-	 * the number of child contexts to log per parent to 100.
+	 * the depth of the hierarchy, as well as the number of child contexts to
+	 * log per parent to 100.
 	 *
 	 * As with MemoryContextStats(), we suppose that practical cases where the
 	 * dump gets long will typically be huge numbers of siblings under the
 	 * same parent context; while the additional debugging value from seeing
 	 * details about individual siblings beyond 100 will not be large.
 	 */
-	MemoryContextStatsDetail(TopMemoryContext, 100, false);
+	MemoryContextStatsDetail(TopMemoryContext, 100, 100, false);
 }
 
 void *
diff --git a/src/include/utils/memutils.h b/src/include/utils/memutils.h
index d14e8546a6..c25f982dc6 100644
--- a/src/include/utils/memutils.h
+++ b/src/include/utils/memutils.h
@@ -85,7 +85,8 @@ extern MemoryContext MemoryContextGetParent(MemoryContext context);
 extern bool MemoryContextIsEmpty(MemoryContext context);
 extern Size MemoryContextMemAllocated(MemoryContext context, bool recurse);
 extern void MemoryContextStats(MemoryContext context);
-extern void MemoryContextStatsDetail(MemoryContext context, int max_children,
+extern void MemoryContextStatsDetail(MemoryContext context,
+									 int max_level, int max_children,
 									 bool print_to_stderr);
 extern void MemoryContextAllowInCriticalSection(MemoryContext context,
 												bool allow);
-- 
2.39.2

From 44568e6feef79d604bbe168c0839ab2e03c99f8a Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakangas@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

From 6a4d963540000877dc1cdaf586ca01121eb9db57 Mon Sep 17 00:00:00 2001
From: Alexander Korotkov <akorotkov@postgresql.org>
Date: Wed, 14 Feb 2024 13:38:44 +0200
Subject: [PATCH 2/2] Add missing check_stack_depth() to some recursive
 functions

Report by Egor Chindyaskin and Alexander Lakhin.
Discussion: https://postgr.es/m/1672760457.940462079%40f306.i.mail.ru
---
 src/backend/catalog/dependency.c      |  7 +++++++
 src/backend/catalog/heap.c            |  3 +++
 src/backend/commands/tablecmds.c      | 13 +++++++++++++
 src/backend/optimizer/util/clauses.c  |  4 ++++
 src/backend/utils/adt/jsonpath_exec.c |  3 +++
 5 files changed, 30 insertions(+)

diff --git a/src/backend/catalog/dependency.c b/src/backend/catalog/dependency.c
index e742c78ea35..df9886efc95 100644
--- a/src/backend/catalog/dependency.c
+++ b/src/backend/catalog/dependency.c
@@ -76,6 +76,7 @@
 #include "commands/trigger.h"
 #include "commands/typecmds.h"
 #include "funcapi.h"
+#include "miscadmin.h"
 #include "nodes/nodeFuncs.h"
 #include "parser/parsetree.h"
 #include "rewrite/rewriteRemove.h"
@@ -524,6 +525,12 @@ findDependentObjects(const ObjectAddress *object,
 	if (stack_address_present_add_flags(object, objflags, stack))
 		return;
 
+	/*
+	 * since this function recurses, it could be driven to stack overflow,
+	 * because of the deep dependency tree, not only due to dependency loops.
+	 */
+	check_stack_depth();
+
 	/*
 	 * It's also possible that the target object has already been completely
 	 * processed and put into targetObjects.  If so, again we just add the
diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index c73f7bcd011..252e106cada 100644
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -552,6 +552,9 @@ CheckAttributeType(const char *attname,
 	char		att_typtype = get_typtype(atttypid);
 	Oid			att_typelem;
 
+	/* since this function recurses, it could be driven to stack overflow */
+	check_stack_depth();
+
 	if (att_typtype == TYPTYPE_PSEUDO)
 	{
 		/*
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 86ea3a228b6..679dee10da4 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -7047,6 +7047,9 @@ ATExecAddColumn(List **wqueue, AlteredTableInfo *tab, Relation rel,
 	ObjectAddress address;
 	TupleDesc	tupdesc;
 
+	/* since this function recurses, it could be driven to stack overflow */
+	check_stack_depth();
+
 	/* At top level, permission check was done in ATPrepCmd, else do it */
 	if (recursing)
 		ATSimplePermissions((*cmd)->subtype, rel, ATT_TABLE | ATT_FOREIGN_TABLE);
@@ -9095,6 +9098,10 @@ ATExecDropColumn(List **wqueue, Relation rel, const char *colName,
 
 	/* Initialize addrs on the first invocation */
 	Assert(!recursing || addrs != NULL);
+
+	/* since this function recurses, it could be driven to stack overflow */
+	check_stack_depth();
+
 	if (!recursing)
 		addrs = new_object_addresses();
 
@@ -11648,6 +11655,9 @@ ATExecAlterConstrRecurse(Constraint *cmdcon, Relation conrel, Relation tgrel,
 	Oid			refrelid;
 	bool		changed = false;
 
+	/* since this function recurses, it could be driven to stack overflow */
+	check_stack_depth();
+
 	currcon = (Form_pg_constraint) GETSTRUCT(contuple);
 	conoid = currcon->oid;
 	refrelid = currcon->confrelid;
@@ -12728,6 +12738,9 @@ dropconstraint_internal(Relation rel, HeapTuple constraintTup, DropBehavior beha
 	/* Guard against stack overflow due to overly deep inheritance tree. */
 	check_stack_depth();
 
+	/* since this function recurses, it could be driven to stack overflow */
+	check_stack_depth();
+
 	/* At top level, permission check was done in ATPrepCmd, else do it */
 	if (recursing)
 		ATSimplePermissions(AT_DropConstraint, rel, ATT_TABLE | ATT_FOREIGN_TABLE);
diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c
index 94eb56a1e79..edc25d712e9 100644
--- a/src/backend/optimizer/util/clauses.c
+++ b/src/backend/optimizer/util/clauses.c
@@ -2423,6 +2423,10 @@ static Node *
 eval_const_expressions_mutator(Node *node,
 							   eval_const_expressions_context *context)
 {
+
+	/* since this function recurses, it could be driven to stack overflow */
+	check_stack_depth();
+
 	if (node == NULL)
 		return NULL;
 	switch (nodeTag(node))
diff --git a/src/backend/utils/adt/jsonpath_exec.c b/src/backend/utils/adt/jsonpath_exec.c
index 8372863de74..50fa724b728 100644
--- a/src/backend/utils/adt/jsonpath_exec.c
+++ b/src/backend/utils/adt/jsonpath_exec.c
@@ -1674,6 +1674,9 @@ executeBoolItem(JsonPathExecContext *cxt, JsonPathItem *jsp,
 	JsonPathBool res;
 	JsonPathBool res2;
 
+	/* since this function recurses, it could be driven to stack overflow */
+	check_stack_depth();
+
 	if (!canHaveNext && jspHasNext(jsp))
 		elog(ERROR, "boolean jsonpath item cannot have next item");
 
-- 
2.39.3 (Apple Git-145)

From 0d474daa36aa6dd83c64fb6ea4b0fc62a87030a6 Mon Sep 17 00:00:00 2001
From: Alexander Korotkov <akorotkov@postgresql.org>
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)

From d9c2a7e8eae2706ff1431ed090146e3a26ce07dc Mon Sep 17 00:00:00 2001
From: Alexander Korotkov <akorotkov@postgresql.org>
Date: Wed, 6 Mar 2024 13:59:41 +0200
Subject: [PATCH v4 2/3] Avoid stack overflow in ShowTransactionStateRec()

The function recurses, but didn't perform stack-depth checks. It's
just a debugging aid, so instead of the usual check_stack_depth()
call, stop the printing if we'd risk stack overflow.

Here's an example of how to test this:

    (n=1000000; printf "BEGIN;"; for ((i=1;i<=$n;i++)); do printf "SAVEPOINT s$i;"; done; printf "SET log_min_messages = 'DEBUG5'; SAVEPOINT sp;") | psql >/dev/null

In the passing, swap building the list of child XIDs and recursing to
parent. That saves memory while recursing, reducing the risk of out of
memory errors with lots of subtransactions. The saving is not very
significant in practice, but this order seems more logical anyway.

Report by Egor Chindyaskin and Alexander Lakhin.

Discussion: https://www.postgresql.org/message-id/1672760457.940462079%40f306.i.mail.ru
Author: Heikki Linnakangas
Reviewed-by: Robert Haas, Andres Freund, Alexander Korotkov
---
 src/backend/access/transam/xact.c | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index 12f8bec1aeb..2f80111e18d 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -5583,8 +5583,22 @@ ShowTransactionStateRec(const char *str, TransactionState s)
 {
 	StringInfoData buf;
 
-	initStringInfo(&buf);
+	if (s->parent)
+	{
+		/*
+		 * Since this function recurses, it could be driven to stack overflow.
+		 * This is just a debugging aid, so we can leave out some details
+		 * instead of erroring out with check_stack_depth().
+		 */
+		if (stack_is_too_deep())
+			ereport(DEBUG5,
+					(errmsg_internal("%s(%d): parent omitted to avoid stack overflow",
+									 str, s->nestingLevel)));
+		else
+			ShowTransactionStateRec(str, s->parent);
+	}
 
+	initStringInfo(&buf);
 	if (s->nChildXids > 0)
 	{
 		int			i;
@@ -5593,10 +5607,6 @@ ShowTransactionStateRec(const char *str, TransactionState s)
 		for (i = 1; i < s->nChildXids; i++)
 			appendStringInfo(&buf, " %u", s->childXids[i]);
 	}
-
-	if (s->parent)
-		ShowTransactionStateRec(str, s->parent);
-
 	ereport(DEBUG5,
 			(errmsg_internal("%s(%d) name: %s; blockState: %s; state: %s, xid/subid/cid: %u/%u/%u%s%s",
 							 str, s->nestingLevel,
@@ -5608,7 +5618,6 @@ ShowTransactionStateRec(const char *str, TransactionState s)
 							 (unsigned int) currentCommandId,
 							 currentCommandIdUsed ? " (used)" : "",
 							 buf.data)));
-
 	pfree(buf.data);
 }
 
-- 
2.39.3 (Apple Git-145)

From 693e9707be2a006c70a06052564c549c37acc07b Mon Sep 17 00:00:00 2001
From: Alexander Korotkov <akorotkov@postgresql.org>
Date: Wed, 6 Mar 2024 13:59:55 +0200
Subject: [PATCH v4 3/3] Avoid recursion in MemoryContext functions.

You might run out of stack space with recursion, which is not nice in
functions that might be used e.g. at cleanup after transaction
abort. MemoryContext contains pointer to parent and siblings, so we
can traverse a tree of contexts iteratively, without using
stack. Refactor the functions to do that.

MemoryContextStats() still recurses, but it now has a limit to how
deep it recurses. Once the limit is reached, it prints just a summary
of the rest of the hierarchy, similar to how it summarizes contexts
with lots of children. That seems good anyway, because a context dump
with hundreds of nested contexts isn't very readable.

Report by Egor Chindyaskin and Alexander Lakhin.

Discussion: https://www.postgresql.org/message-id/1672760457.940462079%40f306.i.mail.ru
Author: Heikki Linnakangas
Reviewed-by: Robert Haas, Andres Freund, Alexander Korotkov
---
 src/backend/utils/mmgr/mcxt.c | 246 +++++++++++++++++++++++-----------
 src/include/utils/memutils.h  |   3 +-
 2 files changed, 170 insertions(+), 79 deletions(-)

diff --git a/src/backend/utils/mmgr/mcxt.c b/src/backend/utils/mmgr/mcxt.c
index 1a615becae4..04b540a578e 100644
--- a/src/backend/utils/mmgr/mcxt.c
+++ b/src/backend/utils/mmgr/mcxt.c
@@ -145,9 +145,10 @@ MemoryContext CurTransactionContext = NULL;
 /* This is a transient link to the active portal's memory context: */
 MemoryContext PortalContext = NULL;
 
+static void MemoryContextDeleteOnly(MemoryContext context);
 static void MemoryContextCallResetCallbacks(MemoryContext context);
 static void MemoryContextStatsInternal(MemoryContext context, int level,
-									   bool print, int max_children,
+									   int max_level, int max_children,
 									   MemoryContextCounters *totals,
 									   bool print_to_stderr);
 static void MemoryContextStatsPrint(MemoryContext context, void *passthru,
@@ -256,6 +257,41 @@ BogusGetChunkSpace(void *pointer)
 	return 0;					/* keep compiler quiet */
 }
 
+/*
+ * MemoryContextTraverseNext
+ *		Helper function to traverse all descendants of a memory context
+ *		without recursion.
+ *
+ * Recursion could lead to out-of-stack errors with deep context hierarchies,
+ * which would be unpleasant in error cleanup code paths.
+ *
+ * To process 'context' and all its descendants, use a loop like this:
+ *
+ *     <process 'context'>
+ *     for (MemoryContext curr = context->firstchild;
+ *          curr != NULL;
+ *          curr = MemoryContextTraverseNext(curr, context))
+ *     {
+ *         <process 'curr'>
+ *     }
+ *
+ * This visits all the contexts in the depth-first order.
+ */
+static MemoryContext
+MemoryContextTraverseNext(MemoryContext curr, MemoryContext top)
+{
+	if (curr->firstchild != NULL)
+		return curr->firstchild;
+
+	while (curr->nextchild == NULL)
+	{
+		curr = curr->parent;
+		if (curr == top)
+			return NULL;
+	}
+	return curr->nextchild;
+}
+
 
 /*****************************************************************************
  *	  EXPORTED ROUTINES														 *
@@ -375,14 +411,13 @@ MemoryContextResetOnly(MemoryContext context)
 void
 MemoryContextResetChildren(MemoryContext context)
 {
-	MemoryContext child;
-
 	Assert(MemoryContextIsValid(context));
 
-	for (child = context->firstchild; child != NULL; child = child->nextchild)
+	for (MemoryContext curr = context->firstchild;
+		 curr != NULL;
+		 curr = MemoryContextTraverseNext(curr, context))
 	{
-		MemoryContextResetChildren(child);
-		MemoryContextResetOnly(child);
+		MemoryContextResetOnly(curr);
 	}
 }
 
@@ -397,16 +432,53 @@ MemoryContextResetChildren(MemoryContext context)
  */
 void
 MemoryContextDelete(MemoryContext context)
+{
+	MemoryContext curr;
+
+	/*
+	 * Delete subcontexts from the bottom up.
+	 *
+	 * Note: Do not use recursion here.  A "stack depth limit exceeded" error
+	 * would be unpleasant if we're already in the process of cleaning up from
+	 * transaction abort.  We also cannot use MemoryContextTraverseNext() here
+	 * because we modify the tree as we go.
+	 */
+	curr = context;
+	for (;;)
+	{
+		MemoryContext parent;
+
+		/* Descend down until we find a leaf context with no children */
+		while (curr->firstchild != NULL)
+			curr = curr->firstchild;
+
+		/*
+		 * We're now at a leaf with no children. Free it and continue from the
+		 * parent.  Or if this was the original node, we're all done.
+		 */
+		parent = curr->parent;
+		MemoryContextDeleteOnly(curr);
+
+		if (curr == context)
+			break;
+		curr = parent;
+	}
+}
+
+/*
+ * Subroutine of MemoryContextDelete, to delete a context that has no
+ * children.
+ */
+static void
+MemoryContextDeleteOnly(MemoryContext context)
 {
 	Assert(MemoryContextIsValid(context));
 	/* We had better not be deleting TopMemoryContext ... */
 	Assert(context != TopMemoryContext);
 	/* And not CurrentMemoryContext, either */
 	Assert(context != CurrentMemoryContext);
-
-	/* save a function call in common case where there are no children */
-	if (context->firstchild != NULL)
-		MemoryContextDeleteChildren(context);
+	/* All the children should've been deleted already */
+	Assert(context->firstchild == NULL);
 
 	/*
 	 * It's not entirely clear whether 'tis better to do this before or after
@@ -672,12 +744,12 @@ MemoryContextMemAllocated(MemoryContext context, bool recurse)
 
 	if (recurse)
 	{
-		MemoryContext child;
-
-		for (child = context->firstchild;
-			 child != NULL;
-			 child = child->nextchild)
-			total += MemoryContextMemAllocated(child, true);
+		for (MemoryContext curr = context->firstchild;
+			 curr != NULL;
+			 curr = MemoryContextTraverseNext(curr, context))
+		{
+			total += curr->mem_allocated;
+		}
 	}
 
 	return total;
@@ -691,9 +763,15 @@ void
 MemoryContextMemConsumed(MemoryContext context,
 						 MemoryContextCounters *consumed)
 {
+
 	memset(consumed, 0, sizeof(*consumed));
 
-	MemoryContextStatsInternal(context, 0, false, 0, consumed, false);
+	for (MemoryContext curr = context->firstchild;
+		 curr != NULL;
+		 curr = MemoryContextTraverseNext(curr, context))
+	{
+		child->methods->stats(child, NULL, NULL, consumed, print_to_stderr);
+	}
 }
 
 /*
@@ -707,8 +785,8 @@ MemoryContextMemConsumed(MemoryContext context,
 void
 MemoryContextStats(MemoryContext context)
 {
-	/* A hard-wired limit on the number of children is usually good enough */
-	MemoryContextStatsDetail(context, 100, true);
+	/* Hard-wired limits are usually good enough */
+	MemoryContextStatsDetail(context, 100, 100, true);
 }
 
 /*
@@ -720,14 +798,15 @@ MemoryContextStats(MemoryContext context)
  * with fprintf(stderr), otherwise use ereport().
  */
 void
-MemoryContextStatsDetail(MemoryContext context, int max_children,
+MemoryContextStatsDetail(MemoryContext context,
+						 int max_level, int max_children,
 						 bool print_to_stderr)
 {
 	MemoryContextCounters grand_totals;
 
 	memset(&grand_totals, 0, sizeof(grand_totals));
 
-	MemoryContextStatsInternal(context, 0, true, max_children, &grand_totals, print_to_stderr);
+	MemoryContextStatsInternal(context, 0, max_level, max_children, &grand_totals, print_to_stderr);
 
 	if (print_to_stderr)
 		fprintf(stderr,
@@ -765,11 +844,10 @@ MemoryContextStatsDetail(MemoryContext context, int max_children,
  */
 static void
 MemoryContextStatsInternal(MemoryContext context, int level,
-						   bool print, int max_children,
+						   int max_level, int max_children,
 						   MemoryContextCounters *totals,
 						   bool print_to_stderr)
 {
-	MemoryContextCounters local_totals;
 	MemoryContext child;
 	int			ichild;
 
@@ -777,65 +855,75 @@ MemoryContextStatsInternal(MemoryContext context, int level,
 
 	/* Examine the context itself */
 	context->methods->stats(context,
-							print ? MemoryContextStatsPrint : NULL,
+							MemoryContextStatsPrint,
 							(void *) &level,
-							totals, print_to_stderr);
+							totals,
+							print_to_stderr);
 
 	/*
-	 * Examine children.  If there are more than max_children of them, we do
-	 * not print the rest explicitly, but just summarize them.
+	 * Examine children.
+	 *
+	 * If we are past the recursion depth limit or already running low on
+	 * stack, do not print them explicitly but just summarize them. Similarly,
+	 * if there are more than max_children of them, we do not print the rest
+	 * explicitly, but just summarize them.
 	 */
-	memset(&local_totals, 0, sizeof(local_totals));
-
-	for (child = context->firstchild, ichild = 0;
-		 child != NULL;
-		 child = child->nextchild, ichild++)
+	ichild = 0;
+	child = context->firstchild;
+	if (level < max_level && !stack_is_too_deep())
 	{
-		if (ichild < max_children)
+		for (; child != NULL && ichild < max_children;
+			 child = child->nextchild)
+		{
 			MemoryContextStatsInternal(child, level + 1,
-									   print, max_children,
+									   max_level, max_children,
 									   totals,
 									   print_to_stderr);
-		else
-			MemoryContextStatsInternal(child, level + 1,
-									   false, max_children,
-									   &local_totals,
-									   print_to_stderr);
+			ichild++;
+		}
 	}
 
-	/* Deal with excess children */
-	if (ichild > max_children)
+	if (child != NULL)
 	{
-		if (print)
+		/* Summarize the rest of the children, avoiding recursion. */
+		MemoryContextCounters local_totals;
+
+		memset(&local_totals, 0, sizeof(local_totals));
+
+		while (child != NULL)
+		{
+			child->methods->stats(child, NULL, NULL, &local_totals, print_to_stderr);
+			child = MemoryContextTraverseNext(child, context);
+			ichild++;
+		}
+
+		if (print_to_stderr)
 		{
-			if (print_to_stderr)
-			{
-				int			i;
-
-				for (i = 0; i <= level; i++)
-					fprintf(stderr, "  ");
-				fprintf(stderr,
-						"%d more child contexts containing %zu total in %zu blocks; %zu free (%zu chunks); %zu used\n",
-						ichild - max_children,
-						local_totals.totalspace,
-						local_totals.nblocks,
-						local_totals.freespace,
-						local_totals.freechunks,
-						local_totals.totalspace - local_totals.freespace);
-			}
-			else
-				ereport(LOG_SERVER_ONLY,
-						(errhidestmt(true),
-						 errhidecontext(true),
-						 errmsg_internal("level: %d; %d more child contexts containing %zu total in %zu blocks; %zu free (%zu chunks); %zu used",
-										 level,
-										 ichild - max_children,
-										 local_totals.totalspace,
-										 local_totals.nblocks,
-										 local_totals.freespace,
-										 local_totals.freechunks,
-										 local_totals.totalspace - local_totals.freespace)));
+			int			i;
+
+			for (i = 0; i <= level; i++)
+				fprintf(stderr, "  ");
+			fprintf(stderr,
+					"%d more child contexts containing %zu total in %zu blocks; %zu free (%zu chunks); %zu used\n",
+					ichild - max_children,
+					local_totals.totalspace,
+					local_totals.nblocks,
+					local_totals.freespace,
+					local_totals.freechunks,
+					local_totals.totalspace - local_totals.freespace);
 		}
+		else
+			ereport(LOG_SERVER_ONLY,
+					(errhidestmt(true),
+					 errhidecontext(true),
+					 errmsg_internal("level: %d; %d more child contexts containing %zu total in %zu blocks; %zu free (%zu chunks); %zu used",
+									 level,
+									 ichild - max_children,
+									 local_totals.totalspace,
+									 local_totals.nblocks,
+									 local_totals.freespace,
+									 local_totals.freechunks,
+									 local_totals.totalspace - local_totals.freespace)));
 
 		if (totals)
 		{
@@ -856,8 +944,7 @@ MemoryContextStatsInternal(MemoryContext context, int level,
  */
 static void
 MemoryContextStatsPrint(MemoryContext context, void *passthru,
-						const char *stats_string,
-						bool print_to_stderr)
+						const char *stats_string, bool print_to_stderr)
 {
 	int			level = *(int *) passthru;
 	const char *name = context->name;
@@ -936,13 +1023,15 @@ MemoryContextStatsPrint(MemoryContext context, void *passthru,
 void
 MemoryContextCheck(MemoryContext context)
 {
-	MemoryContext child;
-
 	Assert(MemoryContextIsValid(context));
-
 	context->methods->check(context);
-	for (child = context->firstchild; child != NULL; child = child->nextchild)
-		MemoryContextCheck(child);
+
+	for (MemoryContext curr = context->firstchild;
+		 curr != NULL;
+		 curr = MemoryContextTraverseNext(curr, context))
+	{
+		curr->methods->check(curr);
+	}
 }
 #endif
 
@@ -1183,14 +1272,15 @@ ProcessLogMemoryContextInterrupt(void)
 	/*
 	 * When a backend process is consuming huge memory, logging all its memory
 	 * contexts might overrun available disk space. To prevent this, we limit
-	 * the number of child contexts to log per parent to 100.
+	 * the depth of the hierarchy, as well as the number of child contexts to
+	 * log per parent to 100.
 	 *
 	 * As with MemoryContextStats(), we suppose that practical cases where the
 	 * dump gets long will typically be huge numbers of siblings under the
 	 * same parent context; while the additional debugging value from seeing
 	 * details about individual siblings beyond 100 will not be large.
 	 */
-	MemoryContextStatsDetail(TopMemoryContext, 100, false);
+	MemoryContextStatsDetail(TopMemoryContext, 100, 100, false);
 }
 
 void *
diff --git a/src/include/utils/memutils.h b/src/include/utils/memutils.h
index 7fd41d20caf..6e5fa72b0e1 100644
--- a/src/include/utils/memutils.h
+++ b/src/include/utils/memutils.h
@@ -87,7 +87,8 @@ extern Size MemoryContextMemAllocated(MemoryContext context, bool recurse);
 extern void MemoryContextMemConsumed(MemoryContext context,
 									 MemoryContextCounters *consumed);
 extern void MemoryContextStats(MemoryContext context);
-extern void MemoryContextStatsDetail(MemoryContext context, int max_children,
+extern void MemoryContextStatsDetail(MemoryContext context,
+									 int max_level, int max_children,
 									 bool print_to_stderr);
 extern void MemoryContextAllowInCriticalSection(MemoryContext context,
 												bool allow);
-- 
2.39.3 (Apple Git-145)

From cc905afa528ab5a98814167dc28ecf976dc7f52f Mon Sep 17 00:00:00 2001
From: Alexander Korotkov <akorotkov@postgresql.org>
Date: Wed, 14 Feb 2024 13:16:41 +0200
Subject: [PATCH v4 1/3] 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 ccd3f4fc550..12f8bec1aeb 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;
 	}
 }
-- 
2.39.3 (Apple Git-145)

diff --git a/src/backend/utils/mmgr/mcxt.c b/src/backend/utils/mmgr/mcxt.c
index 1a615becae..5d426795d9 100644
--- a/src/backend/utils/mmgr/mcxt.c
+++ b/src/backend/utils/mmgr/mcxt.c
@@ -145,9 +145,10 @@ MemoryContext CurTransactionContext = NULL;
 /* This is a transient link to the active portal's memory context: */
 MemoryContext PortalContext = NULL;
 
+static void MemoryContextDeleteOnly(MemoryContext context);
 static void MemoryContextCallResetCallbacks(MemoryContext context);
 static void MemoryContextStatsInternal(MemoryContext context, int level,
-									   bool print, int max_children,
+									   int max_level, int max_children,
 									   MemoryContextCounters *totals,
 									   bool print_to_stderr);
 static void MemoryContextStatsPrint(MemoryContext context, void *passthru,
@@ -219,6 +220,50 @@ GetMemoryChunkHeader(const void *pointer)
 	return header;
 }
 
+/*
+ * MemoryContextTraverseNext
+ *		Helper function to traverse all descendants of a memory context
+ *		without recursion.
+ *
+ * Recursion could lead to out-of-stack errors with deep context hierarchies,
+ * which would be unpleasant in error cleanup code paths.
+ *
+ * To process 'context' and all its descendants, use a loop like this:
+ *
+ *     <process 'context'>
+ *     for (MemoryContext curr = context->firstchild;
+ *          curr != NULL;
+ *          curr = MemoryContextTraverseNext(curr, context))
+ *     {
+ *         <process 'curr'>
+ *     }
+ *
+ * This visits all the contexts in pre-order, that is a node is visited
+ * before its children.
+ */
+static MemoryContext
+MemoryContextTraverseNext(MemoryContext curr, MemoryContext top)
+{
+	/* After processing a node, traverse to its first child if any */
+	if (curr->firstchild != NULL)
+		return curr->firstchild;
+
+	/*
+	 * After processing a childless node, traverse to its next sibling if
+	 * there is one.  If there isn't, traverse back up to the parent (which
+	 * has already been visited, and now so have all its descendants).  We're
+	 * done if that is "top", otherwise traverse to its next sibling if any,
+	 * otherwise repeat moving up.
+	 */
+	while (curr->nextchild == NULL)
+	{
+		curr = curr->parent;
+		if (curr == top)
+			return NULL;
+	}
+	return curr->nextchild;
+}
+
 /*
  * Support routines to trap use of invalid memory context method IDs
  * (from calling pfree or the like on a bogus pointer).  As a possible
@@ -375,14 +420,13 @@ MemoryContextResetOnly(MemoryContext context)
 void
 MemoryContextResetChildren(MemoryContext context)
 {
-	MemoryContext child;
-
 	Assert(MemoryContextIsValid(context));
 
-	for (child = context->firstchild; child != NULL; child = child->nextchild)
+	for (MemoryContext curr = context->firstchild;
+		 curr != NULL;
+		 curr = MemoryContextTraverseNext(curr, context))
 	{
-		MemoryContextResetChildren(child);
-		MemoryContextResetOnly(child);
+		MemoryContextResetOnly(curr);
 	}
 }
 
@@ -392,21 +436,60 @@ MemoryContextResetChildren(MemoryContext context)
  *		allocated therein.
  *
  * The type-specific delete routine removes all storage for the context,
- * but we have to recurse to handle the children.
- * We must also delink the context from its parent, if it has one.
+ * but we have to deal with descendant nodes here.
  */
 void
 MemoryContextDelete(MemoryContext context)
+{
+	MemoryContext curr;
+
+	Assert(MemoryContextIsValid(context));
+
+	/*
+	 * Delete subcontexts from the bottom up.
+	 *
+	 * Note: Do not use recursion here.  A "stack depth limit exceeded" error
+	 * would be unpleasant if we're already in the process of cleaning up from
+	 * transaction abort.  We also cannot use MemoryContextTraverseNext() here
+	 * because we modify the tree as we go.
+	 */
+	curr = context;
+	for (;;)
+	{
+		MemoryContext parent;
+
+		/* Descend down until we find a leaf context with no children */
+		while (curr->firstchild != NULL)
+			curr = curr->firstchild;
+
+		/*
+		 * We're now at a leaf with no children. Free it and continue from the
+		 * parent.  Or if this was the original node, we're all done.
+		 */
+		parent = curr->parent;
+		MemoryContextDeleteOnly(curr);
+
+		if (curr == context)
+			break;
+		curr = parent;
+	}
+}
+
+/*
+ * Subroutine of MemoryContextDelete,
+ * to delete a context that has no children.
+ * We must also delink the context from its parent, if it has one.
+ */
+static void
+MemoryContextDeleteOnly(MemoryContext context)
 {
 	Assert(MemoryContextIsValid(context));
 	/* We had better not be deleting TopMemoryContext ... */
 	Assert(context != TopMemoryContext);
 	/* And not CurrentMemoryContext, either */
 	Assert(context != CurrentMemoryContext);
-
-	/* save a function call in common case where there are no children */
-	if (context->firstchild != NULL)
-		MemoryContextDeleteChildren(context);
+	/* All the children should've been deleted already */
+	Assert(context->firstchild == NULL);
 
 	/*
 	 * It's not entirely clear whether 'tis better to do this before or after
@@ -672,12 +755,12 @@ MemoryContextMemAllocated(MemoryContext context, bool recurse)
 
 	if (recurse)
 	{
-		MemoryContext child;
-
-		for (child = context->firstchild;
-			 child != NULL;
-			 child = child->nextchild)
-			total += MemoryContextMemAllocated(child, true);
+		for (MemoryContext curr = context->firstchild;
+			 curr != NULL;
+			 curr = MemoryContextTraverseNext(curr, context))
+		{
+			total += curr->mem_allocated;
+		}
 	}
 
 	return total;
@@ -691,9 +774,20 @@ void
 MemoryContextMemConsumed(MemoryContext context,
 						 MemoryContextCounters *consumed)
 {
+	Assert(MemoryContextIsValid(context));
+
 	memset(consumed, 0, sizeof(*consumed));
 
-	MemoryContextStatsInternal(context, 0, false, 0, consumed, false);
+	/* Examine the context itself */
+	context->methods->stats(context, NULL, NULL, consumed, false);
+
+	/* Examine children, using iteration not recursion */
+	for (MemoryContext curr = context->firstchild;
+		 curr != NULL;
+		 curr = MemoryContextTraverseNext(curr, context))
+	{
+		curr->methods->stats(curr, NULL, NULL, consumed, false);
+	}
 }
 
 /*
@@ -707,8 +801,8 @@ MemoryContextMemConsumed(MemoryContext context,
 void
 MemoryContextStats(MemoryContext context)
 {
-	/* A hard-wired limit on the number of children is usually good enough */
-	MemoryContextStatsDetail(context, 100, true);
+	/* Hard-wired limits are usually good enough */
+	MemoryContextStatsDetail(context, 100, 100, true);
 }
 
 /*
@@ -720,14 +814,16 @@ MemoryContextStats(MemoryContext context)
  * with fprintf(stderr), otherwise use ereport().
  */
 void
-MemoryContextStatsDetail(MemoryContext context, int max_children,
+MemoryContextStatsDetail(MemoryContext context,
+						 int max_level, int max_children,
 						 bool print_to_stderr)
 {
 	MemoryContextCounters grand_totals;
 
 	memset(&grand_totals, 0, sizeof(grand_totals));
 
-	MemoryContextStatsInternal(context, 0, true, max_children, &grand_totals, print_to_stderr);
+	MemoryContextStatsInternal(context, 0, max_level, max_children,
+							   &grand_totals, print_to_stderr);
 
 	if (print_to_stderr)
 		fprintf(stderr,
@@ -736,7 +832,7 @@ MemoryContextStatsDetail(MemoryContext context, int max_children,
 				grand_totals.freespace, grand_totals.freechunks,
 				grand_totals.totalspace - grand_totals.freespace);
 	else
-
+	{
 		/*
 		 * Use LOG_SERVER_ONLY to prevent the memory contexts from being sent
 		 * to the connected client.
@@ -754,22 +850,22 @@ MemoryContextStatsDetail(MemoryContext context, int max_children,
 								 grand_totals.totalspace, grand_totals.nblocks,
 								 grand_totals.freespace, grand_totals.freechunks,
 								 grand_totals.totalspace - grand_totals.freespace)));
+	}
 }
 
 /*
  * MemoryContextStatsInternal
  *		One recursion level for MemoryContextStats
  *
- * Print this context if print is true, but in any case accumulate counts into
- * *totals (if given).
+ * Print stats for this context if possible, but in any case accumulate counts
+ * into *totals (if not NULL).
  */
 static void
 MemoryContextStatsInternal(MemoryContext context, int level,
-						   bool print, int max_children,
+						   int max_level, int max_children,
 						   MemoryContextCounters *totals,
 						   bool print_to_stderr)
 {
-	MemoryContextCounters local_totals;
 	MemoryContext child;
 	int			ichild;
 
@@ -777,65 +873,72 @@ MemoryContextStatsInternal(MemoryContext context, int level,
 
 	/* Examine the context itself */
 	context->methods->stats(context,
-							print ? MemoryContextStatsPrint : NULL,
+							MemoryContextStatsPrint,
 							(void *) &level,
 							totals, print_to_stderr);
 
 	/*
-	 * Examine children.  If there are more than max_children of them, we do
-	 * not print the rest explicitly, but just summarize them.
+	 * Examine children.
+	 *
+	 * If we are past the recursion depth limit or already running low on
+	 * stack, do not print them explicitly but just summarize them. Similarly,
+	 * if there are more than max_children of them, we do not print the rest
+	 * explicitly, but just summarize them.
 	 */
-	memset(&local_totals, 0, sizeof(local_totals));
-
-	for (child = context->firstchild, ichild = 0;
-		 child != NULL;
-		 child = child->nextchild, ichild++)
+	child = context->firstchild;
+	ichild = 0;
+	if (level < max_level && !stack_is_too_deep())
 	{
-		if (ichild < max_children)
+		for (; child != NULL && ichild < max_children;
+			 child = child->nextchild, ichild++)
+		{
 			MemoryContextStatsInternal(child, level + 1,
-									   print, max_children,
+									   max_level, max_children,
 									   totals,
 									   print_to_stderr);
-		else
-			MemoryContextStatsInternal(child, level + 1,
-									   false, max_children,
-									   &local_totals,
-									   print_to_stderr);
+		}
 	}
 
-	/* Deal with excess children */
-	if (ichild > max_children)
+	if (child != NULL)
 	{
-		if (print)
+		/* Summarize the rest of the children, avoiding recursion. */
+		MemoryContextCounters local_totals;
+
+		memset(&local_totals, 0, sizeof(local_totals));
+
+		ichild = 0;
+		while (child != NULL)
+		{
+			child->methods->stats(child, NULL, NULL, &local_totals, false);
+			ichild++;
+			child = MemoryContextTraverseNext(child, context);
+		}
+
+		if (print_to_stderr)
 		{
-			if (print_to_stderr)
-			{
-				int			i;
-
-				for (i = 0; i <= level; i++)
-					fprintf(stderr, "  ");
-				fprintf(stderr,
-						"%d more child contexts containing %zu total in %zu blocks; %zu free (%zu chunks); %zu used\n",
-						ichild - max_children,
-						local_totals.totalspace,
-						local_totals.nblocks,
-						local_totals.freespace,
-						local_totals.freechunks,
-						local_totals.totalspace - local_totals.freespace);
-			}
-			else
-				ereport(LOG_SERVER_ONLY,
-						(errhidestmt(true),
-						 errhidecontext(true),
-						 errmsg_internal("level: %d; %d more child contexts containing %zu total in %zu blocks; %zu free (%zu chunks); %zu used",
-										 level,
-										 ichild - max_children,
-										 local_totals.totalspace,
-										 local_totals.nblocks,
-										 local_totals.freespace,
-										 local_totals.freechunks,
-										 local_totals.totalspace - local_totals.freespace)));
+			for (int i = 0; i <= level; i++)
+				fprintf(stderr, "  ");
+			fprintf(stderr,
+					"%d more child contexts containing %zu total in %zu blocks; %zu free (%zu chunks); %zu used\n",
+					ichild,
+					local_totals.totalspace,
+					local_totals.nblocks,
+					local_totals.freespace,
+					local_totals.freechunks,
+					local_totals.totalspace - local_totals.freespace);
 		}
+		else
+			ereport(LOG_SERVER_ONLY,
+					(errhidestmt(true),
+					 errhidecontext(true),
+					 errmsg_internal("level: %d; %d more child contexts containing %zu total in %zu blocks; %zu free (%zu chunks); %zu used",
+									 level,
+									 ichild,
+									 local_totals.totalspace,
+									 local_totals.nblocks,
+									 local_totals.freespace,
+									 local_totals.freechunks,
+									 local_totals.totalspace - local_totals.freespace)));
 
 		if (totals)
 		{
@@ -928,7 +1031,7 @@ MemoryContextStatsPrint(MemoryContext context, void *passthru,
 
 /*
  * MemoryContextCheck
- *		Check all chunks in the named context.
+ *		Check all chunks in the named context and its children.
  *
  * This is just a debugging utility, so it's not fancy.
  */
@@ -936,13 +1039,16 @@ MemoryContextStatsPrint(MemoryContext context, void *passthru,
 void
 MemoryContextCheck(MemoryContext context)
 {
-	MemoryContext child;
-
 	Assert(MemoryContextIsValid(context));
-
 	context->methods->check(context);
-	for (child = context->firstchild; child != NULL; child = child->nextchild)
-		MemoryContextCheck(child);
+
+	for (MemoryContext curr = context->firstchild;
+		 curr != NULL;
+		 curr = MemoryContextTraverseNext(curr, context))
+	{
+		Assert(MemoryContextIsValid(curr));
+		curr->methods->check(curr);
+	}
 }
 #endif
 
@@ -1183,14 +1289,15 @@ ProcessLogMemoryContextInterrupt(void)
 	/*
 	 * When a backend process is consuming huge memory, logging all its memory
 	 * contexts might overrun available disk space. To prevent this, we limit
-	 * the number of child contexts to log per parent to 100.
+	 * the depth of the hierarchy, as well as the number of child contexts to
+	 * log per parent to 100.
 	 *
 	 * As with MemoryContextStats(), we suppose that practical cases where the
 	 * dump gets long will typically be huge numbers of siblings under the
 	 * same parent context; while the additional debugging value from seeing
 	 * details about individual siblings beyond 100 will not be large.
 	 */
-	MemoryContextStatsDetail(TopMemoryContext, 100, false);
+	MemoryContextStatsDetail(TopMemoryContext, 100, 100, false);
 }
 
 void *
diff --git a/src/include/utils/memutils.h b/src/include/utils/memutils.h
index 7fd41d20ca..6e5fa72b0e 100644
--- a/src/include/utils/memutils.h
+++ b/src/include/utils/memutils.h
@@ -87,7 +87,8 @@ extern Size MemoryContextMemAllocated(MemoryContext context, bool recurse);
 extern void MemoryContextMemConsumed(MemoryContext context,
 									 MemoryContextCounters *consumed);
 extern void MemoryContextStats(MemoryContext context);
-extern void MemoryContextStatsDetail(MemoryContext context, int max_children,
+extern void MemoryContextStatsDetail(MemoryContext context,
+									 int max_level, int max_children,
 									 bool print_to_stderr);
 extern void MemoryContextAllowInCriticalSection(MemoryContext context,
 												bool allow);

diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index df5a67e4c31..8870d1f961e 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -346,8 +346,8 @@ static void CommitTransaction(void);
 static TransactionId RecordTransactionAbort(bool isSubXact);
 static void StartTransaction(void);
 
-static void CommitTransactionCommandInternal(void);
-static void AbortCurrentTransactionInternal(void);
+static bool CommitTransactionCommandInternal(void);
+static bool AbortCurrentTransactionInternal(void);
 
 static void StartSubTransaction(void);
 static void CommitSubTransaction(void);
@@ -3092,33 +3092,7 @@ RestoreTransactionCharacteristics(const SavedTransactionCharacteristics *s)
 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;
-	}
+	while (!CommitTransactionCommandInternal());
 }
 
 /*
@@ -3126,16 +3100,12 @@ CommitTransactionCommand(void)
  *		regarding handling the commit transaction command except for loop over
  *		subtransactions.
  */
-static void
+static bool
 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);
 
@@ -3376,10 +3346,31 @@ CommitTransactionCommandInternal(void)
 				s->blockState = TBLOCK_SUBINPROGRESS;
 			}
 			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();
+			return false;
+
+			/*
+			 * As above, but it's not dead yet, so abort first.
+				*/
+		case TBLOCK_SUBABORT_PENDING:
+			AbortSubTransaction();
+			CleanupSubTransaction();
+			return false;
+
 		default:
 			/* Keep compiler quiet */
 			break;
 	}
+
+	return true;
 }
 
 /*
@@ -3390,38 +3381,7 @@ CommitTransactionCommandInternal(void)
 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;
-	}
+	while (!AbortCurrentTransactionInternal());
 }
 
 /*
@@ -3429,20 +3389,11 @@ AbortCurrentTransaction(void)
  *		regarding handling the abort transaction command except for loop over
  *		subtransactions.
  */
-static void
+static bool
 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:
@@ -3563,10 +3514,36 @@ AbortCurrentTransactionInternal(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();
+			return false;
+
+			/*
+			 * Same as above, except the Abort() was already done.
+			 */
+		case TBLOCK_SUBABORT_END:
+		case TBLOCK_SUBABORT_RESTART:
+			CleanupSubTransaction();
+			return false;
+
 		default:
 			/* Keep compiler quiet */
 			break;
 	}
+
+	return true;
 }
 
 /*

From a3d8d3e2cff9eaf7ec7bbfe6b84f1171d1b9a811 Mon Sep 17 00:00:00 2001
From: Alexander Korotkov <akorotkov@postgresql.org>
Date: Wed, 17 Apr 2024 14:27:33 +0300
Subject: [PATCH v2] Refactoring for
 CommitTransactionCommand()/AbortCurrentTransaction()

fefd9a3fed turned tail recursion of CommitTransactionCommand() and
AbortCurrentTransaction() into iteration.  However, it splits the handling of
cases between different functions.

This commit puts the handling of all the cases into
AbortCurrentTransactionInternal() and CommitTransactionCommandInternal().
Now CommitTransactionCommand() and AbortCurrentTransaction() are just doing
the repeated calls of internal functions.

Reported-by: Andres Freund
Discussion: https://postgr.es/m/20240415224834.w6piwtefskoh32mv%40awork3.anarazel.de
Author: Andres Freund
---
 src/backend/access/transam/xact.c | 159 +++++++++++++-----------------
 1 file changed, 71 insertions(+), 88 deletions(-)

diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index df5a67e4c31..d82d083cf43 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -346,8 +346,8 @@ static void CommitTransaction(void);
 static TransactionId RecordTransactionAbort(bool isSubXact);
 static void StartTransaction(void);
 
-static void CommitTransactionCommandInternal(void);
-static void AbortCurrentTransactionInternal(void);
+static bool CommitTransactionCommandInternal(void);
+static bool AbortCurrentTransactionInternal(void);
 
 static void StartSubTransaction(void);
 static void CommitSubTransaction(void);
@@ -3092,50 +3092,25 @@ RestoreTransactionCharacteristics(const SavedTransactionCharacteristics *s)
 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;
-	}
+	/*
+	 * Repeatedly call CommitTransactionCommandInternal() untill all the work
+	 * is done.
+	 */
+	while (!CommitTransactionCommandInternal());
 }
 
 /*
- *	CommitTransactionCommandInternal - a function doing all the material work
- *		regarding handling the commit transaction command except for loop over
- *		subtransactions.
+ *	CommitTransactionCommandInternal - a function doing an iteration of work
+ *		regarding handling the commit transaction command.  In the case of
+ *		subtransactions more than one iterations could be required.  Returns
+ *		true when no more iterations required, false otherwise.
  */
-static void
+static bool
 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);
 
@@ -3319,6 +3294,25 @@ CommitTransactionCommandInternal(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).
+			 * As we need to examine the parent, return false to request the
+			 * caller to do the next iteration.
+			 */
+		case TBLOCK_SUBABORT_END:
+			CleanupSubTransaction();
+			return false;
+
+			/*
+			 * As above, but it's not dead yet, so abort first.
+			 */
+		case TBLOCK_SUBABORT_PENDING:
+			AbortSubTransaction();
+			CleanupSubTransaction();
+			return false;
+
 			/*
 			 * The current subtransaction is the target of a ROLLBACK TO
 			 * command.  Abort and pop it, then start a new subtransaction
@@ -3376,10 +3370,10 @@ CommitTransactionCommandInternal(void)
 				s->blockState = TBLOCK_SUBINPROGRESS;
 			}
 			break;
-		default:
-			/* Keep compiler quiet */
-			break;
 	}
+
+	/* Done, no more iterations required */
+	return true;
 }
 
 /*
@@ -3390,59 +3384,24 @@ CommitTransactionCommandInternal(void)
 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;
-	}
+	/*
+	 * Repeatedly call AbortCurrentTransactionInternal() untill all the work
+	 * is done.
+	 */
+	while (!AbortCurrentTransactionInternal());
 }
 
 /*
- *	AbortCurrentTransactionInternal - a function doing all the material work
- *		regarding handling the abort transaction command except for loop over
- *		subtransactions.
+ *	AbortCurrentTransactionInternal - a function doing an iteration of work
+ *		regarding handling the current transaction abort.  In the case of
+ *		subtransactions more than one iterations could be required.  Returns
+ *		true when no more iterations required, false otherwise.
  */
-static void
+static bool
 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:
@@ -3563,10 +3522,34 @@ AbortCurrentTransactionInternal(void)
 			AbortSubTransaction();
 			s->blockState = TBLOCK_SUBABORT;
 			break;
-		default:
-			/* Keep compiler quiet */
-			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.  As
+			 * we need to abort the parent, return false to request the caller
+			 * to do the next iteration.
+			 */
+		case TBLOCK_SUBBEGIN:
+		case TBLOCK_SUBRELEASE:
+		case TBLOCK_SUBCOMMIT:
+		case TBLOCK_SUBABORT_PENDING:
+		case TBLOCK_SUBRESTART:
+			AbortSubTransaction();
+			CleanupSubTransaction();
+			return false;
+
+			/*
+			 * Same as above, except the Abort() was already done.
+			 */
+		case TBLOCK_SUBABORT_END:
+		case TBLOCK_SUBABORT_RESTART:
+			CleanupSubTransaction();
+			return false;
 	}
+
+	/* Done, no more iterations required */
+	return true;
 }
 
 /*
-- 
2.39.3 (Apple Git-145)

From 846c833033f06766f725a63e8121579430de1dd2 Mon Sep 17 00:00:00 2001
From: Alexander Korotkov <akorotkov@postgresql.org>
Date: Wed, 17 Apr 2024 14:27:33 +0300
Subject: [PATCH v3] Refactoring for
 CommitTransactionCommand()/AbortCurrentTransaction()

fefd9a3fed turned tail recursion of CommitTransactionCommand() and
AbortCurrentTransaction() into iteration.  However, it splits the handling of
cases between different functions.

This commit puts the handling of all the cases into
AbortCurrentTransactionInternal() and CommitTransactionCommandInternal().
Now CommitTransactionCommand() and AbortCurrentTransaction() are just doing
the repeated calls of internal functions.

Reported-by: Andres Freund
Discussion: https://postgr.es/m/20240415224834.w6piwtefskoh32mv%40awork3.anarazel.de
Author: Andres Freund
---
 src/backend/access/transam/xact.c | 159 +++++++++++++-----------------
 1 file changed, 71 insertions(+), 88 deletions(-)

diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index df5a67e4c31..62a42f29d44 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -346,8 +346,8 @@ static void CommitTransaction(void);
 static TransactionId RecordTransactionAbort(bool isSubXact);
 static void StartTransaction(void);
 
-static void CommitTransactionCommandInternal(void);
-static void AbortCurrentTransactionInternal(void);
+static bool CommitTransactionCommandInternal(void);
+static bool AbortCurrentTransactionInternal(void);
 
 static void StartSubTransaction(void);
 static void CommitSubTransaction(void);
@@ -3092,50 +3092,25 @@ RestoreTransactionCharacteristics(const SavedTransactionCharacteristics *s)
 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;
-	}
+	/*
+	 * Repeatedly call CommitTransactionCommandInternal() until all the work
+	 * is done.
+	 */
+	while (!CommitTransactionCommandInternal());
 }
 
 /*
- *	CommitTransactionCommandInternal - a function doing all the material work
- *		regarding handling the commit transaction command except for loop over
- *		subtransactions.
+ *	CommitTransactionCommandInternal - a function doing an iteration of work
+ *		regarding handling the commit transaction command.  In the case of
+ *		subtransactions more than one iterations could be required.  Returns
+ *		true when no more iterations required, false otherwise.
  */
-static void
+static bool
 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);
 
@@ -3319,6 +3294,25 @@ CommitTransactionCommandInternal(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).
+			 * As we need to examine the parent, return false to request the
+			 * caller to do the next iteration.
+			 */
+		case TBLOCK_SUBABORT_END:
+			CleanupSubTransaction();
+			return false;
+
+			/*
+			 * As above, but it's not dead yet, so abort first.
+			 */
+		case TBLOCK_SUBABORT_PENDING:
+			AbortSubTransaction();
+			CleanupSubTransaction();
+			return false;
+
 			/*
 			 * The current subtransaction is the target of a ROLLBACK TO
 			 * command.  Abort and pop it, then start a new subtransaction
@@ -3376,10 +3370,10 @@ CommitTransactionCommandInternal(void)
 				s->blockState = TBLOCK_SUBINPROGRESS;
 			}
 			break;
-		default:
-			/* Keep compiler quiet */
-			break;
 	}
+
+	/* Done, no more iterations required */
+	return true;
 }
 
 /*
@@ -3390,59 +3384,24 @@ CommitTransactionCommandInternal(void)
 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;
-	}
+	/*
+	 * Repeatedly call AbortCurrentTransactionInternal() until all the work
+	 * is done.
+	 */
+	while (!AbortCurrentTransactionInternal());
 }
 
 /*
- *	AbortCurrentTransactionInternal - a function doing all the material work
- *		regarding handling the abort transaction command except for loop over
- *		subtransactions.
+ *	AbortCurrentTransactionInternal - a function doing an iteration of work
+ *		regarding handling the current transaction abort.  In the case of
+ *		subtransactions more than one iterations could be required.  Returns
+ *		true when no more iterations required, false otherwise.
  */
-static void
+static bool
 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:
@@ -3563,10 +3522,34 @@ AbortCurrentTransactionInternal(void)
 			AbortSubTransaction();
 			s->blockState = TBLOCK_SUBABORT;
 			break;
-		default:
-			/* Keep compiler quiet */
-			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.  As
+			 * we need to abort the parent, return false to request the caller
+			 * to do the next iteration.
+			 */
+		case TBLOCK_SUBBEGIN:
+		case TBLOCK_SUBRELEASE:
+		case TBLOCK_SUBCOMMIT:
+		case TBLOCK_SUBABORT_PENDING:
+		case TBLOCK_SUBRESTART:
+			AbortSubTransaction();
+			CleanupSubTransaction();
+			return false;
+
+			/*
+			 * Same as above, except the Abort() was already done.
+			 */
+		case TBLOCK_SUBABORT_END:
+		case TBLOCK_SUBABORT_RESTART:
+			CleanupSubTransaction();
+			return false;
 	}
+
+	/* Done, no more iterations required */
+	return true;
 }
 
 /*
-- 
2.39.3 (Apple Git-145)