On Wed, Oct 14, 2026 at 8:35 AM Henson Choi <[email protected]> wrote: > > 0003 Remove WindowAggState.nfaStateSize > > > I always want to get rid of WindowAggState.nfaStateSize. It's ugly. > > I would like to keep that field. The size depends only on the > pattern, so it is computed once, and I do not want to repeat the > same computation on a path this frequent just to save a field. To > me the field is the cleaner choice. > Hi.
Let's agree to disagree, at least now, removing WindowAggState.nfaStateSize is not performance related. Also as mentioned in https://www.postgresql.org/message-id/688312.1782222788%40sss.pgh.pa.us we should use size_t, not using Size Please check attached 0001 and 0002. 0001: Remove RPRNavExpr.navno navno spans all three stages: ParseRPRNavCall() initializes it, compute_define_metadata() compute it while planning, and ExecInitExprRec() use it. Tracking and keeping those in sync is not trivial. It's doable to just remove it. 0002: Remove compute_define_metadata IMHO compute_define_metadata() is complicated. Instead, loop over the DEFINE clause with foreach, run an expression walker on each variable's expression to check whether it depends on the match start, and if it does, record that variable's index. -- jian https://www.enterprisedb.com/
From d662aaad6d62990e3fef6724da3c56b8d127f667 Mon Sep 17 00:00:00 2001 From: jian he <[email protected]> Date: Sat, 17 Oct 2026 01:16:55 +0800 Subject: [PATCH v54 1/2] Remove RPRNavExpr.navno navno spans all three stages: ParseRPRNavCall() initializes it, compute_define_metadata() compute it while planning, and ExecInitExprRec() use it Tracking and keep those in the same page is not trivial. Instead, initialize each navigation with its WindowAggState while the DEFINE clause is compiled, the same way ExecInitExprRec() handles WindowAggState.funcs for window functions. The list is then built by the same walk that compiles the navigations, and navno is no longer needed. While at it, move the T_RPRNavExpr case of ExecInitExprRec() into a dedicated function, ExecInitRPRNavExpr(), which makes the code tidier. Based on https://github.com/assam258-5892/postgres/tree/RPR-20260930 --- src/backend/executor/README.rpr | 47 +++---- src/backend/executor/execExpr.c | 167 ++++++++++++++---------- src/backend/executor/nodeWindowAgg.c | 110 ++-------------- src/backend/optimizer/plan/createplan.c | 26 +--- src/backend/parser/parse_func.c | 1 - src/include/nodes/execnodes.h | 16 +-- src/include/nodes/primnodes.h | 3 - 7 files changed, 145 insertions(+), 225 deletions(-) diff --git a/src/backend/executor/README.rpr b/src/backend/executor/README.rpr index 16e158fdcf..4d5dadcc57 100644 --- a/src/backend/executor/README.rpr +++ b/src/backend/executor/README.rpr @@ -458,8 +458,7 @@ list exactly as the generated code would and then appends each entry's resname. A window with no DEFINE clause has an empty list, so the second step contributes nothing and its query id is unchanged. -The other RPR node fields follow the usual rule. RPRNavExpr.navno is -query_jumble_ignore because the planner assigns it (VII-6), and +The other RPR node fields follow the usual rule. RPRPatternNode.trailing_alt never survives parsing (III-2). Chapter IV Planning the DEFINE Clause @@ -1457,10 +1456,9 @@ frees that tuple when it re-fetches the slot for another position. EEOP_RPR_NAV_RESTORE therefore copies a non-null pass-by-ref result into ecxt_per_tuple_memory, which survives until the next ResetExprContext, before it returns. Rather than look the type up on every evaluation, -ExecInitExprRec() reads the type length and by-value flag once from the -RPRNavExpr's resulttype and stores them in the shared RPRNavState as it -emits the RESTORE step; every compilation of the same navigation writes -the same pair, so repeating it is harmless. +ExecInitRPRNavExpr() reads the type length and by-value flag once from +the RPRNavExpr's resulttype and stores them in the RPRNavState that the +SET and RESTORE steps share. The nfaVarMatched entries are filled lazily during Phase 1 (Match) as variables are consumed. @@ -1580,27 +1578,22 @@ row, so the two drag the single seekpos back and forth. In memory that costs a pointer move; spilled it costs a re-read, and the XXX at prepare_tuplestore() carries the measurement. -Each RPRNavExpr carries a navno, its index into that list of offsets. -The parser leaves it -1; compute_define_metadata() -(optimizer/plan/createplan.c) numbers the navigations in walk order -while it classifies match_start dependency, and build_define_offsets() -fills rprNavOffsets by walking the same expressions in the same order, -so entry i is the navigation with navno i. The offsets live in -executor state rather than on the RPRNavExpr, the plan tree being -read-only, so the expression compiler looks the entry up by navno when -it emits the EEOP_RPR_NAV_SET/RESTORE pair. The two walks have to stay -in step: execExpr.c raises "RPRNavExpr navno %d out of range" instead -of indexing past the list, and rejects an entry that does not point -back at the same RPRNavExpr. - -What the compiler takes from the entry is its RPRNavState, the state -VII-3 calls shared. build_nav_offsets() creates one per navigation with -both offsets null, and the SET and RESTORE steps compiled for that -navigation both point at it, so build_define_offsets() has to run before -the ExecInitQual() loop over defineClause in ExecInitWindowAgg(). -resolve_one_nav() pins the inner and outer offsets, defaults included, -into it: at init for a navigation whose offsets are all Const -(for EXPLAIN), and again on every scan through resolve_nav_offsets(). +The offsets live in executor state rather than on the RPRNavExpr, the +plan tree being read-only. Each navigation registers itself while the +DEFINE clause is compiled: when ExecInitExprRec() (execExpr.c) reaches an +RPRNavExpr, it calls ExecInitRPRNavExpr(), which appends an RPRNavOffsets +entry to rprNavOffsets, compiles the offset expressions, creates the +entry's RPRNavState, the state VII-3 calls shared, and emits the SET and +RESTORE steps that both point at it. This is how ExecInitExprRec() +registers a WindowFunc with winstate->funcs, and it means the list is +built by the same walk that compiles the navigations, so no index has to +tie the two together. + +Once the ExecInitQual() loop over defineClause in ExecInitWindowAgg() has +registered every navigation, build_define_offsets() resolves the +ones whose offsets are all Const, for EXPLAIN. resolve_one_nav() pins +the inner and outer offsets, defaults included, into the RPRNavState: +at that point, and again on every scan through resolve_nav_offsets(). ExecEvalRPRNavSet() reads them from there, asserting they are set, rather than evaluating the offset per row, so a rescan changes the offsets the compiled DEFINE expressions use without recompiling them. diff --git a/src/backend/executor/execExpr.c b/src/backend/executor/execExpr.c index d6b1b3cd31..4bfe6ca95d 100644 --- a/src/backend/executor/execExpr.c +++ b/src/backend/executor/execExpr.c @@ -103,6 +103,9 @@ static void ExecInitJsonCoercion(ExprState *state, JsonReturning *returning, ErrorSaveContext *escontext, bool omit_quotes, bool exists_coerce, Datum *resv, bool *resnull); +static void ExecInitRPRNavExpr(ExprEvalStep *scratch, RPRNavExpr *nav, + ExprState *state, + Datum *resv, bool *resnull); /* @@ -1170,77 +1173,9 @@ ExecInitExprRec(Expr *node, ExprState *state, case T_RPRNavExpr: { - /* - * RPR navigation functions (PREV/NEXT/FIRST/LAST) are - * compiled into EEOP_RPR_NAV_SET / EEOP_RPR_NAV_RESTORE - * opcodes instead of a normal function call. The SET opcode - * swaps ecxt_outertuple to the target row, the argument - * expression is compiled normally (reads from the swapped - * slot), and the RESTORE opcode restores the original slot. - */ - RPRNavState *rprnavstate; - RPRNavOffsets *entry; RPRNavExpr *nav = (RPRNavExpr *) node; - WindowAggState *winstate; - int skip_arg_step; - - Assert(state->parent && IsA(state->parent, WindowAggState)); - winstate = (WindowAggState *) state->parent; - - /* - * The offsets live in executor state, not on the RPRNavExpr, - * because the plan tree is read-only. navno indexes the list - * build_define_offsets() filled at startup; the values in it - * are settled per scan by resolve_nav_offsets(). - */ - Assert(nav->navno >= 0 && - nav->navno < list_length(winstate->rprNavOffsets)); - - entry = list_nth(winstate->rprNavOffsets, nav->navno); - Assert(entry->nav == nav); - rprnavstate = entry->rprnavstate; - - /* Emit SET opcode: swap slot to target row */ - scratch.opcode = EEOP_RPR_NAV_SET; - scratch.d.rpr_nav.rprnavstate = rprnavstate; - - ExprEvalPushStep(state, &scratch); - - /* - * If the target row does not exist, skip evaluation of the - * argument expression and go straight to RESTORE. The - * EEOP_RPR_NAV_SET step writes a definitive resnull (false - * when the target row exists), so the jump condition is - * always up to date. - */ - skip_arg_step = state->steps_len; - scratch.opcode = EEOP_JUMP_IF_NULL; - scratch.resvalue = resv; - scratch.resnull = resnull; - scratch.d.jump.jumpdone = -1; /* set below */ - ExprEvalPushStep(state, &scratch); - - /* Compile the argument expression normally */ - ExecInitExprRec(nav->arg, state, resv, resnull); - - /* out-of-range jump lands on the RESTORE step */ - state->steps[skip_arg_step].d.jump.jumpdone = state->steps_len; - - /* Emit RESTORE opcode: restore original slot */ - scratch.opcode = EEOP_RPR_NAV_RESTORE; - scratch.resvalue = resv; - scratch.resnull = resnull; - scratch.d.rpr_nav.rprnavstate = rprnavstate; - /* - * The state is shared with the offsets entry, but resulttype - * belongs to the plan node, so every compilation of this - * navigation writes the same pair. - */ - get_typlenbyval(nav->resulttype, - &rprnavstate->resulttyplen, - &rprnavstate->resulttypbyval); - ExprEvalPushStep(state, &scratch); + ExecInitRPRNavExpr(&scratch, nav, state, resv, resnull); break; } @@ -5141,3 +5076,97 @@ ExecInitJsonCoercion(ExprState *state, JsonReturning *returning, DomainHasConstraints(returning->typid, NULL); ExprEvalPushStep(state, &scratch); } + +/* + * Prepare evaluation of a row pattern navigation (PREV/NEXT/FIRST/LAST). + * + * A navigation is compiled into EEOP_RPR_NAV_SET / EEOP_RPR_NAV_RESTORE steps + * instead of a function call: EEOP_RPR_NAV_SET swaps ecxt_outertuple to the + * target row, the argument expression is compiled normally and so reads from + * the swapped slot, and EEOP_RPR_NAV_RESTORE puts the original slot back. + */ +static void +ExecInitRPRNavExpr(ExprEvalStep *scratch, RPRNavExpr *nav, + ExprState *state, + Datum *resv, bool *resnull) +{ + WindowAggState *winstate; + RPRNavOffsets *entry; + RPRNavState *rprnavstate; + int skip_arg_step; + + Assert(state->parent && IsA(state->parent, WindowAggState)); + winstate = (WindowAggState *) state->parent; + + /* + * Parser guarantee (mirrors compute_matchStartDependent): nav's direct + * children are never RPRNavExpr -- compound nesting is flattened in place + * and any other nesting is rejected. Outer-kind dispatch is sufficient. + */ + Assert(nav->arg == NULL || !IsA(nav->arg, RPRNavExpr)); + Assert(nav->offset_arg == NULL || !IsA(nav->offset_arg, RPRNavExpr)); + Assert(nav->compound_offset_arg == NULL || + !IsA(nav->compound_offset_arg, RPRNavExpr)); + + entry = palloc0_object(RPRNavOffsets); + entry->nav = nav; + + if (nav->offset_arg != NULL) + entry->offset_state = ExecInitExpr(nav->offset_arg, state->parent); + + if (nav->compound_offset_arg != NULL) + entry->compound_offset_state = + ExecInitExpr(nav->compound_offset_arg, state->parent); + + /* + * The execution state the SET and RESTORE steps share. The offsets stay + * unset until resolve_nav_offsets() settles them for the scan. RESTORE + * copies a pass-by-reference result out of nav_slot, so it needs the + * result type's length and by-value flag; look them up once here rather + * than on every evaluation. + */ + rprnavstate = makeNode(RPRNavState); + rprnavstate->winstate = winstate; + rprnavstate->rprnavexpr = nav; + rprnavstate->offset.isnull = true; + rprnavstate->offset.value = (Datum) 0; + rprnavstate->compound_offset.isnull = true; + rprnavstate->compound_offset.value = (Datum) 0; + get_typlenbyval(nav->resulttype, + &rprnavstate->resulttyplen, + &rprnavstate->resulttypbyval); + + entry->rprnavstate = rprnavstate; + winstate->rprNavOffsets = lappend(winstate->rprNavOffsets, entry); + + /* Emit SET opcode: swap slot to target row */ + scratch->opcode = EEOP_RPR_NAV_SET; + scratch->d.rpr_nav.rprnavstate = rprnavstate; + ExprEvalPushStep(state, scratch); + + /* + * If the target row does not exist, skip evaluation of the argument + * expression and go straight to RESTORE. The EEOP_RPR_NAV_SET step + * writes a definitive resnull (false when the target row exists), so the + * jump condition is always up to date. + */ + skip_arg_step = state->steps_len; + scratch->opcode = EEOP_JUMP_IF_NULL; + scratch->resvalue = resv; + scratch->resnull = resnull; + scratch->d.jump.jumpdone = -1; /* set below */ + ExprEvalPushStep(state, scratch); + + /* Compile the argument expression normally */ + ExecInitExprRec(nav->arg, state, resv, resnull); + + /* out-of-range jump lands on the RESTORE step */ + state->steps[skip_arg_step].d.jump.jumpdone = state->steps_len; + + /* Emit RESTORE opcode: restore original slot */ + scratch->opcode = EEOP_RPR_NAV_RESTORE; + scratch->resvalue = resv; + scratch->resnull = resnull; + scratch->d.rpr_nav.rprnavstate = rprnavstate; + ExprEvalPushStep(state, scratch); +} diff --git a/src/backend/executor/nodeWindowAgg.c b/src/backend/executor/nodeWindowAgg.c index 01cec0f849..9af74f3703 100644 --- a/src/backend/executor/nodeWindowAgg.c +++ b/src/backend/executor/nodeWindowAgg.c @@ -261,11 +261,9 @@ static void update_reduced_frame(WindowObject winobj, int64 pos); /* Forward declarations - DEFINE row evaluation */ static bool rpr_prepare_row(WindowObject winobj, int64 pos, RPRVarMatch *varMatched); -static void build_define_offsets(WindowAggState *winstate, List *defineClause); +static void build_define_offsets(WindowAggState *winstate); static void resolve_nav_offsets(WindowAggState *winstate); static void resolve_one_nav(RPRNavOffsets *entry, EvalDefineOffsetsContext *context); -static bool nav_offsets_walker(Node *node, WindowAggState *winstate); -static void build_nav_offsets(RPRNavExpr *nav, WindowAggState *winstate); /* * Not null info bit array consists of 2-bit items @@ -3060,8 +3058,8 @@ ExecInitWindowAgg(WindowAgg *node, EState *estate, int eflags) /* * RPR stuff, in struct declaration order except for the nav offsets, - * which build_define_offsets() below accumulates into; the four - * NFALengthStats members keep the zeroes palloc0 gave them. + * which compiling the DEFINE clause below accumulates into; the four + * NFALengthStats members keep the zeroes makeNode gave them. */ if (node->rpPattern != NULL) { @@ -3080,14 +3078,6 @@ ExecInitWindowAgg(WindowAgg *node, EState *estate, int eflags) winstate->navFirstOffset = 0; winstate->navFirstOffsetKind = RPR_NAV_OFFSET_FIXED; - /* - * Must run this before the ExecInitQual() loop over defineClause: - * while compiling each RPRNavExpr, ExecInitQual() reads - * winstate->rprNavOffsets to link the RPRNavState to its entry and - * seed the offset, and this call is what fills that list - */ - build_define_offsets(winstate, node->defineClause); - /* * Compile DEFINE clause expressions. PREV/NEXT navigation is handled * by EEOP_RPR_NAV_SET/RESTORE opcodes emitted during ExecInitQual, so @@ -3116,6 +3106,8 @@ ExecInitWindowAgg(WindowAgg *node, EState *estate, int eflags) lappend(winstate->defineClauseExprs, exprstate); } + build_define_offsets(winstate); + /* Initialize NFA free lists for row pattern matching */ winstate->nfaContext = NULL; winstate->nfaContextTail = NULL; @@ -4123,98 +4115,22 @@ eval_nav_offset(WindowAggState *winstate, ExprState *estate, bool validate) return offset; } -/* - * build_nav_offsets - * Create the per-navigation offset bookkeeping entry at executor init and - * compile its offset argument expression(s). - * - * The offsets are not evaluated here: a PARAM_EXEC offset (function inlining or - * a LATERAL reference) has no value until the node is (re)scanned. The - * concrete value is resolved per scan by resolve_nav_offsets(), mirroring how - * calculate_frame_offsets() handles the window frame bounds. - */ -static void -build_nav_offsets(RPRNavExpr *nav, WindowAggState *winstate) -{ - RPRNavOffsets *entry = palloc0_object(RPRNavOffsets); - - /* - * Parser guarantee (mirrors compute_matchStartDependent): nav's direct - * children are never RPRNavExpr -- compound nesting is flattened in place - * and any other nesting is rejected. Outer-kind dispatch is sufficient. - */ - Assert(nav->arg == NULL || !IsA(nav->arg, RPRNavExpr)); - Assert(nav->offset_arg == NULL || !IsA(nav->offset_arg, RPRNavExpr)); - Assert(nav->compound_offset_arg == NULL || - !IsA(nav->compound_offset_arg, RPRNavExpr)); - - entry->nav = nav; - if (nav->offset_arg != NULL) - entry->offset_state = ExecInitExpr(nav->offset_arg, - (PlanState *) winstate); - if (nav->compound_offset_arg != NULL) - entry->compound_offset_state = ExecInitExpr(nav->compound_offset_arg, - (PlanState *) winstate); - - /* - * Own the execution state of the compiled navigation. ExecInitExprRec() - * runs after this and reaches the entry by nav->navno; the offsets stay - * unset until resolve_nav_offsets() settles them for the scan. - */ - entry->rprnavstate = makeNode(RPRNavState); - entry->rprnavstate->winstate = winstate; - entry->rprnavstate->rprnavexpr = nav; - entry->rprnavstate->offset.isnull = true; - entry->rprnavstate->offset.value = (Datum) 0; - entry->rprnavstate->compound_offset.isnull = true; - entry->rprnavstate->compound_offset.value = (Datum) 0; - - winstate->rprNavOffsets = lappend(winstate->rprNavOffsets, entry); -} - -static bool -nav_offsets_walker(Node *node, WindowAggState *winstate) -{ - if (node == NULL) - return false; - if (IsA(node, RPRNavExpr)) - build_nav_offsets(castNode(RPRNavExpr, node), winstate); - - return expression_tree_walker(node, nav_offsets_walker, winstate); -} - /* * build_define_offsets - * At executor init, create one RPRNavOffsets entry per navigation in the - * DEFINE clause and compile its offset argument expressions. + * Resolve the navigation offsets that are already constant at plan time. * - * Entries are appended in walk order, the order compute_define_metadata() - * numbered them in, so entry i is the navigation with navno i. - * - * The concrete offset values -- and the tuplestore trim bounds derived from - * them -- are resolved later, per scan, by resolve_nav_offsets(). Only an RPR - * window reaches here. + * Runs at executor init, once compiling the DEFINE clause has registered every + * navigation, so that EXPLAIN (which never executes, hence never reaches + * resolve_nav_offsets()) shows the real trim bounds. A parameterized offset + * (PARAM_EXTERN under a generic plan, or a PARAM_EXEC) has no value yet and + * is left for resolve_nav_offsets() to bound per scan. Only an RPR window + * reaches here. */ static void -build_define_offsets(WindowAggState *winstate, List *defineClause) +build_define_offsets(WindowAggState *winstate) { EvalDefineOffsetsContext ctx; - /* DEFINE is mandatory, so an RPR window always has a clause */ - Assert(defineClause != NIL); - - foreach_node(TargetEntry, te, defineClause) - { - nav_offsets_walker((Node *) te->expr, winstate); - } - - /* - * Resolve the offsets that are already constant at plan time, so EXPLAIN - * (which never executes, hence never reaches resolve_nav_offsets()) shows - * the real trim bounds. A parameterized offset (PARAM_EXTERN under a - * generic plan, or a PARAM_EXEC) has no value yet and is left for - * resolve_nav_offsets() to bound per scan. - */ ctx.winstate = winstate; ctx.maxOffset = 0; ctx.maxOverflow = false; diff --git a/src/backend/optimizer/plan/createplan.c b/src/backend/optimizer/plan/createplan.c index a4bc5709a6..ae510f9cc7 100644 --- a/src/backend/optimizer/plan/createplan.c +++ b/src/backend/optimizer/plan/createplan.c @@ -2474,9 +2474,6 @@ create_minmaxagg_plan(PlannerInfo *root, MinMaxAggPath *best_path) * * The walk classifies one thing: which DEFINE variables depend on the match * start, which is what buildRPRPattern() needs to decide context absorption. - * The trim offsets are not plan-time metadata; the executor records them at - * init (build_define_offsets) and settles their values per scan - * (resolve_nav_offsets), both in nodeWindowAgg.c. * * The driver sets curVarIdx to the index of the variable being walked before * each invocation; the walker uses it to populate matchStartDependent. @@ -2484,7 +2481,6 @@ create_minmaxagg_plan(PlannerInfo *root, MinMaxAggPath *best_path) typedef struct DefineMetadataContext { int curVarIdx; /* DEFINE variable currently being walked */ - int navno; /* next RPRNavExpr.navno to assign */ Bitmapset *matchStartDependent; /* variables that depend on * match_start */ } DefineMetadataContext; @@ -2539,20 +2535,14 @@ define_metadata_walker(Node *node, DefineMetadataContext *ctx) if (node == NULL) return false; if (IsA(node, RPRNavExpr)) - { - RPRNavExpr *nav = castNode(RPRNavExpr, node); - - nav->navno = ctx->navno++; - compute_matchStartDependent(nav, ctx); - } + compute_matchStartDependent(castNode(RPRNavExpr, node), ctx); return expression_tree_walker(node, define_metadata_walker, ctx); } /* * compute_define_metadata - * Classify which DEFINE variables depend on the match start, and number - * the navigations. + * Classify which DEFINE variables depend on the match start. * * Walks each DEFINE variable expression once and returns the set of variable * indices whose navigation reaches match_start: those containing FIRST or a @@ -2561,13 +2551,10 @@ define_metadata_walker(Node *node, DefineMetadataContext *ctx) * variables require per-context re-evaluation during NFA processing, and * their presence disqualifies the pattern from context absorption. * - * The same walk assigns RPRNavExpr.navno in visit order, which is the order - * the executor builds WindowAggState.rprNavOffsets in. - * - * Navigation offsets for tuplestore trim are not computed here; they are - * built at executor init (build_define_offsets) and settled per scan - * (resolve_nav_offsets), which can evaluate non-constant offsets that the - * planner cannot fold. + * Navigation offsets for tuplestore trim are not computed here; the executor + * records them as it compiles each navigation (ExecInitRPRNavExpr) and settles + * them per scan (resolve_nav_offsets), which can evaluate non-constant + * offsets that the planner cannot fold. */ static void compute_define_metadata(List *defineClause, Bitmapset **matchStartDependent) @@ -2575,7 +2562,6 @@ compute_define_metadata(List *defineClause, Bitmapset **matchStartDependent) DefineMetadataContext ctx; ctx.curVarIdx = 0; - ctx.navno = 0; ctx.matchStartDependent = NULL; foreach_node(TargetEntry, te, defineClause) diff --git a/src/backend/parser/parse_func.c b/src/backend/parser/parse_func.c index e688009e6b..7ebaa652cb 100644 --- a/src/backend/parser/parse_func.c +++ b/src/backend/parser/parse_func.c @@ -2265,7 +2265,6 @@ ParseRPRNavCall(ParseState *pstate, List *funcname, List *fargs, navexpr = makeNode(RPRNavExpr); navexpr->kind = kind; navexpr->arg = (Expr *) arg; - navexpr->navno = -1; /* assigned while planning */ /* an explicit offset is coerced to int8, which the executor reads */ if (nargs == 2) diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h index 40cf83b235..7f18b4010a 100644 --- a/src/include/nodes/execnodes.h +++ b/src/include/nodes/execnodes.h @@ -1060,10 +1060,10 @@ typedef struct RPRNavState RPRNavExpr *rprnavexpr; /* - * Resolved navigation offsets for this execution, captured from - * winstate->rprNavOffsets at expression compile time. These live in - * executor state (not on the RPRNavExpr) because plan trees are read-only - * and may be shared by concurrent executions. + * Resolved navigation offsets for this execution, settled per scan by + * resolve_nav_offsets(). These live in executor state (not on the + * RPRNavExpr) because plan trees are read-only and may be shared by + * concurrent executions. */ NullableDatum offset; /* inner offset */ NullableDatum compound_offset; /* outer offset for compound nav */ @@ -1087,8 +1087,8 @@ typedef enum RPRNavOffsetKind * RPRNavOffsets - one entry of WindowAggState.rprNavOffsets * * Associates an RPRNavExpr from the (read-only) plan tree with its offsets, - * built by build_define_offsets() at executor startup and settled once per - * scan by resolve_nav_offsets(). The list is in RPRNavExpr.navno order. + * registered by ExecInitRPRNavExpr() as the DEFINE clause is compiled and + * settled once per scan by resolve_nav_offsets(). */ typedef struct RPRNavOffsets { @@ -2770,8 +2770,8 @@ typedef struct WindowAggState /* RPR navigation */ /* - * per-execution resolved nav offsets: list of RPRNavOffsets, indexed by - * RPRNavExpr.navno; built by build_define_offsets() + * per-execution resolved nav offsets: list of RPRNavOffsets, one per + * navigation. */ List *rprNavOffsets; bool navResolvePending; /* nav offsets need (re)resolving at the diff --git a/src/include/nodes/primnodes.h b/src/include/nodes/primnodes.h index b92c604e93..e7865685ea 100644 --- a/src/include/nodes/primnodes.h +++ b/src/include/nodes/primnodes.h @@ -692,9 +692,6 @@ typedef struct RPRNavExpr Expr *compound_offset_arg; /* outer offset for compound nav, or * NULL for its default */ - /* unique ID within the WindowAgg; -1 until the planner assigns it */ - int navno pg_node_attr(query_jumble_ignore); - /* result type (same as arg's type) */ Oid resulttype pg_node_attr(query_jumble_ignore); /* OID of collation of result */ -- 2.34.1
From d05704cb8025c06dbb577c27de7371142aa7f01f Mon Sep 17 00:00:00 2001 From: jian he <[email protected]> Date: Sat, 17 Oct 2026 01:18:27 +0800 Subject: [PATCH v54 2/2] Remove compute_define_metadata compute_define_metadata() is complicated for what it computes: a context struct, a current-variable cursor, a separate classification function and an out-parameter, all of these is just to find which DEFINE variables depend on the match start. A simple expression walker can do this. Based on https://github.com/assam258-5892/postgres/tree/RPR-20260930 --- src/backend/executor/README.rpr | 2 +- src/backend/executor/execExpr.c | 5 - src/backend/optimizer/plan/createplan.c | 137 +++++++++--------------- src/tools/pgindent/typedefs.list | 1 - 4 files changed, 50 insertions(+), 95 deletions(-) diff --git a/src/backend/executor/README.rpr b/src/backend/executor/README.rpr index 4d5dadcc57..8b6b263308 100644 --- a/src/backend/executor/README.rpr +++ b/src/backend/executor/README.rpr @@ -324,7 +324,7 @@ LAST over any navigation, PREV or NEXT over PREV or NEXT, an inner navigation that is only part of the argument, as in PREV(v + FIRST(v)), and a third level. A navigation inside an offset is rejected as well. Once step (4) is done, no RPRNavExpr contains another one anywhere; the planner -relies on this (compute_matchStartDependent() asserts it), and the deparser +relies on this (contain_match_start_nav_walker() asserts it), and the deparser rebuilds the nested spelling from the compound kind. Variables that are used in PATTERN but not defined in DEFINE are implicitly diff --git a/src/backend/executor/execExpr.c b/src/backend/executor/execExpr.c index 4bfe6ca95d..391348d216 100644 --- a/src/backend/executor/execExpr.c +++ b/src/backend/executor/execExpr.c @@ -5098,11 +5098,6 @@ ExecInitRPRNavExpr(ExprEvalStep *scratch, RPRNavExpr *nav, Assert(state->parent && IsA(state->parent, WindowAggState)); winstate = (WindowAggState *) state->parent; - /* - * Parser guarantee (mirrors compute_matchStartDependent): nav's direct - * children are never RPRNavExpr -- compound nesting is flattened in place - * and any other nesting is rejected. Outer-kind dispatch is sufficient. - */ Assert(nav->arg == NULL || !IsA(nav->arg, RPRNavExpr)); Assert(nav->offset_arg == NULL || !IsA(nav->offset_arg, RPRNavExpr)); Assert(nav->compound_offset_arg == NULL || diff --git a/src/backend/optimizer/plan/createplan.c b/src/backend/optimizer/plan/createplan.c index ae510f9cc7..6991706791 100644 --- a/src/backend/optimizer/plan/createplan.c +++ b/src/backend/optimizer/plan/createplan.c @@ -2469,109 +2469,65 @@ create_minmaxagg_plan(PlannerInfo *root, MinMaxAggPath *best_path) return plan; } -/* - * DefineMetadataContext - context for the DEFINE clause walk below. - * - * The walk classifies one thing: which DEFINE variables depend on the match - * start, which is what buildRPRPattern() needs to decide context absorption. - * - * The driver sets curVarIdx to the index of the variable being walked before - * each invocation; the walker uses it to populate matchStartDependent. - */ -typedef struct DefineMetadataContext -{ - int curVarIdx; /* DEFINE variable currently being walked */ - Bitmapset *matchStartDependent; /* variables that depend on - * match_start */ -} DefineMetadataContext; - -/* - * compute_matchStartDependent - * - * per-variable match_start dependency for absorption suppression: outer nav - * kinds that reach match_start (FIRST, LAST-with-offset, PREV_FIRST, - * NEXT_FIRST, PREV_LAST/NEXT_LAST-with-offset) add curVarIdx to - * matchStartDependent. - * - * Classification uses only the outer nav kind: parser nesting restrictions - * prevent FIRST/LAST inside a PREV/NEXT value subexpression. - */ -static void -compute_matchStartDependent(RPRNavExpr *nav, DefineMetadataContext *context) -{ - /* - * Parser guarantee: by the time the planner sees a DEFINE expression, - * compound nesting has been flattened into a single RPRNavExpr and any - * other RPRNavExpr nesting has been rejected. So nav's direct child - * fields are not themselves RPRNavExpr nodes, and outer-kind dispatch - * below is sufficient. - */ - Assert(nav->arg == NULL || !IsA(nav->arg, RPRNavExpr)); - Assert(nav->offset_arg == NULL || !IsA(nav->offset_arg, RPRNavExpr)); - Assert(nav->compound_offset_arg == NULL || - !IsA(nav->compound_offset_arg, RPRNavExpr)); - - /* - * Match-start dependency: classify the outer nav kind. A constant - * LAST(x, 0) is conservatively included (offset_arg is a non-NULL Const), - * causing a harmless extra re-evaluation; since LAST(x, 0) is the current - * row, its result is independent of the match start. - */ - if (nav->kind == RPR_NAV_FIRST || - (nav->kind == RPR_NAV_LAST && nav->offset_arg != NULL) || - nav->kind == RPR_NAV_PREV_FIRST || - nav->kind == RPR_NAV_NEXT_FIRST || - ((nav->kind == RPR_NAV_PREV_LAST || - nav->kind == RPR_NAV_NEXT_LAST) && - nav->offset_arg != NULL)) - context->matchStartDependent = - bms_add_member(context->matchStartDependent, - context->curVarIdx); -} - static bool -define_metadata_walker(Node *node, DefineMetadataContext *ctx) +contain_match_start_nav_walker(Node *node, void *context) { if (node == NULL) return false; if (IsA(node, RPRNavExpr)) - compute_matchStartDependent(castNode(RPRNavExpr, node), ctx); + { + RPRNavExpr *nav = castNode(RPRNavExpr, node); + + /* + * Parser guarantee: by the time the planner sees a DEFINE expression, + * compound nesting has been flattened into a single RPRNavExpr and + * any other RPRNavExpr nesting has been rejected. So nav's direct + * child fields are not themselves RPRNavExpr nodes, and outer-kind + * dispatch below is sufficient. + */ + Assert(nav->arg == NULL || !IsA(nav->arg, RPRNavExpr)); + Assert(nav->offset_arg == NULL || !IsA(nav->offset_arg, RPRNavExpr)); + Assert(nav->compound_offset_arg == NULL || + !IsA(nav->compound_offset_arg, RPRNavExpr)); - return expression_tree_walker(node, define_metadata_walker, ctx); + /* + * Match-start dependency: classify the outer nav kind. A constant + * LAST(x, 0) is conservatively included (offset_arg is a non-NULL + * Const), causing a harmless extra re-evaluation; since LAST(x, 0) is + * the current row, its result is independent of the match start. + */ + if (nav->offset_arg != NULL) + { + if (nav->kind == RPR_NAV_PREV_LAST || + nav->kind == RPR_NAV_NEXT_LAST || + nav->kind == RPR_NAV_LAST) + return true; + } + + if (nav->kind == RPR_NAV_FIRST || + nav->kind == RPR_NAV_PREV_FIRST || + nav->kind == RPR_NAV_NEXT_FIRST) + return true; + } + + return expression_tree_walker(node, + contain_match_start_nav_walker, + context); } /* - * compute_define_metadata - * Classify which DEFINE variables depend on the match start. - * - * Walks each DEFINE variable expression once and returns the set of variable - * indices whose navigation reaches match_start: those containing FIRST or a - * compound PREV_FIRST/NEXT_FIRST, or a LAST that carries an offset of its - * own, whether plain or inside a compound PREV_LAST/NEXT_LAST. Such - * variables require per-context re-evaluation during NFA processing, and - * their presence disqualifies the pattern from context absorption. + * contain_match_start_nav + * Does the expression contain a navigation that depends on the match start? * * Navigation offsets for tuplestore trim are not computed here; the executor * records them as it compiles each navigation (ExecInitRPRNavExpr) and settles * them per scan (resolve_nav_offsets), which can evaluate non-constant * offsets that the planner cannot fold. */ -static void -compute_define_metadata(List *defineClause, Bitmapset **matchStartDependent) +static bool +contain_match_start_nav(Node *node) { - DefineMetadataContext ctx; - - ctx.curVarIdx = 0; - ctx.matchStartDependent = NULL; - - foreach_node(TargetEntry, te, defineClause) - { - ctx.curVarIdx = foreach_current_index(te); - - define_metadata_walker((Node *) te->expr, &ctx); - } - - *matchStartDependent = ctx.matchStartDependent; + return contain_match_start_nav_walker(node, NULL); } /* @@ -2658,7 +2614,12 @@ create_windowagg_plan(PlannerInfo *root, WindowAggPath *best_path) * absorption suppression in buildRPRPattern). Nav offsets for * tuplestore trim are built at executor init and resolved per scan. */ - compute_define_metadata(wc->defineClause, &matchStartDependent); + foreach_node(TargetEntry, te, wc->defineClause) + { + if (contain_match_start_nav((Node *) te->expr)) + matchStartDependent = bms_add_member(matchStartDependent, + foreach_current_index(te)); + } /* Compile and optimize RPR patterns */ compiledPattern = buildRPRPattern(wc->rpPattern, diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list index 3ab9605230..709333f4d6 100644 --- a/src/tools/pgindent/typedefs.list +++ b/src/tools/pgindent/typedefs.list @@ -664,7 +664,6 @@ DecodingWorkerShared DefElem DefElemAction DefaultACLInfo -DefineMetadataContext DefinePhase DefineStmt DefineWalkCtx -- 2.34.1
