public inbox for [email protected]
help / color / mirror / Atom feedRe: Stack overflow issue
10+ messages / 5 participants
[nested] [flat]
* Re: Stack overflow issue
@ 2023-06-21 13:45 Egor Chindyaskin <[email protected]>
2023-11-24 15:14 ` Re: Stack overflow issue Heikki Linnakangas <[email protected]>
0 siblings, 1 reply; 10+ messages in thread
From: Egor Chindyaskin @ 2023-06-21 13:45 UTC (permalink / raw)
To: Sascha Kuhl <[email protected]>; +Cc: [email protected]
Hello! In continuation of the topic I would like to suggest solution.
This patch adds several checks to the vulnerable functions above.
Attachments:
[text/x-patch] v1-0001-Add-some-checks-to-avoid-stack-overflow.patch (6.0K, ../../[email protected]/2-v1-0001-Add-some-checks-to-avoid-stack-overflow.patch)
download | inline diff:
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);
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: Stack overflow issue
2023-06-21 13:45 Re: Stack overflow issue Egor Chindyaskin <[email protected]>
@ 2023-11-24 15:14 ` Heikki Linnakangas <[email protected]>
2023-12-21 08:45 ` Re: Stack overflow issue Egor Chindyaskin <[email protected]>
2024-01-05 17:23 ` Re: Stack overflow issue Robert Haas <[email protected]>
0 siblings, 2 replies; 10+ messages in thread
From: Heikki Linnakangas @ 2023-11-24 15:14 UTC (permalink / raw)
To: Egor Chindyaskin <[email protected]>; Sascha Kuhl <[email protected]>; +Cc: [email protected]
On 21/06/2023 16:45, Egor Chindyaskin wrote:
> Hello! In continuation of the topic I would like to suggest solution.
> This patch adds several checks to the vulnerable functions above.
I looked at this last patch. The depth checks are clearly better than
segfaulting, but I think we can also avoid the recursions and having to
error out. That seems nice especially for MemoryContextDelete(), which
is called at transaction cleanup.
1. CommitTransactionCommand
This is just tail recursion. The compiler will almost certainly optimize
it away unless you're using -O0. We can easily turn it into iteration
ourselves to avoid that hazard, per attached
0001-Turn-tail-recursion-into-iteration-in-CommitTransact.patch.
2. ShowTransactionStateRec
Since this is just a debugging aid, I think we can just stop recursing
if we're about to run out of stack space. Seems nicer than erroring out,
although it can still error if you run out of memory. See
0002-Avoid-stack-overflow-in-ShowTransactionStateRec.patch.
3. All the MemoryContext functions
I'm reluctant to add stack checks to these, because they are called in
places like cleaning up after transaction abort. MemoryContextDelete()
in particular. If you ereport an error, it's not clear that you can
recover cleanly; you'll leak memory if nothing else.
Fortunately MemoryContext contains pointers to parent and siblings, so
we can traverse a tree of MemoryContexts iteratively, without using stack.
MemoryContextStats() is a bit tricky, but we can put a limit on how much
it recurses, and just print a summary line if the limit is reached.
That's what we already do if a memory context has a lot of children.
(Actually, if we didn't try keep track of the # of children at each
level, to trigger the summarization, we could traverse the tree without
using stack. But a limit seems useful.)
What do you think?
--
Heikki Linnakangas
Neon (https://neon.tech)
Attachments:
[text/x-patch] 0001-Turn-tail-recursion-into-iteration-in-CommitTransact.patch (1.9K, ../../[email protected]/2-0001-Turn-tail-recursion-into-iteration-in-CommitTransact.patch)
download | inline diff:
From 543e6569a3f4730f3105379cc946cc6b54525179 Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <[email protected]>
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
[text/x-patch] 0002-Avoid-stack-overflow-in-ShowTransactionStateRec.patch (2.6K, ../../[email protected]/3-0002-Avoid-stack-overflow-in-ShowTransactionStateRec.patch)
download | inline diff:
From 1da20313b6252346fbc3f46cdfde612532e98c84 Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <[email protected]>
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
[text/x-patch] 0003-Avoid-recursion-in-MemoryContext-functions.patch (13.2K, ../../[email protected]/4-0003-Avoid-recursion-in-MemoryContext-functions.patch)
download | inline diff:
From be7a1be658479de72ee694e41a8ce0f07b8b40ad Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <[email protected]>
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
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: Stack overflow issue
2023-06-21 13:45 Re: Stack overflow issue Egor Chindyaskin <[email protected]>
2023-11-24 15:14 ` Re: Stack overflow issue Heikki Linnakangas <[email protected]>
@ 2023-12-21 08:45 ` Egor Chindyaskin <[email protected]>
1 sibling, 0 replies; 10+ messages in thread
From: Egor Chindyaskin @ 2023-12-21 08:45 UTC (permalink / raw)
To: Heikki Linnakangas <[email protected]>; Sascha Kuhl <[email protected]>; +Cc: [email protected]
On 24/11/2023 21:14, Heikki Linnakangas wrote:
> What do you think?
Hello! Thank you for researching the problem! I'm more of a tester than
a developer, so I was able to check the patches from that side.
I've configured the server with CFLAGS=" -O0" and cassert enabled and
checked the following queries:
#CommitTransactionCommand
(n=1000000; printf "BEGIN;"; for ((i=1;i<=$n;i++)); do printf "SAVEPOINT
s$i;"; done; printf "ERROR; COMMIT;") | psql >/dev/null
#ShowTransactionStateRec
(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
#MemoryContextCheck
(n=1000000; printf "begin;"; for ((i=1;i<=$n;i++)); do printf "savepoint
s$i;"; done; printf "release s1;" ) | psql >/dev/null
#MemoryContextStatsInternal
(n=1000000; printf "BEGIN;"; for ((i=1;i<=$n;i++)); do printf "SAVEPOINT
s$i;"; done; printf "SELECT
pg_log_backend_memory_contexts(pg_backend_pid())") | psql >/dev/null
On my system, every of that queries led to a server crash at a number of
savepoints in the range from 174,400 to 174,700.
With your patches applied, the savepoint counter goes well beyond these
values, I settled on an amount of approximately 300,000 savepoints.
Your patches look good to me.
Best regards,
Egor Chindyaskin
Postgres Professional: http://postgrespro.com/
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: Stack overflow issue
2023-06-21 13:45 Re: Stack overflow issue Egor Chindyaskin <[email protected]>
2023-11-24 15:14 ` Re: Stack overflow issue Heikki Linnakangas <[email protected]>
@ 2024-01-05 17:23 ` Robert Haas <[email protected]>
2024-01-05 20:16 ` Re: Stack overflow issue Andres Freund <[email protected]>
2024-01-10 21:25 ` Re: Stack overflow issue Heikki Linnakangas <[email protected]>
1 sibling, 2 replies; 10+ messages in thread
From: Robert Haas @ 2024-01-05 17:23 UTC (permalink / raw)
To: Heikki Linnakangas <[email protected]>; +Cc: Egor Chindyaskin <[email protected]>; Sascha Kuhl <[email protected]>; [email protected]
On Fri, Nov 24, 2023 at 10:47 AM Heikki Linnakangas <[email protected]> wrote:
> What do you think?
At least for 0001 and 0002, I think we should just add the stack depth checks.
With regard to 0001, CommitTransactionCommand() and friends are hard
enough to understand as it is; they need "goto" like I need an extra
hole in my head.
With regard to 0002, this function isn't sufficiently important to
justify adding special-case code for an extremely rare event. We
should just handle it the way we do in general.
I agree that in the memory-context case it might be worth expending
some more code to be more clever. But I probably wouldn't do that for
MemoryContextStats(); check_stack_depth() seems fine for that one.
In general, I think we should try to keep the number of places that
handle stack overflow in "special" ways as small as possible.
--
Robert Haas
EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: Stack overflow issue
2023-06-21 13:45 Re: Stack overflow issue Egor Chindyaskin <[email protected]>
2023-11-24 15:14 ` Re: Stack overflow issue Heikki Linnakangas <[email protected]>
2024-01-05 17:23 ` Re: Stack overflow issue Robert Haas <[email protected]>
@ 2024-01-05 20:16 ` Andres Freund <[email protected]>
2024-01-05 20:19 ` Re: Stack overflow issue Robert Haas <[email protected]>
1 sibling, 1 reply; 10+ messages in thread
From: Andres Freund @ 2024-01-05 20:16 UTC (permalink / raw)
To: Robert Haas <[email protected]>; +Cc: Heikki Linnakangas <[email protected]>; Egor Chindyaskin <[email protected]>; Sascha Kuhl <[email protected]>; [email protected]
Hi,
On 2024-01-05 12:23:25 -0500, Robert Haas wrote:
> I agree that in the memory-context case it might be worth expending
> some more code to be more clever. But I probably wouldn't do that for
> MemoryContextStats(); check_stack_depth() seems fine for that one.
We run MemoryContextStats() when we fail to allocate memory, including during
abort processing after a previous error. So I think it qualifies for being
somewhat special. Thus I suspect check_stack_depth() wouldn't be a good idea -
but we could make the stack_is_too_deep() path simpler and just return in the
existing MemoryContextStatsInternal() when that's the case.
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: Stack overflow issue
2023-06-21 13:45 Re: Stack overflow issue Egor Chindyaskin <[email protected]>
2023-11-24 15:14 ` Re: Stack overflow issue Heikki Linnakangas <[email protected]>
2024-01-05 17:23 ` Re: Stack overflow issue Robert Haas <[email protected]>
2024-01-05 20:16 ` Re: Stack overflow issue Andres Freund <[email protected]>
@ 2024-01-05 20:19 ` Robert Haas <[email protected]>
0 siblings, 0 replies; 10+ messages in thread
From: Robert Haas @ 2024-01-05 20:19 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: Heikki Linnakangas <[email protected]>; Egor Chindyaskin <[email protected]>; Sascha Kuhl <[email protected]>; [email protected]
On Fri, Jan 5, 2024 at 3:16 PM Andres Freund <[email protected]> wrote:
> On 2024-01-05 12:23:25 -0500, Robert Haas wrote:
> > I agree that in the memory-context case it might be worth expending
> > some more code to be more clever. But I probably wouldn't do that for
> > MemoryContextStats(); check_stack_depth() seems fine for that one.
>
> We run MemoryContextStats() when we fail to allocate memory, including during
> abort processing after a previous error. So I think it qualifies for being
> somewhat special.
OK.
> Thus I suspect check_stack_depth() wouldn't be a good idea -
> but we could make the stack_is_too_deep() path simpler and just return in the
> existing MemoryContextStatsInternal() when that's the case.
Since this kind of code will be exercised so rarely, it's highly
vulnerable to bugs, so I favor keeping it as simple as we can.
--
Robert Haas
EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: Stack overflow issue
2023-06-21 13:45 Re: Stack overflow issue Egor Chindyaskin <[email protected]>
2023-11-24 15:14 ` Re: Stack overflow issue Heikki Linnakangas <[email protected]>
2024-01-05 17:23 ` Re: Stack overflow issue Robert Haas <[email protected]>
@ 2024-01-10 21:25 ` Heikki Linnakangas <[email protected]>
2024-01-11 17:37 ` Re: Stack overflow issue Robert Haas <[email protected]>
1 sibling, 1 reply; 10+ messages in thread
From: Heikki Linnakangas @ 2024-01-10 21:25 UTC (permalink / raw)
To: Robert Haas <[email protected]>; +Cc: Egor Chindyaskin <[email protected]>; Sascha Kuhl <[email protected]>; [email protected]
On 05/01/2024 19:23, Robert Haas wrote:
> On Fri, Nov 24, 2023 at 10:47 AM Heikki Linnakangas <[email protected]> wrote:
>> What do you think?
>
> At least for 0001 and 0002, I think we should just add the stack depth checks.
>
> With regard to 0001, CommitTransactionCommand() and friends are hard
> enough to understand as it is; they need "goto" like I need an extra
> hole in my head.
>
> With regard to 0002, this function isn't sufficiently important to
> justify adding special-case code for an extremely rare event. We
> should just handle it the way we do in general.
>
> I agree that in the memory-context case it might be worth expending
> some more code to be more clever. But I probably wouldn't do that for
> MemoryContextStats(); check_stack_depth() seems fine for that one.
>
> In general, I think we should try to keep the number of places that
> handle stack overflow in "special" ways as small as possible.
The problem with CommitTransactionCommand (or rather
AbortCurrentTransaction() which has the same problem)
and ShowTransactionStateRec is that they get called in a state where
aborting can lead to a panic. If you add a "check_stack_depth()" to them
and try to reproducer scripts that Egor posted, you still get a panic.
I'm not sure if MemoryContextStats() could safely elog(ERROR). But at
least it would mask the "out of memory" that caused the stats to be
printed in the first place.
--
Heikki Linnakangas
Neon (https://neon.tech)
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: Stack overflow issue
2023-06-21 13:45 Re: Stack overflow issue Egor Chindyaskin <[email protected]>
2023-11-24 15:14 ` Re: Stack overflow issue Heikki Linnakangas <[email protected]>
2024-01-05 17:23 ` Re: Stack overflow issue Robert Haas <[email protected]>
2024-01-10 21:25 ` Re: Stack overflow issue Heikki Linnakangas <[email protected]>
@ 2024-01-11 17:37 ` Robert Haas <[email protected]>
2024-01-12 15:12 ` Re: Stack overflow issue Heikki Linnakangas <[email protected]>
0 siblings, 1 reply; 10+ messages in thread
From: Robert Haas @ 2024-01-11 17:37 UTC (permalink / raw)
To: Heikki Linnakangas <[email protected]>; +Cc: Egor Chindyaskin <[email protected]>; Sascha Kuhl <[email protected]>; [email protected]
On Wed, Jan 10, 2024 at 4:25 PM Heikki Linnakangas <[email protected]> wrote:
> The problem with CommitTransactionCommand (or rather
> AbortCurrentTransaction() which has the same problem)
> and ShowTransactionStateRec is that they get called in a state where
> aborting can lead to a panic. If you add a "check_stack_depth()" to them
> and try to reproducer scripts that Egor posted, you still get a panic.
Hmm, that's unfortunate. I'm not sure what to do about that. But I'd
still suggest looking for a goto-free approach.
--
Robert Haas
EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: Stack overflow issue
2023-06-21 13:45 Re: Stack overflow issue Egor Chindyaskin <[email protected]>
2023-11-24 15:14 ` Re: Stack overflow issue Heikki Linnakangas <[email protected]>
2024-01-05 17:23 ` Re: Stack overflow issue Robert Haas <[email protected]>
2024-01-10 21:25 ` Re: Stack overflow issue Heikki Linnakangas <[email protected]>
2024-01-11 17:37 ` Re: Stack overflow issue Robert Haas <[email protected]>
@ 2024-01-12 15:12 ` Heikki Linnakangas <[email protected]>
0 siblings, 0 replies; 10+ messages in thread
From: Heikki Linnakangas @ 2024-01-12 15:12 UTC (permalink / raw)
To: Robert Haas <[email protected]>; +Cc: Egor Chindyaskin <[email protected]>; Sascha Kuhl <[email protected]>; [email protected]
On 11/01/2024 19:37, Robert Haas wrote:
> On Wed, Jan 10, 2024 at 4:25 PM Heikki Linnakangas <[email protected]> wrote:
>> The problem with CommitTransactionCommand (or rather
>> AbortCurrentTransaction() which has the same problem)
>> and ShowTransactionStateRec is that they get called in a state where
>> aborting can lead to a panic. If you add a "check_stack_depth()" to them
>> and try to reproducer scripts that Egor posted, you still get a panic.
>
> Hmm, that's unfortunate. I'm not sure what to do about that. But I'd
> still suggest looking for a goto-free approach.
Here's one goto-free attempt. It adds a local loop to where the
recursion was, so that if you have a chain of subtransactions that need
to be aborted in CommitTransactionCommand, they are aborted iteratively.
The TBLOCK_SUBCOMMIT case already had such a loop.
I added a couple of comments in the patch marked with "REVIEWER NOTE",
to explain why I changed some things. They are to be removed before
committing.
I'm not sure if this is better than a goto. In fact, even if we commit
this, I think I'd still prefer to replace the remaining recursive calls
with a goto. Recursion feels a weird to me, when we're unwinding the
states from the stack as we go.
Of course we could use a "for (;;) { ... continue }" construct around
the whole function, instead of a goto, but I don't think that's better
than a goto in this case.
--
Heikki Linnakangas
Neon (https://neon.tech)
Attachments:
[text/x-patch] v2-0001-Turn-tail-recursion-into-iteration-in-AbortCurren.patch (7.5K, ../../[email protected]/2-v2-0001-Turn-tail-recursion-into-iteration-in-AbortCurren.patch)
download | inline diff:
From 44568e6feef79d604bbe168c0839ab2e03c99f8a Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <[email protected]>
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
^ permalink raw reply [nested|flat] 10+ messages in thread
* [PATCH v25 6/9] Row pattern recognition patch (docs).
@ 2024-12-21 06:19 Tatsuo Ishii <[email protected]>
0 siblings, 0 replies; 10+ messages in thread
From: Tatsuo Ishii @ 2024-12-21 06:19 UTC (permalink / raw)
---
doc/src/sgml/advanced.sgml | 82 ++++++++++++++++++++++++++++++++++++
doc/src/sgml/func.sgml | 54 ++++++++++++++++++++++++
doc/src/sgml/ref/select.sgml | 38 ++++++++++++++++-
3 files changed, 172 insertions(+), 2 deletions(-)
diff --git a/doc/src/sgml/advanced.sgml b/doc/src/sgml/advanced.sgml
index 755c9f1485..b0b1d1c51e 100644
--- a/doc/src/sgml/advanced.sgml
+++ b/doc/src/sgml/advanced.sgml
@@ -537,6 +537,88 @@ WHERE pos < 3;
<literal>rank</literal> less than 3.
</para>
+ <para>
+ Row pattern common syntax can be used to perform row pattern recognition
+ in a query. The row pattern common syntax includes two sub
+ clauses: <literal>DEFINE</literal>
+ and <literal>PATTERN</literal>. <literal>DEFINE</literal> defines
+ definition variables along with an expression. The expression must be a
+ logical expression, which means it must
+ return <literal>TRUE</literal>, <literal>FALSE</literal>
+ or <literal>NULL</literal>. The expression may comprise column references
+ and functions. Window functions, aggregate functions and subqueries are
+ not allowed. An example of <literal>DEFINE</literal> is as follows.
+
+<programlisting>
+DEFINE
+ LOWPRICE AS price <= 100,
+ UP AS price > PREV(price),
+ DOWN AS price < PREV(price)
+</programlisting>
+
+ Note that <function>PREV</function> returns the price column in the
+ previous row if it's called in a context of row pattern recognition. Thus in
+ the second line the definition variable "UP" is <literal>TRUE</literal>
+ when the price column in the current row is greater than the price column
+ in the previous row. Likewise, "DOWN" is <literal>TRUE</literal> when when
+ the price column in the current row is lower than the price column in the
+ previous row.
+ </para>
+ <para>
+ Once <literal>DEFINE</literal> exists, <literal>PATTERN</literal> can be
+ used. <literal>PATTERN</literal> defines a sequence of rows that satisfies
+ certain conditions. For example following <literal>PATTERN</literal>
+ defines that a row starts with the condition "LOWPRICE", then one or more
+ rows satisfy "UP" and finally one or more rows satisfy "DOWN". Note that
+ "+" means one or more matches. Also you can use "*", which means zero or
+ more matches. If a sequence of rows which satisfies the PATTERN is found,
+ in the starting row of the sequence of rows all window functions and
+ aggregates are shown in the target list. Note that aggregations only look
+ into the matched rows, rather than whole frame. On the second or
+ subsequent rows all window functions are NULL. Aggregates are NULL or 0
+ (count case) depending on its aggregation definition. For rows that do not
+ match on the PATTERN, all window functions and aggregates are shown AS
+ NULL too, except count showing 0. This is because the rows do not match,
+ thus they are in an empty frame. Example of a <literal>SELECT</literal>
+ using the <literal>DEFINE</literal> and <literal>PATTERN</literal> clause
+ is as follows.
+
+<programlisting>
+SELECT company, tdate, price,
+ first_value(price) OVER w,
+ max(price) OVER w,
+ count(price) OVER w
+FROM stock
+ WINDOW w AS (
+ PARTITION BY company
+ ORDER BY tdate
+ ROWS BETWEEN CURRENT ROW AND UNBOUNDED FOLLOWING
+ AFTER MATCH SKIP PAST LAST ROW
+ INITIAL
+ PATTERN (LOWPRICE UP+ DOWN+)
+ DEFINE
+ LOWPRICE AS price <= 100,
+ UP AS price > PREV(price),
+ DOWN AS price < PREV(price)
+);
+</programlisting>
+<screen>
+ company | tdate | price | first_value | max | count
+----------+------------+-------+-------------+-----+-------
+ company1 | 2023-07-01 | 100 | 100 | 200 | 4
+ company1 | 2023-07-02 | 200 | | | 0
+ company1 | 2023-07-03 | 150 | | | 0
+ company1 | 2023-07-04 | 140 | | | 0
+ company1 | 2023-07-05 | 150 | | | 0
+ company1 | 2023-07-06 | 90 | 90 | 130 | 4
+ company1 | 2023-07-07 | 110 | | | 0
+ company1 | 2023-07-08 | 130 | | | 0
+ company1 | 2023-07-09 | 120 | | | 0
+ company1 | 2023-07-10 | 130 | | | 0
+(10 rows)
+</screen>
+ </para>
+
<para>
When a query involves multiple window functions, it is possible to write
out each one with a separate <literal>OVER</literal> clause, but this is
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 47370e581a..17a38c4046 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -23338,6 +23338,7 @@ SELECT count(*) FROM sometable;
returns <literal>NULL</literal> if there is no such row.
</para></entry>
</row>
+
</tbody>
</tgroup>
</table>
@@ -23377,6 +23378,59 @@ SELECT count(*) FROM sometable;
Other frame specifications can be used to obtain other effects.
</para>
+ <para>
+ Row pattern recognition navigation functions are listed in
+ <xref linkend="functions-rpr-navigation-table"/>. These functions
+ can be used to describe DEFINE clause of Row pattern recognition.
+ </para>
+
+ <table id="functions-rpr-navigation-table">
+ <title>Row Pattern Navigation Functions</title>
+ <tgroup cols="1">
+ <thead>
+ <row>
+ <entry role="func_table_entry"><para role="func_signature">
+ Function
+ </para>
+ <para>
+ Description
+ </para></entry>
+ </row>
+ </thead>
+
+ <tbody>
+ <row>
+ <entry role="func_table_entry"><para role="func_signature">
+ <indexterm>
+ <primary>prev</primary>
+ </indexterm>
+ <function>prev</function> ( <parameter>value</parameter> <type>anyelement</type> )
+ <returnvalue>anyelement</returnvalue>
+ </para>
+ <para>
+ Returns the column value at the previous row;
+ returns NULL if there is no previous row in the window frame.
+ </para></entry>
+ </row>
+
+ <row>
+ <entry role="func_table_entry"><para role="func_signature">
+ <indexterm>
+ <primary>next</primary>
+ </indexterm>
+ <function>next</function> ( <parameter>value</parameter> <type>anyelement</type> )
+ <returnvalue>anyelement</returnvalue>
+ </para>
+ <para>
+ Returns the column value at the next row;
+ returns NULL if there is no next row in the window frame.
+ </para></entry>
+ </row>
+
+ </tbody>
+ </tgroup>
+ </table>
+
<note>
<para>
The SQL standard defines a <literal>RESPECT NULLS</literal> or
diff --git a/doc/src/sgml/ref/select.sgml b/doc/src/sgml/ref/select.sgml
index d7089eac0b..7e1c9989ba 100644
--- a/doc/src/sgml/ref/select.sgml
+++ b/doc/src/sgml/ref/select.sgml
@@ -969,8 +969,8 @@ WINDOW <replaceable class="parameter">window_name</replaceable> AS ( <replaceabl
The <replaceable class="parameter">frame_clause</replaceable> can be one of
<synopsis>
-{ RANGE | ROWS | GROUPS } <replaceable>frame_start</replaceable> [ <replaceable>frame_exclusion</replaceable> ]
-{ RANGE | ROWS | GROUPS } BETWEEN <replaceable>frame_start</replaceable> AND <replaceable>frame_end</replaceable> [ <replaceable>frame_exclusion</replaceable> ]
+{ RANGE | ROWS | GROUPS } <replaceable>frame_start</replaceable> [ <replaceable>frame_exclusion</replaceable> ] [row_pattern_common_syntax]
+{ RANGE | ROWS | GROUPS } BETWEEN <replaceable>frame_start</replaceable> AND <replaceable>frame_end</replaceable> [ <replaceable>frame_exclusion</replaceable> ] [row_pattern_common_syntax]
</synopsis>
where <replaceable>frame_start</replaceable>
@@ -1077,6 +1077,40 @@ EXCLUDE NO OTHERS
a given peer group will be in the frame or excluded from it.
</para>
+ <para>
+ The
+ optional <replaceable class="parameter">row_pattern_common_syntax</replaceable>
+ defines the <firstterm>row pattern recognition condition</firstterm> for
+ this
+ window. <replaceable class="parameter">row_pattern_common_syntax</replaceable>
+ includes following subclauses. <literal>AFTER MATCH SKIP PAST LAST
+ ROW</literal> or <literal>AFTER MATCH SKIP TO NEXT ROW</literal> controls
+ how to proceed to next row position after a match
+ found. With <literal>AFTER MATCH SKIP PAST LAST ROW</literal> (the
+ default) next row position is next to the last row of previous match. On
+ the other hand, with <literal>AFTER MATCH SKIP TO NEXT ROW</literal> next
+ row position is always next to the last row of previous
+ match. <literal>DEFINE</literal> defines definition variables along with a
+ boolean expression. <literal>PATTERN</literal> defines a sequence of rows
+ that satisfies certain conditions using variables defined
+ in <literal>DEFINE</literal> clause. If the variable is not defined in
+ the <literal>DEFINE</literal> clause, it is implicitly assumed
+ following is defined in the <literal>DEFINE</literal> clause.
+
+<synopsis>
+<literal>variable_name</literal> AS TRUE
+</synopsis>
+
+ Note that the maximu number of variables defined
+ in <literal>DEFINE</literal> clause is 26.
+
+<synopsis>
+[ AFTER MATCH SKIP PAST LAST ROW | AFTER MATCH SKIP TO NEXT ROW ]
+PATTERN <replaceable class="parameter">pattern_variable_name</replaceable>[+] [, ...]
+DEFINE <replaceable class="parameter">definition_varible_name</replaceable> AS <replaceable class="parameter">expression</replaceable> [, ...]
+</synopsis>
+ </para>
+
<para>
The purpose of a <literal>WINDOW</literal> clause is to specify the
behavior of <firstterm>window functions</firstterm> appearing in the query's
--
2.25.1
----Next_Part(Sat_Dec_21_18_20_04_2024_526)--
Content-Type: Text/X-Patch; charset=us-ascii
Content-Transfer-Encoding: 7bit
Content-Disposition: inline;
filename="v25-0007-Row-pattern-recognition-patch-tests.patch"
^ permalink raw reply [nested|flat] 10+ messages in thread
end of thread, other threads:[~2024-12-21 06:19 UTC | newest]
Thread overview: 10+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2023-06-21 13:45 Re: Stack overflow issue Egor Chindyaskin <[email protected]>
2023-11-24 15:14 ` Heikki Linnakangas <[email protected]>
2023-12-21 08:45 ` Egor Chindyaskin <[email protected]>
2024-01-05 17:23 ` Robert Haas <[email protected]>
2024-01-05 20:16 ` Andres Freund <[email protected]>
2024-01-05 20:19 ` Robert Haas <[email protected]>
2024-01-10 21:25 ` Heikki Linnakangas <[email protected]>
2024-01-11 17:37 ` Robert Haas <[email protected]>
2024-01-12 15:12 ` Heikki Linnakangas <[email protected]>
2024-12-21 06:19 [PATCH v25 6/9] Row pattern recognition patch (docs). Tatsuo Ishii <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox