From cf4ddcf56a9c6fd415c9b24dc36a869d9f4533eb Mon Sep 17 00:00:00 2001 From: jian he Date: Sat, 11 Jul 2026 15:34:13 +0800 Subject: [PATCH v51 3/3] Introduce execution struct RprNavState MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Storing the resolved offset values (offset, compound_offset) on RPRNavExpr, next to the offset expressions they are computed from (offset_arg, compound_offset_arg), seems wrong to me: the expressions are plan-tree fields, but the resolved values are per-execution results, and the executor must treat the plan tree as read-only. Compare how FOR PORTION OF handles this in ExecInitModifyTable: ``` exprState = ExecPrepareExpr((Expr *) forPortionOf->targetRange, estate); targetRange = ExecEvalExpr(exprState, econtext, &isNull); /* Create state for FOR PORTION OF operation */ fpoState = makeNode(ForPortionOfState); fpoState->fp_targetRange = targetRange; ``` The expression is evaluated at executor startup, and the resulting value is stored in an executor-state struct. The plan node is never modified. We should do the same: evaluate the offset expressions at executor startup and store the resulting values in executor state. So in v51-0003 I added a new executor-state struct, RprNavState, which captures everything the evaluation code in execExprInterp.c needs: WindowAggState, RPRNavExpr, the resolved offset and compound_offset, and the result type info (resulttyplen, resulttypbyval). Keeping such per-node evaluation state in an out-of-line struct, with the ExprEvalStep holding just a pointer to it, follows an existing convention in the expression evaluation code — see JsonExprState for a similar example. Based on: https://github.com/assam258-5892/postgres/commits/RPR Discussion: https://postgr.es/m/ --- src/backend/executor/execExpr.c | 41 +++++++++++++++++---- src/backend/executor/execExprInterp.c | 27 +++++++------- src/backend/executor/nodeWindowAgg.c | 10 ++++-- src/backend/optimizer/util/clauses.c | 2 -- src/include/executor/execExpr.h | 8 +---- src/include/nodes/execnodes.h | 42 ++++++++++++++++++++++ src/include/nodes/primnodes.h | 12 ------- src/test/regress/expected/rpr_base.out | 49 ++++++++++++++++++++++++++ src/test/regress/sql/rpr_base.sql | 38 ++++++++++++++++++++ src/tools/pgindent/typedefs.list | 2 ++ 10 files changed, 189 insertions(+), 42 deletions(-) diff --git a/src/backend/executor/execExpr.c b/src/backend/executor/execExpr.c index 80794fe47c..0bd63419cc 100644 --- a/src/backend/executor/execExpr.c +++ b/src/backend/executor/execExpr.c @@ -36,6 +36,7 @@ #include "catalog/pg_type.h" #include "executor/execExpr.h" #include "executor/nodeSubplan.h" +#include "executor/nodeWindowAgg.h" #include "funcapi.h" #include "jit/jit.h" #include "miscadmin.h" @@ -1199,19 +1200,45 @@ ExecInitExprRec(Expr *node, ExprState *state, * expression is compiled normally (reads from the swapped * slot), and the RESTORE opcode restores the original slot. */ + RprNavState *rprnavstate = makeNode(RprNavState); RPRNavExpr *nav = (RPRNavExpr *) node; WindowAggState *winstate; int skip_arg_step; + bool find_navexpr = false; Assert(state->parent && IsA(state->parent, WindowAggState)); winstate = (WindowAggState *) state->parent; + rprnavstate->winstate = winstate; + rprnavstate->rprnavexpr = nav; + + /* + * Fetch the resolved navigation offsets for a given + * RPRNavExpr, as computed by eval_define_offsets() at + * executor startup. Called from ExecInitExprRec when + * compiling a navigation expression; the values are captured + * into its RprNavState. + * + * The offsets live in executor state rather than on the + * RPRNavExpr because the plan tree is read-only and may be + * shared by concurrent executions. + */ + foreach_ptr(RprNavOffsets, entry, winstate->rprNavOffsets) + { + if (entry->nav == nav && find_navexpr) + elog(ERROR, "RPRNavExpr occruence more than once"); + + if (entry->nav == nav) + { + rprnavstate->offset = entry->offset; + rprnavstate->compound_offset = entry->compound_offset; + find_navexpr = true; + } + } + /* Emit SET opcode: swap slot to target row */ scratch.opcode = EEOP_RPR_NAV_SET; - scratch.d.rpr_nav.winstate = winstate; - scratch.d.rpr_nav.kind = nav->kind; - scratch.d.rpr_nav.offset = nav->offset; - scratch.d.rpr_nav.compound_offset = nav->compound_offset; + scratch.d.rpr_nav.rprnavstate = rprnavstate; ExprEvalPushStep(state, &scratch); @@ -1240,10 +1267,10 @@ ExecInitExprRec(Expr *node, ExprState *state, scratch.opcode = EEOP_RPR_NAV_RESTORE; scratch.resvalue = resv; scratch.resnull = resnull; - scratch.d.rpr_nav.winstate = winstate; + scratch.d.rpr_nav.rprnavstate = rprnavstate; get_typlenbyval(nav->resulttype, - &scratch.d.rpr_nav.resulttyplen, - &scratch.d.rpr_nav.resulttypbyval); + &rprnavstate->resulttyplen, + &rprnavstate->resulttypbyval); ExprEvalPushStep(state, &scratch); break; } diff --git a/src/backend/executor/execExprInterp.c b/src/backend/executor/execExprInterp.c index 3901fa2745..75aec9f044 100644 --- a/src/backend/executor/execExprInterp.c +++ b/src/backend/executor/execExprInterp.c @@ -6024,23 +6024,26 @@ ExecAggPlainTransByRef(AggState *aggstate, AggStatePerTrans pertrans, void ExecEvalRPRNavSet(ExprState *state, ExprEvalStep *op, ExprContext *econtext) { - WindowAggState *winstate = op->d.rpr_nav.winstate; + WindowAggState *winstate; int64 offset; int64 compound_offset; int64 target_pos; TupleTableSlot *target_slot; + RprNavState *rprnavstate = op->d.rpr_nav.rprnavstate; + + winstate = rprnavstate->winstate; /* Save current slot for later restore */ winstate->nav_saved_outertuple = econtext->ecxt_outertuple; - offset = op->d.rpr_nav.offset; - compound_offset = op->d.rpr_nav.compound_offset; + offset = rprnavstate->offset; + compound_offset = rprnavstate->compound_offset; /* * Calculate target position based on navigation direction. On overflow, * use -1 so that ExecRPRNavGetSlot treats it as out of range. */ - switch (op->d.rpr_nav.kind) + switch (rprnavstate->rprnavexpr->kind) { case RPR_NAV_PREV: @@ -6089,7 +6092,7 @@ ExecEvalRPRNavSet(ExprState *state, ExprEvalStep *op, ExprContext *econtext) } /* Apply outer: PREV subtracts, NEXT adds */ - if (op->d.rpr_nav.kind == RPR_NAV_PREV_FIRST) + if (rprnavstate->rprnavexpr->kind == RPR_NAV_PREV_FIRST) { /* * inner_pos is in [0, currentpos] and compound_offset is @@ -6124,7 +6127,7 @@ ExecEvalRPRNavSet(ExprState *state, ExprEvalStep *op, ExprContext *econtext) } /* Apply outer: PREV subtracts, NEXT adds */ - if (op->d.rpr_nav.kind == RPR_NAV_PREV_LAST) + if (rprnavstate->rprnavexpr->kind == RPR_NAV_PREV_LAST) { /* * inner_pos is in [nav_match_start, currentpos] (>= 0) @@ -6143,7 +6146,7 @@ ExecEvalRPRNavSet(ExprState *state, ExprEvalStep *op, ExprContext *econtext) break; default: elog(ERROR, "unrecognized RPR navigation kind: %d", - (int) op->d.rpr_nav.kind); + (int) rprnavstate->rprnavexpr->kind); break; } @@ -6168,8 +6171,8 @@ ExecEvalRPRNavSet(ExprState *state, ExprEvalStep *op, ExprContext *econtext) /* * If the target row does exist, resnull must still be set to a definitive * false, since it may hold a stale value from a previous evaluation and - * the jump step tests it before the argument expression overwrites it. -1 */ + * the jump step tests it before the argument expression overwrites it. 1 + */ if (target_slot == winstate->nav_null_slot) { *op->resvalue = (Datum) 0; @@ -6205,7 +6208,7 @@ void ExecEvalRPRNavRestore(ExprState *state, ExprEvalStep *op, ExprContext *econtext) { - WindowAggState *winstate = op->d.rpr_nav.winstate; + WindowAggState *winstate = op->d.rpr_nav.rprnavstate->winstate; /* * When slot swap was elided (target == currentpos), this is a harmless @@ -6217,7 +6220,7 @@ ExecEvalRPRNavRestore(ExprState *state, ExprEvalStep *op, econtext->ecxt_outertuple = winstate->nav_saved_outertuple; /* Stabilize pass-by-ref result against nav_slot re-fetch */ - if (!op->d.rpr_nav.resulttypbyval && + if (!op->d.rpr_nav.rprnavstate->resulttypbyval && !*op->resnull) { MemoryContext oldContext; @@ -6225,7 +6228,7 @@ ExecEvalRPRNavRestore(ExprState *state, ExprEvalStep *op, oldContext = MemoryContextSwitchTo(econtext->ecxt_per_tuple_memory); *op->resvalue = datumCopy(*op->resvalue, false, - op->d.rpr_nav.resulttyplen); + op->d.rpr_nav.rprnavstate->resulttyplen); MemoryContextSwitchTo(oldContext); } } diff --git a/src/backend/executor/nodeWindowAgg.c b/src/backend/executor/nodeWindowAgg.c index 3f6e8f54a2..b79ecc1a91 100644 --- a/src/backend/executor/nodeWindowAgg.c +++ b/src/backend/executor/nodeWindowAgg.c @@ -4043,6 +4043,7 @@ compute_nav_offsets(RPRNavExpr *nav, EvalDefineOffsetsContext *context) { int64 inner; int64 outer; + RprNavOffsets *entry = palloc_object(RprNavOffsets); /* * Parser guarantee (mirrors compute_matchStartDependent): nav's direct @@ -4068,8 +4069,12 @@ compute_nav_offsets(RPRNavExpr *nav, EvalDefineOffsetsContext *context) else outer = 1; - nav->offset = inner; - nav->compound_offset = outer; + entry->nav = nav; + entry->offset = inner; + entry->compound_offset = outer; + + context->winstate->rprNavOffsets = + lappend(context->winstate->rprNavOffsets, entry); /* Backward reach: PREV, LAST-with-offset */ if (!context->maxOverflow) @@ -4156,6 +4161,7 @@ eval_define_offsets(WindowAggState *winstate, List *defineClause) winstate->navMaxOffset = 0; winstate->hasFirstNav = false; winstate->navFirstOffset = 0; + winstate->rprNavOffsets = NIL; if (defineClause == NIL) return; diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c index c13376e987..c4320b4f44 100644 --- a/src/backend/optimizer/util/clauses.c +++ b/src/backend/optimizer/util/clauses.c @@ -2868,8 +2868,6 @@ eval_const_expressions_mutator(Node *node, newexpr->arg = arg; newexpr->offset_arg = offset_arg; newexpr->compound_offset_arg = compound_offset_arg; - newexpr->offset = expr->offset; - newexpr->compound_offset = expr->compound_offset; newexpr->resulttype = expr->resulttype; newexpr->resultcollid = expr->resultcollid; newexpr->location = expr->location; diff --git a/src/include/executor/execExpr.h b/src/include/executor/execExpr.h index 0561fd8f40..f63b9e22a7 100644 --- a/src/include/executor/execExpr.h +++ b/src/include/executor/execExpr.h @@ -702,13 +702,7 @@ typedef struct ExprEvalStep /* for EEOP_RPR_NAV_SET / EEOP_RPR_NAV_RESTORE */ struct { - WindowAggState *winstate; - RPRNavKind kind; - int64 offset; - int64 compound_offset; - /* For compound nav: offset_value[0] = inner, [1] = outer */ - int16 resulttyplen; /* RESTORE: result type length */ - bool resulttypbyval; /* RESTORE: result pass-by-value? */ + RprNavState *rprnavstate; } rpr_nav; /* for EEOP_AGG_*DESERIALIZE */ diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h index ad1371862a..a8274fb4a6 100644 --- a/src/include/nodes/execnodes.h +++ b/src/include/nodes/execnodes.h @@ -70,6 +70,7 @@ typedef struct TupleTableSlot TupleTableSlot; typedef struct TupleTableSlotOps TupleTableSlotOps; typedef struct WalUsage WalUsage; typedef struct WorkerNodeInstrumentation WorkerNodeInstrumentation; +typedef struct WindowAggState WindowAggState; /* ---------------- @@ -1074,6 +1075,41 @@ typedef struct SubPlanState ExprState *cur_eq_comp; /* equality comparator for LHS vs. table */ } SubPlanState; +typedef struct RprNavState +{ + NodeTag type; + + WindowAggState *winstate; + 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. + */ + int64 offset; /* inner offset */ + int64 compound_offset; /* outer offset for compound nav */ + + int16 resulttyplen; /* RESTORE: result type length */ + bool resulttypbyval; /* RESTORE: result pass-by-value? */ +} RprNavState; + +/* + * RprNavOffsets - one entry of WindowAggState.rprNavOffsets + * + * Associates an RPRNavExpr from the (read-only) plan tree with its offsets + * as resolved by eval_define_offsets() at executor startup. The plan node + * pointer serves as lookup key; ExecInitExprRec copies the values into the + * RprNavState of each compiled navigation expression. + */ +typedef struct RprNavOffsets +{ + RPRNavExpr *nav; /* plan-tree node (lookup key) */ + int64 offset; /* resolved inner offset */ + int64 compound_offset; /* resolved outer offset */ +} RprNavOffsets; + /* * DomainConstraintState - one item to check during CoerceToDomain * @@ -2726,6 +2762,12 @@ typedef struct WindowAggState TupleTableSlot *temp_slot_2; /* RPR navigation */ + + /* + * per-execution resolved nav offsets: list of RprNavOffsets, keyed by + * RPRNavExpr pointer; built by eval_define_offsets() + */ + List *rprNavOffsets; int64 navMaxOffset; /* max backward nav offset, -1 means infinite * offset, retain all */ bool hasFirstNav; /* FIRST() present in DEFINE */ diff --git a/src/include/nodes/primnodes.h b/src/include/nodes/primnodes.h index a6903b300f..e9c3bcdc91 100644 --- a/src/include/nodes/primnodes.h +++ b/src/include/nodes/primnodes.h @@ -690,18 +690,6 @@ typedef struct RPRNavExpr /* outer offset for compound navigation */ Expr *compound_offset_arg; - /* - * The computed offset value, for default value see RPRNavKind comments - * above. The value will be set in visit_nav_exec. - */ - int64 offset; - - /* - * The computeed compound_offset offset value, for default value see - * RPRNavKind comments above. The value will be set in visit_nav_exec. - */ - int64 compound_offset; - /* result type (same as arg's type) */ Oid resulttype pg_node_attr(query_jumble_ignore); /* OID of collation of result */ diff --git a/src/test/regress/expected/rpr_base.out b/src/test/regress/expected/rpr_base.out index 9da3141eef..70b0847e1a 100644 --- a/src/test/regress/expected/rpr_base.out +++ b/src/test/regress/expected/rpr_base.out @@ -1721,6 +1721,55 @@ DETAIL: Pattern has 32768 elements, maximum is 32767. -- ============================================================ -- Navigation Functions Tests (PREV / NEXT / FIRST / LAST) -- ============================================================ +CREATE TEMP TABLE rpr_nav0 (id int, v int); +INSERT INTO rpr_nav0 SELECT g, g*10 FROM generate_series(1, 5) g; +-- Two concurrently open portals of the SAME cached generic plan, with different +-- offset parameters. +-- +-- The parameterized cursor 'c' compiles to one plpgsql statement -> one SPI +-- cached plan. The recursive call OPENs a second portal of that same plan +-- (with a different offset) while the outer portal is already started but has +-- not yet FETCHed. +CREATE OR REPLACE FUNCTION rpr_nested(p_off int, depth int) +RETURNS SETOF text LANGUAGE plpgsql AS $$ +DECLARE + c CURSOR (o int) FOR + SELECT id, count(*) OVER w AS cnt + FROM rpr_nav0 + WINDOW w AS (ORDER BY id + ROWS BETWEEN CURRENT ROW AND UNBOUNDED FOLLOWING + PATTERN (A) + DEFINE A AS PREV(v, o) IS NULL); + r record; +BEGIN + OPEN c(p_off); + IF depth > 0 THEN + RETURN QUERY SELECT * FROM rpr_nested(p_off + 2, depth - 1); + END IF; + + LOOP + FETCH c INTO r; + EXIT WHEN NOT FOUND; + RETURN NEXT format('off=%s id=%s cnt=%s', p_off, r.id, r.cnt); + END LOOP; + CLOSE c; +END $$; +SET plan_cache_mode = force_generic_plan; +SELECT * FROM rpr_nested(1, 1); + rpr_nested +------------------ + off=3 id=1 cnt=1 + off=3 id=2 cnt=1 + off=3 id=3 cnt=1 + off=3 id=4 cnt=0 + off=3 id=5 cnt=0 + off=1 id=1 cnt=1 + off=1 id=2 cnt=0 + off=1 id=3 cnt=0 + off=1 id=4 cnt=0 + off=1 id=5 cnt=0 +(10 rows) + CREATE TABLE rpr_nav (id INT, val INT); INSERT INTO rpr_nav VALUES (1, 10), (2, 20), (3, 15), (4, 25), (5, 30); diff --git a/src/test/regress/sql/rpr_base.sql b/src/test/regress/sql/rpr_base.sql index 274bcbebf1..9e27c886cb 100644 --- a/src/test/regress/sql/rpr_base.sql +++ b/src/test/regress/sql/rpr_base.sql @@ -1268,6 +1268,44 @@ SELECT format($$SELECT count(*) OVER w FROM (SELECT 1 i) t -- ============================================================ -- Navigation Functions Tests (PREV / NEXT / FIRST / LAST) -- ============================================================ +CREATE TEMP TABLE rpr_nav0 (id int, v int); +INSERT INTO rpr_nav0 SELECT g, g*10 FROM generate_series(1, 5) g; + +-- Two concurrently open portals of the SAME cached generic plan, with different +-- offset parameters. +-- +-- The parameterized cursor 'c' compiles to one plpgsql statement -> one SPI +-- cached plan. The recursive call OPENs a second portal of that same plan +-- (with a different offset) while the outer portal is already started but has +-- not yet FETCHed. +CREATE OR REPLACE FUNCTION rpr_nested(p_off int, depth int) +RETURNS SETOF text LANGUAGE plpgsql AS $$ +DECLARE + c CURSOR (o int) FOR + SELECT id, count(*) OVER w AS cnt + FROM rpr_nav0 + WINDOW w AS (ORDER BY id + ROWS BETWEEN CURRENT ROW AND UNBOUNDED FOLLOWING + PATTERN (A) + DEFINE A AS PREV(v, o) IS NULL); + r record; +BEGIN + OPEN c(p_off); + IF depth > 0 THEN + RETURN QUERY SELECT * FROM rpr_nested(p_off + 2, depth - 1); + END IF; + + LOOP + FETCH c INTO r; + EXIT WHEN NOT FOUND; + RETURN NEXT format('off=%s id=%s cnt=%s', p_off, r.id, r.cnt); + END LOOP; + CLOSE c; +END $$; + +SET plan_cache_mode = force_generic_plan; +SELECT * FROM rpr_nested(1, 1); + CREATE TABLE rpr_nav (id INT, val INT); INSERT INTO rpr_nav VALUES diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list index d1887bcc9b..21f1d3241f 100644 --- a/src/tools/pgindent/typedefs.list +++ b/src/tools/pgindent/typedefs.list @@ -2754,6 +2754,8 @@ RowMarkClause RowMarkType RowSecurityDesc RowSecurityPolicy +RprNavOffsets +RprNavState RtlGetLastNtStatus_t RtlNtStatusToDosError_t RuleInfo -- 2.34.1