From 34a1cd1b41ca72d67042392eaef0d95977b14709 Mon Sep 17 00:00:00 2001 From: jian he Date: Fri, 4 Sep 2026 09:32:53 +0900 Subject: [PATCH] Reset the DEFINE evaluation context where the predicate runs rpr_prepare_row() reset rprContext once per row and nfa_reevaluate_dependent_vars() reset it again per context, so the placement was a property held at a distance: it was correct only because nothing allocated across one of those boundaries, and the comment on the evaluation itself had to name both functions to say where its scratch would be freed. Reset it in nfa_eval_var_match(), immediately before the predicate it belongs to. A DEFINE predicate leaves nothing behind that outlives it: EEOP_RPR_NAV_RESTORE stabilizes a pass-by-reference navigation result in this same context, and the predicate consumes it before returning a by-value bool. Every allocation into the context is now preceded by the reset that frees the previous one, so no caller has to arrange it, and one row's contexts no longer accumulate their scratch to the row boundary. nfa_reevaluate_dependent_vars() becomes nfa_invalidate_dependent_vars(). The function evaluates nothing; it flips the match_start-dependent variables back to RPR_VAR_UNEVALUATED and leaves the evaluating to nfa_match(), which is the verb its own header comment already used. Its guard moves in with it -- both the query-wide test for any dependent variable and the per-context test for a match_start the shared evaluation already installed -- so the call site is now unconditional. Also say what matchedState is for. Its being non-NULL is what records a match; matchEndRow cannot, because an empty match ends below matchStartRow and would read as a failure. The state itself is never read. --- src/backend/executor/execRPR.c | 44 +++++++++++++--------------- src/backend/executor/nodeWindowAgg.c | 3 -- src/include/nodes/execnodes.h | 10 ++++++- 3 files changed, 29 insertions(+), 28 deletions(-) diff --git a/src/backend/executor/execRPR.c b/src/backend/executor/execRPR.c index 16a16221fb8..dd11157e4c5 100644 --- a/src/backend/executor/execRPR.c +++ b/src/backend/executor/execRPR.c @@ -105,7 +105,7 @@ static void nfa_advance_state(WindowAggState *winstate, RPRNFAContext *ctx, static void nfa_advance(WindowAggState *winstate, RPRNFAContext *ctx, int64 currentPos); -static void nfa_reevaluate_dependent_vars(WindowAggState *winstate, +static void nfa_invalidate_dependent_vars(WindowAggState *winstate, RPRNFAContext *ctx, int64 currentPos); @@ -764,8 +764,8 @@ nfa_prune_skipped_contexts(WindowAggState *winstate, RPRNFAContext *ctx) * mismatch at a frame boundary and at partition-end finalization. * * The caller must have set up the current row (ecxt_outertuple, currentpos, - * nav_match_start, nav_slot cache) via rpr_prepare_row() / - * nfa_reevaluate_dependent_vars() before consumption. + * nav_match_start) and invalidated the nav slot cache, via rpr_prepare_row() + * or nfa_invalidate_dependent_vars(), before consumption. * * Per ISO/IEC 19075-5 Feature R020, pattern variables not listed in DEFINE * are implicitly TRUE -- they match every row. This is checked via @@ -795,11 +795,14 @@ nfa_eval_var_match(WindowAggState *winstate, RPRPatternElement *elem, bool isnull; /* - * Switch into rprContext's per-tuple memory: ExecEvalExpr() does not - * do it for us, and the predicate's scratch has to land in the - * context that rpr_prepare_row() and nfa_reevaluate_dependent_vars() - * reset, not in the caller's longer-lived one. + * Free the previous predicate evaluation's storage. A DEFINE + * predicate leaves nothing behind but the RPRVarMatch stored below -- + * the navigation steps stabilize pass-by-ref results in this same + * context, and those are consumed before the predicate returns -- so + * resetting here is always safe and no caller has to arrange it. */ + ResetExprContext(winstate->rprContext); + result = ExecEvalExprSwitchContext(exprState, winstate->rprContext, &isnull); varMatched[varId] = (!isnull && DatumGetBool(result)) ? @@ -1648,14 +1651,14 @@ nfa_advance(WindowAggState *winstate, RPRNFAContext *ctx, int64 currentPos) } /* - * nfa_reevaluate_dependent_vars + * nfa_invalidate_dependent_vars * Invalidate match_start-dependent DEFINE variables for a context whose * matchStartRow differs from the shared evaluation's nav_match_start. * * Only variables in defineMatchStartDependent are affected: they are reset to * RPR_VAR_UNEVALUATED so nfa_match() re-evaluates them lazily against this - * context's matchStartRow. match_start-independent variables keep their - * cached value across contexts, since they do not read nav_match_start. + * context's matchStartRow. The remaining variables keep their cached value + * across contexts, since they do not read nav_match_start. * * nav_match_start is installed for this context and left in place: FIRST/LAST * read it at evaluation time, which happens later during nfa_match(), so it @@ -1663,24 +1666,18 @@ nfa_advance(WindowAggState *winstate, RPRNFAContext *ctx, int64 currentPos) * row's shared setup in advance_reduced_frame_nfa, overwrites it. */ static void -nfa_reevaluate_dependent_vars(WindowAggState *winstate, RPRNFAContext *ctx, +nfa_invalidate_dependent_vars(WindowAggState *winstate, RPRNFAContext *ctx, int64 currentPos) { int varIdx = -1; + if (bms_is_empty(winstate->defineMatchStartDependent) || + ctx->matchStartRow == winstate->nav_match_start) + return; + /* Caller keeps winstate->currentpos at the scan position for lazy eval. */ Assert(winstate->currentpos == currentPos); - /* - * Release the previous context's DEFINE evaluation memory. Match-start- - * dependent variables are re-evaluated once per context (they are reset - * to UNEVALUATED below), so without this reset their per-tuple scratch - * would accumulate across every context of a row -- bounded only by the - * per-row reset in rpr_prepare_row. rprContext is the dedicated DEFINE - * context, so this frees neither the input nor the output tuple memory. - */ - ResetExprContext(winstate->rprContext); - /* Install this context's match_start for FIRST/LAST and keep it in place. */ winstate->nav_match_start = ctx->matchStartRow; @@ -1838,7 +1835,6 @@ void ExecRPRProcessRow(WindowAggState *winstate, int64 currentPos) { RPRVarMatch *varMatched = winstate->nfaVarMatched; - bool hasDependent = !bms_is_empty(winstate->defineMatchStartDependent); int64 frameOffset = -1; /* -1 = frame runs to the partition end */ /* @@ -1908,8 +1904,8 @@ ExecRPRProcessRow(WindowAggState *winstate, int64 currentPos) Assert(ctx != winstate->nfaContext || ctx->matchStartRow == winstate->nav_match_start); - if (hasDependent && ctx->matchStartRow != winstate->nav_match_start) - nfa_reevaluate_dependent_vars(winstate, ctx, currentPos); + nfa_invalidate_dependent_vars(winstate, ctx, currentPos); + nfa_match(winstate, ctx, varMatched, currentPos); ctx->lastProcessedRow = currentPos; } diff --git a/src/backend/executor/nodeWindowAgg.c b/src/backend/executor/nodeWindowAgg.c index 92189d358be..d9c5b25cd3d 100644 --- a/src/backend/executor/nodeWindowAgg.c +++ b/src/backend/executor/nodeWindowAgg.c @@ -4852,9 +4852,6 @@ rpr_prepare_row(WindowObject winobj, int64 pos, RPRVarMatch *varMatched) ExprContext *econtext = winstate->rprContext; TupleTableSlot *slot; - /* Release the previous row's DEFINE evaluation memory */ - ResetExprContext(econtext); - /* Fetch current row into temp_slot_1 */ slot = winstate->temp_slot_1; if (!window_gettupleslot(winobj, pos, slot)) diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h index 06a7b3e9587..d34d0758d26 100644 --- a/src/include/nodes/execnodes.h +++ b/src/include/nodes/execnodes.h @@ -2617,7 +2617,15 @@ typedef struct RPRNFAContext int64 matchEndRow; /* last row of the match; below matchStartRow * for an empty one, -1 before any */ int64 lastProcessedRow; /* last row processed (for fail depth) */ - RPRNFAState *matchedState; /* this context's match candidate, or NULL */ + + /* + * The state that reached FIN, or NULL. Its being non-NULL is what + * records a match; matchEndRow cannot, because an empty match ends below + * matchStartRow and would read as a failure. The state itself is never + * read. + */ + RPRNFAState *matchedState; + bool matchUpdated; /* matchedState was set or replaced during the * advance now running */