From 077e5e1c1bff888a9b05d86d139f5e103d8ec971 Mon Sep 17 00:00:00 2001 From: Henson Choi Date: Mon, 28 Sep 2026 15:53:22 +0900 Subject: [PATCH 07/10] Record two RPR costs and correct comments the code no longer matches This changes comments only, so there is no behavior change. 1. Two costs recorded next to the code that causes them Neither is fixed here; the point is to document both where they arise. nfa_states_equal() (execRPR.c): an XXX records that the state comparison is finer than it needs to be. Where an element's max is RPR_QUANTITY_INF, RPRElemCanLoop() is true at every count and RPRElemCanExit() at every count at or above min, so two states that differ only in a count above min behave the same from then on. Counts saturate only at RPR_COUNT_INF, the int32 guard, so the memcmp keeps such states apart: dedup treats them as different, and the FIN early termination in nfa_advance() never fires while the pattern cannot complete. A branching unbounded pattern that never reaches FIN then holds Theta(n^2) states at peak and creates Theta(n^3) of them, each re-tested by the linear scan in nfa_append_state_unique(). With C never true, (A{2,} B)+ C takes 30 ms over 80 rows and 37 s over 320 rows, and does not finish over 640. Clamping the increment at min where max is unbounded would let the existing memcmp fold these states together, but first the clamp has to be shown safe for every reader of counts[], nfa_states_covered() included. prepare_tuplestore() (nodeWindowAgg.c): an XXX records that two fetches that sit far apart share nav_winobj's one read pointer. rpr_prepare_row() fetches the frontier row the NFA is advancing over, and ExecRPRNavGetSlot() fetches near matchStartRow for the FIRST family. Both go through window_gettupleslot(), which seeks relative to seekpos, so the two drag the pointer across the whole match on every row. In memory this costs a pointer move. Once the tuplestore spills, tuplestore_skiptuples() does tape I/O one tuple at a time and the cost grows close to quadratically. With work_mem = 64kB, PATTERN (S A+) DEFINE A AS v >= FIRST(v) takes 0.84 s at 2000 rows, 5.4 s at 4000 and 24.3 s at 8000; the same 8000 rows take 2.6 ms in memory. The results are identical either way. The fix would be a second WindowObject with its own read pointer for match-start navigation, and it would stay inside nodeWindowAgg.c. 2. Comments the earlier commits of this series left behind WindowClause (parsenodes.h): parse analysis sets rpSkipTo, defineClause and rpPattern together from one grammar production, or sets none of them. The planner does not keep the three in step: it empties defineClause, and only defineClause, on a window clause that will not run. The header comment now says to test rpPattern, never defineClause, to find out whether a clause is a row pattern window. An empty defineClause next to a non-null rpPattern means the clause will not run; a non-empty one does not promise that it will. The field comment on rpSkipTo now says that ST_NONE marks a window that is not a row pattern window, and the comment names grouping_planner() as the place that empties defineClause. The other corrections, by area: - Parser - parse_target.c: the two DEFINE comments on star expansion credited the rejection of a range variable to transformWholeRowRef(), which is never reached, and said an unresolved "name.*" is diagnosed as a qualified name; transformColumnRef() rejects either form as a whole-row reference. - parse_expr.c: the CRERR_NO_RTE comment said that adding the missing relation leads to the range variable rejection, which holds only for a two-part name. The pattern variable qualifier comment said normal resolution would report a missing FROM-clause entry; inside DEFINE it reports a qualified expression. - parse_func.c: the ParseRPRNavCall() header keyed navigation on EXPR_KIND_RPR_DEFINE; it is p_rpr_define. - Planner - initsplan.c: the build_base_rel_tlists() header now lists DEFINE clauses among what it marks as needed. - createplan.c: the trim offsets are built at executor init and resolved per scan. allpaths.c points at the loop below, not above. - var.c: no longer says it mirrors pullup_replace_vars_callback(), which leaves a strict expression over row Vars unwrapped. - rpr.c: the fillRPRPatternAlt() header still described the removed BEGIN.jump redirect. mergeGroupPrefixSuffix() runs its two phases as two passes over the whole sequence, and computeAbsorbability() adds the enclosing BEGIN/ALT for a simple VAR as well. rpr.h: RPR_COUNT_INF pointed at a count++ guard that is now RPRCountIncrement(). - Executor - nodeWindowAgg.c: the DEFINE loop is compiled with ExecInitQual(), not ExecInitExpr(). The nav slot cache saves a fetch and is not needed for pass-by-ref safety, since NAV_RESTORE copies the result. row_is_in_reduced_frame() drives the match itself and reports an empty match as unmatched. The startPos example names the case that actually starts past nfaLastProcessedRow. - execRPR.c: the loop-only branch of nfa_advance_var() is also reached by a state that has matched nothing yet, which arrives with a count of 0. The nfa_eval_var_match() precondition names the function that sets each part of the current row. nfa_advance_begin() calls a group with min > 0 non-optional rather than non-nullable, and the below-min VAR example is one that reaches that branch. - execExprInterp.c: FIRST and LAST do not clamp a target outside the match, they give NULL. llvmjit_expr.c: every navigation kind swaps the outer slot, not only PREV and NEXT. - Deparse and cross-references - ruleutils.c: preset_input_colname() also serves a RIGHT JOIN, and a NULL from get_attname() was labelled a dropped column, which comes back with its placeholder name; NULL means no such attribute. - execRPR.c, rpr.c, rpr.h: the three comments that cite README.rpr by section use the numbering it gets once its chapters are reordered by processing order -- chapters IX and X, V-6 and V-7. - parse_func.c, parse_target.c, parsenodes.h, plannodes.h and ruleutils.c also get their stated reasons corrected. Note on section numbers: the README.rpr citations corrected above (execRPR.c, rpr.c and rpr.h) use the numbering README.rpr gets in the last commit of this series, "Reorder README.rpr by processing order". Until then they point ahead of the file. Author: Henson Choi --- src/backend/executor/execExpr.c | 10 ++--- src/backend/executor/execExprInterp.c | 4 +- src/backend/executor/execRPR.c | 43 ++++++++++++++----- src/backend/executor/nodeWindowAgg.c | 55 +++++++++++++++---------- src/backend/jit/llvm/llvmjit_expr.c | 10 ++--- src/backend/optimizer/plan/createplan.c | 2 +- src/backend/optimizer/plan/initsplan.c | 3 +- src/backend/optimizer/plan/rpr.c | 27 ++++++------ src/backend/optimizer/util/var.c | 3 +- src/backend/parser/parse_expr.c | 8 ++-- src/backend/parser/parse_func.c | 10 +++-- src/backend/parser/parse_target.c | 21 +++++----- src/backend/utils/adt/ruleutils.c | 11 ++--- src/include/nodes/parsenodes.h | 15 +++++-- src/include/nodes/plannodes.h | 10 ++--- src/include/optimizer/rpr.h | 4 +- 16 files changed, 141 insertions(+), 95 deletions(-) diff --git a/src/backend/executor/execExpr.c b/src/backend/executor/execExpr.c index fa58de60c91..d6b1b3cd313 100644 --- a/src/backend/executor/execExpr.c +++ b/src/backend/executor/execExpr.c @@ -1193,15 +1193,11 @@ ExecInitExprRec(Expr *node, ExprState *state, * build_define_offsets() filled at startup; the values in it * are settled per scan by resolve_nav_offsets(). */ - if (nav->navno < 0 || - nav->navno >= list_length(winstate->rprNavOffsets)) - elog(ERROR, "RPRNavExpr navno %d out of range for %d offsets entries", - nav->navno, list_length(winstate->rprNavOffsets)); + Assert(nav->navno >= 0 && + nav->navno < list_length(winstate->rprNavOffsets)); entry = list_nth(winstate->rprNavOffsets, nav->navno); - if (entry->nav != nav) - elog(ERROR, "offsets entry %d belongs to a different RPRNavExpr", - nav->navno); + Assert(entry->nav == nav); rprnavstate = entry->rprnavstate; /* Emit SET opcode: swap slot to target row */ diff --git a/src/backend/executor/execExprInterp.c b/src/backend/executor/execExprInterp.c index a252450423a..8d94313a0f5 100644 --- a/src/backend/executor/execExprInterp.c +++ b/src/backend/executor/execExprInterp.c @@ -6069,14 +6069,14 @@ ExecEvalRPRNavSet(ExprState *state, ExprEvalStep *op, ExprContext *econtext) target_pos = -1; break; case RPR_NAV_FIRST: - /* FIRST: offset from match_start, clamped to currentpos */ + /* FIRST: offset from match_start, NULL beyond currentpos */ if (pg_add_s64_overflow(winstate->nav_match_start, offset, &target_pos)) target_pos = -1; else if (target_pos > winstate->currentpos) target_pos = -1; /* beyond current match range */ break; case RPR_NAV_LAST: - /* LAST: offset backward from currentpos, clamped to match_start */ + /* LAST: offset backward from currentpos, NULL before match_start */ target_pos = winstate->currentpos - offset; if (target_pos < winstate->nav_match_start) target_pos = -1; /* before match_start */ diff --git a/src/backend/executor/execRPR.c b/src/backend/executor/execRPR.c index 45ad10c8c78..9d8db1afc6b 100644 --- a/src/backend/executor/execRPR.c +++ b/src/backend/executor/execRPR.c @@ -135,7 +135,7 @@ static void nfa_invalidate_dependent_vars(WindowAggState *winstate, * states), absorb (drop contexts an older context already covers), advance * (expand epsilon transitions until states park on VARs). Per-element * advance behaviour, the absorption argument and the dual-flag contract are - * documented in README.rpr chapters VIII and IX and in the RPRNFAContext + * documented in README.rpr chapters IX and X and in the RPRNFAContext * comment in nodes/execnodes.h. */ @@ -303,6 +303,22 @@ nfa_states_equal(WindowAggState *winstate, RPRNFAState *s1, RPRNFAState *s2) * groups. Per the count-clear policy such a slot is zeroed when its * owning element exits (see nfa_advance_var and the inline fast path in * nfa_match), so it must not participate in equivalence judgment. + * + * XXX the comparison is finer than the future it stands for. Where max + * is RPR_QUANTITY_INF, RPRElemCanLoop() holds at every count and + * RPRElemCanExit() at every count at or above min, so two states that + * differ only above min behave identically from here on. Counts saturate + * at RPR_COUNT_INF, which is only the int32 guard, so this memcmp keeps + * such states apart and neither in-context discard folds them back: dedup + * calls them different, and the FIN early termination in nfa_advance() + * never fires while the pattern cannot complete. A branching unbounded + * pattern that never reaches FIN then holds Theta(n^2) states at peak and + * creates Theta(n^3) of them, each re-tested by the linear scan in + * nfa_append_state_unique(): (A{2,} B)+ C with C never true takes 30 ms + * over 80 rows, 37 s over 320, and does not finish over 640. Clamping the + * increment to min where max is unbounded would fold them into the memcmp + * already here, but every counts[] consumer, nfa_states_covered() + * included, has to be shown that the clamp preserves it. */ elem = &pattern->elements[s1->elemIdx]; compareDepth = elem->depth + 1; @@ -784,9 +800,11 @@ nfa_prune_skipped_contexts(WindowAggState *winstate, RPRNFAContext *ctx) * makes every VAR not match; nfa_match() is called that way to force a * 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) and invalidated the nav slot cache, via rpr_prepare_row() - * or nfa_invalidate_dependent_vars(), before consumption. + * The caller must have set up the current row before consumption: + * advance_reduced_frame_nfa() sets currentpos and nav_match_start, + * rpr_prepare_row() sets ecxt_outertuple and invalidates the nav slot cache, + * and nfa_invalidate_dependent_vars() reinstalls nav_match_start (and + * invalidates the nav slot cache) for a context whose matchStartRow differs. * * Per ISO/IEC 19075-5 Feature R020, pattern variables not listed in DEFINE * are implicitly TRUE -- they match every row. This is checked via @@ -1193,9 +1211,9 @@ nfa_advance_begin(WindowAggState *winstate, RPRNFAContext *ctx, else { /* - * Greedy-or-non-nullable: route to the first child. For optional + * Greedy-or-non-optional: route to the first child. For optional * groups (skipState != NULL, greedy min=0) additionally create the - * skip path; for non-nullable groups (skipState == NULL, min>0) the + * skip path; for non-optional groups (skipState == NULL, min>0) the * skip-path action is suppressed by the guard below. */ nfa_mark_group_entered(winstate, elem); @@ -1462,13 +1480,16 @@ nfa_advance_var(WindowAggState *winstate, RPRNFAContext *ctx, { /* * Below the minimum, so exiting is illegal and matching this VAR - * again on the next row is the only legal continuation. This row's - * match already incremented counts[depth] in the match phase, and the - * advance phase only decides where the state goes next, so staying + * again on the next row is the only legal continuation. The advance + * phase only decides where the state goes next, and counts[depth] + * already holds this VAR's matches so far: a state that matched on + * this row had it incremented in the match phase, and one that has + * matched nothing yet -- the initial state of a new context, or a + * state routed here by a skip -- arrives with 0. Either way staying * parked at the same VAR is expressed by appending the state * unchanged to the new generation. Dropping it instead would strand - * every quantifier below its minimum: (A B){2} would lose its state - * after the first A B match and never complete. + * every quantifier below its minimum: A{2} B would lose its state + * after the first A match and never complete. * * No clone is needed. With a single continuation, ownership of the * original simply transfers to the list. diff --git a/src/backend/executor/nodeWindowAgg.c b/src/backend/executor/nodeWindowAgg.c index 2dd1f7c26af..01cec0f8497 100644 --- a/src/backend/executor/nodeWindowAgg.c +++ b/src/backend/executor/nodeWindowAgg.c @@ -1298,6 +1298,21 @@ prepare_tuplestore(WindowAggState *winstate) * resolve_nav_offsets() runs before the first begin_partition(), so * the kind here is FIXED or RETAIN_ALL even for a parameterized * offset; RETAIN_ALL disables trim. + * + * XXX one read pointer serves two fetches that sit far apart. + * rpr_prepare_row() fetches the frontier row the NFA is advancing + * over, and ExecRPRNavGetSlot() fetches near matchStartRow for the + * FIRST family; both go through window_gettupleslot(), which seeks + * relative to seekpos, so the two drag the one pointer across the + * whole match on every row. In memory that is a pointer move, but + * once the tuplestore spills tuplestore_skiptuples() is + * tuple-at-a-time tape I/O and the cost turns quasi-quadratic: + * PATTERN (S A+) DEFINE A AS v >= FIRST(v) under work_mem 64kB takes + * 0.84 s at 2,000 rows, 5.4 s at 4,000 and 24.3 s at 8,000, against + * 2.6 ms for those same 8,000 rows in memory. The answers are + * identical either way. Separating them needs a second WindowObject + * carrying its own read pointer for match-start navigation, which + * stays inside this file. */ winstate->nav_winobj->markptr = tuplestore_alloc_read_pointer(winstate->buffer, 0); @@ -3066,8 +3081,8 @@ ExecInitWindowAgg(WindowAgg *node, EState *estate, int eflags) winstate->navFirstOffsetKind = RPR_NAV_OFFSET_FIXED; /* - * Must run this before the ExecInitExpr() loop over defineClause: - * while compiling each RPRNavExpr, ExecInitExpr() reads + * 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 */ @@ -3075,7 +3090,7 @@ ExecInitWindowAgg(WindowAgg *node, EState *estate, int eflags) /* * Compile DEFINE clause expressions. PREV/NEXT navigation is handled - * by EEOP_RPR_NAV_SET/RESTORE opcodes emitted during ExecInitExpr, so + * by EEOP_RPR_NAV_SET/RESTORE opcodes emitted during ExecInitQual, so * no varno rewriting is needed here. Expressions are kept in DEFINE * order, so their list index equals the variable's varId. */ @@ -3198,10 +3213,10 @@ ExecRPRNavGetSlot(WindowAggState *winstate, int64 pos) /* * If nav_slot already holds this position, return it without re-fetching. - * This is critical when multiple PREV/NEXT calls in the same expression - * navigate to the same row, because re-fetching would free the slot's - * tuple memory and invalidate any pass-by-ref Datum pointers from earlier - * navigation results. + * This saves a tuplestore fetch when several navigations in the same + * expression target the same row. Earlier pass-by-ref results do not + * depend on it: EEOP_RPR_NAV_RESTORE copies them out of nav_slot's tuple + * memory. */ if (winstate->nav_slot_pos == pos) return slot; @@ -4178,16 +4193,15 @@ nav_offsets_walker(Node *node, WindowAggState *winstate) * * 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, and the fields this fills are left at their palloc0 - * defaults on the paths that return early. + * window reaches here. */ static void build_define_offsets(WindowAggState *winstate, List *defineClause) { EvalDefineOffsetsContext ctx; - if (defineClause == NIL) - return; + /* DEFINE is mandatory, so an RPR window always has a clause */ + Assert(defineClause != NIL); foreach_node(TargetEntry, te, defineClause) { @@ -4404,8 +4418,8 @@ resolve_nav_offsets(WindowAggState *winstate) winstate->navFirstOffset = 0; winstate->navFirstOffsetKind = RPR_NAV_OFFSET_FIXED; - if (winstate->rprNavOffsets == NIL) - return; + /* The request is pending only for a window that holds a navigation */ + Assert(winstate->rprNavOffsets != NIL); ctx.winstate = winstate; ctx.maxOffset = 0; @@ -4453,14 +4467,14 @@ rpr_is_defined(WindowAggState *winstate) * Determine whether a row is in the current row's reduced window frame * according to row pattern matching * - * The row must have already been determined to be in a full window frame - * and fetched into the slot. + * If pos is not yet determined, the match is first driven forward by + * ensure_reduced_frame(). * * Returns: * = 0, RPR is not defined. * >0, if the row is the first in the reduced frame. Return the number of rows * in the reduced frame. - * -1, if the row is an unmatched row + * -1, if the row is unmatched or starts an empty match * -2, if the row is inside the current match but is not its first row (an * interior row of the match) * ----------------- @@ -4616,9 +4630,8 @@ advance_nav_mark(WindowAggState *winstate, int64 currentPos) { int64 navmarkpos; - /* No RPR navigation read pointer: nothing to advance */ - if (winstate->nav_winobj == NULL) - return; + /* Every RPR window has its navigation read pointer */ + Assert(winstate->nav_winobj != NULL); /* RETAIN_ALL (offset overflow) disables trim for the backward dimension */ if (winstate->navMaxOffsetKind == RPR_NAV_OFFSET_RETAIN_ALL) @@ -4666,8 +4679,8 @@ advance_reduced_frame_nfa(WindowObject winobj, RPRNFAContext *targetCtx) /* * Determine where to start processing. Usually nfaLastProcessedRow+1 >= * matchStartRow since contexts are created at currentPos+1 during - * processing. However, matchStartRow can exceed this when rows are - * skipped (e.g., unmatched rows don't update nfaLastProcessedRow). + * processing. However, a context update_reduced_frame() creates on + * demand, for a pos past nfaLastProcessedRow, can start beyond it. */ startPos = Max(targetCtx->matchStartRow, winstate->nfaLastProcessedRow + 1); diff --git a/src/backend/jit/llvm/llvmjit_expr.c b/src/backend/jit/llvm/llvmjit_expr.c index 74b7392f15f..8251cad135c 100644 --- a/src/backend/jit/llvm/llvmjit_expr.c +++ b/src/backend/jit/llvm/llvmjit_expr.c @@ -301,11 +301,11 @@ llvm_compile_expr(ExprState *state) "v.econtext.aggnulls"); /* - * RPR navigation opcodes (PREV/NEXT) swap ecxt_outertuple to a different - * row mid-expression. The JIT code loads v_outervalues and v_outernulls - * once in the entry block and reuses them for all EEOP_OUTER_VAR steps. - * After a slot swap, these cached pointers become stale because the new - * slot has its own tts_values/tts_isnull arrays. + * RPR navigation opcodes (PREV/NEXT/FIRST/LAST) swap ecxt_outertuple to a + * different row mid-expression. The JIT code loads v_outervalues and + * v_outernulls once in the entry block and reuses them for all + * EEOP_OUTER_VAR steps. After a slot swap, these cached pointers become + * stale because the new slot has its own tts_values/tts_isnull arrays. * * When RPR navigation opcodes are present, EEOP_OUTER_VAR reloads the * slot pointer from econtext->ecxt_outertuple on every access instead of diff --git a/src/backend/optimizer/plan/createplan.c b/src/backend/optimizer/plan/createplan.c index dad285eef96..a4bc5709a65 100644 --- a/src/backend/optimizer/plan/createplan.c +++ b/src/backend/optimizer/plan/createplan.c @@ -2670,7 +2670,7 @@ create_windowagg_plan(PlannerInfo *root, WindowAggPath *best_path) /* * Classify which DEFINE variables depend on match_start (for * absorption suppression in buildRPRPattern). Nav offsets for - * tuplestore trim are resolved later, at executor init. + * tuplestore trim are built at executor init and resolved per scan. */ compute_define_metadata(wc->defineClause, &matchStartDependent); diff --git a/src/backend/optimizer/plan/initsplan.c b/src/backend/optimizer/plan/initsplan.c index 6994c5d478c..e672c6945ae 100644 --- a/src/backend/optimizer/plan/initsplan.c +++ b/src/backend/optimizer/plan/initsplan.c @@ -244,7 +244,8 @@ add_other_rels_to_query(PlannerInfo *root) /* * build_base_rel_tlists * Add targetlist entries for each var needed in the query's final tlist - * (and HAVING clause, if any) to the appropriate base relations. + * (and HAVING clause and row pattern DEFINE clauses, if any) to the + * appropriate base relations. * * We mark such vars as needed by "relation 0" to ensure that they will * propagate up through all join plan steps. diff --git a/src/backend/optimizer/plan/rpr.c b/src/backend/optimizer/plan/rpr.c index cc0ae0df6ed..df3a0b51850 100644 --- a/src/backend/optimizer/plan/rpr.c +++ b/src/backend/optimizer/plan/rpr.c @@ -535,14 +535,13 @@ mergeConsecutiveAlts(List *children) * the GROUP in a SEQ, merge them by incrementing the GROUP's quantifier. * This runs iteratively: A B A B (A B)+ A B -> (A B){4,}. * - * Algorithm: - * For each GROUP encountered in the sequence: - * 1. PREFIX phase: compare the last N survivors kept so far against the - * GROUP's children. On match, drop them and increment the GROUP's - * min/max. Repeat until no match. - * 2. SUFFIX phase: compare the next N elements not yet read against the - * GROUP's children. On match, skip them and increment min/max. - * Repeat until no match. + * Algorithm, in two passes over the whole sequence: + * 1. PREFIX phase: for each GROUP, compare the last N survivors kept so + * far against the GROUP's children. On match, drop them and increment + * the GROUP's min/max. Repeat until no match. + * 2. SUFFIX phase: for each GROUP, compare the next N elements not yet + * read against the GROUP's children. On match, skip them and + * increment min/max. Repeat until no match. * * Examples: * A B (A B)+ -> (A B){2,} @@ -1335,7 +1334,7 @@ fillRPRPatternGroup(RPRPatternNode *node, RPRPattern *pat, int *idx, RPRDepth de if (node->reluctant) endElem->flags |= RPR_ELEM_RELUCTANT; - /* The END carries the body's bits, not the group's; see README IV-4b */ + /* The END carries the body's bits, not the group's; see README V-6 */ endElem->flags |= bodyFlags; /* Set BEGIN's link to its END (next is set by finalize) */ @@ -1369,8 +1368,9 @@ fillRPRPatternGroup(RPRPatternNode *node, RPRPattern *pat, int *idx, RPRDepth de * ALT.jump -> first SEP (post-ALT on the last) * SEP.jump -> next SEP (-1 on the last) * - * SEP is a marker, never a state: a branch's tail and a branch-terminal - * group's BEGIN skip are redirected past the alternation. + * SEP is a marker, never a state: each branch's tail is redirected past the + * alternation. A branch-terminal group's BEGIN skip needs no redirect of its + * own, since it leaves through the group's END, which is that branch's tail. * * Returns the alternation's empty-match flags. RPR_ELEM_EMPTY_LOOP is set if * any branch is nullable (OR: one nullable branch suffices). @@ -1922,9 +1922,10 @@ computeAbsorbabilityRecursive(RPRPattern *pattern, * - Simple unbounded VAR: the VAR itself (e.g., A in A+) * - Unbounded GROUP: the END element (e.g., END in (A B)+) * RPR_ELEM_ABSORBABLE_BRANCH: All elements in absorbable region - * - Simple unbounded VAR: the VAR itself only + * - Simple unbounded VAR: the VAR itself * - Unbounded GROUP: the whole body (including nested subgroups) and the - * group's END, plus any enclosing BEGIN/ALT on the path to it + * group's END + * - in either case, plus any enclosing BEGIN/ALT on the path to it * * Examples: * A+ B C - absorbable (A gets both flags) diff --git a/src/backend/optimizer/util/var.c b/src/backend/optimizer/util/var.c index ff308a9c7c5..d65a3218c66 100644 --- a/src/backend/optimizer/util/var.c +++ b/src/backend/optimizer/util/var.c @@ -933,7 +933,8 @@ flatten_join_alias_vars_mutator(Node *node, /* * Below a navigation argument, wrap a non-Var/PHV replacement in a * PlaceHolderVar so it can't be constant-folded away before the - * navigation runs (mirrors pullup_replace_vars_callback()). + * navigation runs. Unlike pullup_replace_vars_callback(), this also + * wraps a strict expression over Vars. */ if (context->in_rpr_nav_arg && context->root != NULL && !(IsA(newvar, Var) && ((Var *) newvar)->varlevelsup == var->varlevelsup) && diff --git a/src/backend/parser/parse_expr.c b/src/backend/parser/parse_expr.c index 7dd53a1f455..94193cfcdd3 100644 --- a/src/backend/parser/parse_expr.c +++ b/src/backend/parser/parse_expr.c @@ -617,7 +617,7 @@ transformColumnRef(ParseState *pstate, ColumnRef *cref) * A pattern variable qualifier (e.g. UP.price) is valid per ISO/IEC * 19075-5 6.15 / 4.16 but not yet implemented, and has to be recognized * here: a pattern variable names no range table entry, so leaving it to - * normal resolution would report a missing FROM-clause entry instead. + * normal resolution would reject it as a qualified expression instead. * * Like every other rule below, this one only reaches names the ref hooks * left for the query parser to resolve. A PL that answers a name first @@ -942,8 +942,10 @@ transformColumnRef(ParseState *pstate, ColumnRef *cref) * nothing is rejected for occupying it, the same as one that * names something. Reporting a missing FROM-clause entry * would point at a repair that does not exist: adding the - * relation only moves the reference to the range variable - * rejection below. + * relation only moves the reference to one of the rejections + * below, the range variable one for a two-part name and the + * qualified expression one for a longer name, or the outer + * column one if the relation is added to an outer query. */ if (pstate->p_rpr_define) ereport(ERROR, diff --git a/src/backend/parser/parse_func.c b/src/backend/parser/parse_func.c index b95f0ff04be..e688009e6bb 100644 --- a/src/backend/parser/parse_func.c +++ b/src/backend/parser/parse_func.c @@ -2156,12 +2156,14 @@ is_rpr_navigation_name(const char *name) * ParseRPRNavCall * Recognize a row pattern navigation operation in a DEFINE clause. * - * Inside an EXPR_KIND_RPR_DEFINE clause an unqualified call to one of the - * names PREV/NEXT/FIRST/LAST denotes the corresponding row pattern navigation - * operation (ISO/IEC 19075-5 Subclause 5.6), not an ordinary function call. + * Anywhere inside a DEFINE condition (p_rpr_define is set, even where a + * nested FILTER or aggregate ORDER BY has changed p_expr_kind) an unqualified + * call to one of the names PREV/NEXT/FIRST/LAST denotes the corresponding row + * pattern navigation operation (ISO/IEC 19075-5 Subclause 5.6), not an + * ordinary function call. * The name is matched here, before any catalog lookup, with no fallback to * function resolution: once it matches, decoration and argument-count - * violations are dedicated errors rather than letting an ordinary function of + * violations are hard errors rather than letting an ordinary function of * the same name take over. A schema-qualified call (the caller restricts us * to unqualified names) is the documented way to reach such a function * instead. diff --git a/src/backend/parser/parse_target.c b/src/backend/parser/parse_target.c index 30217f9c9fb..441e10fb5c3 100644 --- a/src/backend/parser/parse_target.c +++ b/src/backend/parser/parse_target.c @@ -270,11 +270,12 @@ transformExpressionList(ParseState *pstate, List *exprlist, * * No DEFINE test is needed here, unlike the ColumnRef arm * above. ExpandIndirectionStar() transforms the - * parenthesized argument under the same expression kind, so a - * range variable still reaches transformWholeRowRef() and is - * rejected; what survives is field selection on a value, - * "(x).*", which occupies no qualifier slot and is allowed in - * DEFINE for the same reason "(x).f" is. + * parenthesized argument with p_rpr_define still set, so + * transformColumnRef() still rejects a range variable before + * it becomes a whole-row reference; what survives is field + * selection on a value, "(x).*", which occupies no qualifier + * slot and is allowed in DEFINE for the same reason "(x).f" + * is. */ result = list_concat(result, ExpandIndirectionStar(pstate, ind, @@ -1284,11 +1285,11 @@ ExpandColumnRefStar(ParseState *pstate, ColumnRef *cref, * FROM-clause relation, or a name that resolves to nothing at all. A * row pattern DEFINE condition may have neither, and expanding one * here binds it by RTE rather than by name, past the checks in - * transformColumnRef() and transformWholeRowRef(). Decline, so that - * the caller hands the whole reference to transformExpr() and it is - * diagnosed there, where every other DEFINE spelling is: a relation - * as a whole-row reference, a pattern variable as the qualifier it - * reserves, an unresolved name as the qualified name it is. + * transformColumnRef(). Decline, so that the caller hands the whole + * reference to transformExpr() and it is diagnosed there, where every + * other DEFINE spelling is: a pattern variable as the qualifier it + * reserves, and any other name, resolved or not, as the whole-row + * reference its form makes it. * * A name a hook owns has returned above, which is the point of * deciding here rather than in the caller. Withholding the expansion diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c index 697900a231a..33d96f782d5 100644 --- a/src/backend/utils/adt/ruleutils.c +++ b/src/backend/utils/adt/ruleutils.c @@ -4697,11 +4697,12 @@ mark_define_column(deparse_namespace *dpns, Var *var) * * mark_define_columns() stores a name into the colnames entry of the RTE a * DEFINE reference resolves to, and for a column merged by an unaliased - * INNER or LEFT JOIN that is an input of the join rather than the join - * itself. A merged column has to be named the same on both sides, so - * set_using_names() asks here, before inventing a name, whether one of the - * inputs has settled it already. A join input is followed down the way the - * parser built the merged column, through joinaliasvars. + * INNER, LEFT or RIGHT JOIN with no type coercion that is an input of the + * join rather than the join itself. A merged column has to be named the + * same on both sides, so set_using_names() asks here, before inventing a + * name, whether one of the inputs has settled it already. A join input is + * followed down the way the parser built the merged column, through + * joinaliasvars. */ static char * preset_input_colname(deparse_namespace *dpns, int varno, AttrNumber attno) diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h index a8bdaecd2c4..5f06e054f32 100644 --- a/src/include/nodes/parsenodes.h +++ b/src/include/nodes/parsenodes.h @@ -584,8 +584,8 @@ typedef struct SortBy */ typedef enum RPSkipTo { - ST_NONE, /* no AFTER MATCH clause; default for non-RPR - * windows */ + ST_NONE, /* not a row pattern window; an omitted AFTER + * MATCH gives ST_PAST_LAST_ROW */ ST_NEXT_ROW, /* SKIP TO NEXT ROW */ ST_PAST_LAST_ROW, /* SKIP TO PAST LAST ROW */ } RPSkipTo; @@ -1636,6 +1636,13 @@ typedef struct GroupingSet * TargetEntry). TargetEntry.resname represents row pattern definition * variable name. "rpPattern" represents the PATTERN clause as a parse tree * (RPRPatternNode). + * Parse analysis sets rpSkipTo, defineClause and rpPattern from one grammar + * production, or none of them. The planner does not keep them so: + * grouping_planner() empties defineClause alone on a window clause that will + * not run, one not in activeWindows. Test rpPattern, never defineClause, for + * "is this a row pattern window". An empty defineClause beside a non-null + * rpPattern means the clause will not run; a non-empty one does not mean it + * will. * */ typedef struct WindowClause @@ -1664,8 +1671,8 @@ typedef struct WindowClause Index winref; /* ID referenced by window functions */ /* did we copy orderClause from refname? */ bool copiedOrder pg_node_attr(query_jumble_ignore); - /* Row Pattern AFTER MATCH SKIP clause */ - RPSkipTo rpSkipTo; /* Row Pattern Skip To type */ + /* AFTER MATCH SKIP type; ST_NONE if this is not a row pattern window */ + RPSkipTo rpSkipTo; /* Row Pattern DEFINE clause (list of TargetEntry) */ List *defineClause pg_node_attr(custom_query_jumble); /* Row Pattern PATTERN parse tree */ diff --git a/src/include/nodes/plannodes.h b/src/include/nodes/plannodes.h index e2f27eb0034..b0e6e7f8ec2 100644 --- a/src/include/nodes/plannodes.h +++ b/src/include/nodes/plannodes.h @@ -1294,11 +1294,11 @@ typedef struct RPRPattern /* * RPRPattern is a plan/exec-only node with arrays that need a * hand-written copy (custom_copy_equal). It is never compared with - * equal(): equal() routines are generated only for parse/rewrite-level - * nodes, not for plan nodes, so there is nothing to compare it against - * and equal support is suppressed with no_equal. It is not reachable - * from a Query either, so query jumbling has nothing to do here and is - * suppressed with no_query_jumble. + * equal(): like Plan and PlannedStmt, which carry no_equal, it is never + * handed to equal(), so there is nothing to compare it against and equal + * support is suppressed with no_equal. It is not reachable from a Query + * either, so query jumbling has nothing to do here and is suppressed with + * no_query_jumble. */ pg_node_attr(custom_copy_equal, custom_read_write, no_equal, no_query_jumble) diff --git a/src/include/optimizer/rpr.h b/src/include/optimizer/rpr.h index 0c1e0898397..f6eaae977d7 100644 --- a/src/include/optimizer/rpr.h +++ b/src/include/optimizer/rpr.h @@ -30,7 +30,7 @@ /* * RPR_COUNT_INF is the value a runtime repetition count saturates at to avoid - * int32 overflow (see the count++ guard in nfa_match). It is defined as + * int32 overflow (see RPRCountIncrement() below). It is defined as * RPR_QUANTITY_INF (from nodes/parsenodes.h, included above) so that a * saturated count compares as "unbounded", just like an unbounded * quantifier's max. @@ -58,7 +58,7 @@ #define RPR_ELEM_EMPTY_PREFERRED 0x04 /* END: group body prefers the * empty match */ /* - * The two absorption flags below are explained in README.rpr IV-5 + * The two absorption flags below are explained in README.rpr V-7 * ("Absorbability Analysis"), with worked examples in Appendix B; the * analysis that sets them is computeAbsorbability() in * optimizer/plan/rpr.c. -- 2.54.0 (Apple Git-157)