From 7dd896a8b279a1f02e8a5d9f241d9a59a32ee588 Mon Sep 17 00:00:00 2001 From: Henson Choi Date: Thu, 2 Apr 2026 11:55:02 +0900 Subject: [PATCH] Fix DEFINE expression handling in RPR window planning transformDefineClause() added full DEFINE expressions including RPRNavExpr (PREV/NEXT) nodes to the query targetlist. These propagated to upper WindowAgg nodes that lack RPR navigation state, causing a SIGSEGV when RPR and non-RPR windows coexist in the same query. Add only the Var nodes referenced by DEFINE expressions to the targetlist, and protect those Vars from removal by remove_unused_subquery_outputs() so they remain available in the tuplestore slot for pattern matching evaluation. Move the subquery wrapping tests from rpr.sql to rpr_integration.sql. --- src/backend/optimizer/path/allpaths.c | 68 +++++++++++++++++ src/backend/parser/parse_rpr.c | 106 ++++++++++++++------------ 2 files changed, 126 insertions(+), 48 deletions(-) diff --git a/src/backend/optimizer/path/allpaths.c b/src/backend/optimizer/path/allpaths.c index f42a2bae14a..f0ef4e289a6 100644 --- a/src/backend/optimizer/path/allpaths.c +++ b/src/backend/optimizer/path/allpaths.c @@ -4750,6 +4750,74 @@ remove_unused_subquery_outputs(Query *subquery, RelOptInfo *rel, if (contain_volatile_functions(texpr)) continue; + /* + * If any RPR window clause references this column in its DEFINE + * clause, don't remove it. The DEFINE expression needs these columns + * in the tuplestore slot for pattern matching evaluation, even if the + * outer query doesn't reference them. + */ + if (IsA(texpr, Var)) + { + Var *var = (Var *) texpr; + ListCell *wlc; + bool needed_by_define = false; + + foreach(wlc, subquery->windowClause) + { + WindowClause *wc = lfirst_node(WindowClause, wlc); + + if (wc->defineClause != NIL) + { + List *vars = pull_var_clause((Node *) wc->defineClause, 0); + ListCell *vlc; + + foreach(vlc, vars) + { + Var *dvar = (Var *) lfirst(vlc); + + if (dvar->varattno == var->varattno) + { + needed_by_define = true; + break; + } + } + list_free(vars); + if (needed_by_define) + break; + } + } + if (needed_by_define) + continue; + } + + /* + * If it's a window function referencing a window clause with RPR (Row + * Pattern Recognition), don't remove it. Even when the window + * function result is unused by the outer query, the RPR pattern + * matching (frame reduction via DEFINE/PATTERN) must still execute. + * Replacing this with NULL would leave no active window functions for + * the WindowClause, causing the planner to omit the WindowAgg node + * entirely. + */ + if (IsA(texpr, WindowFunc)) + { + WindowFunc *wfunc = (WindowFunc *) texpr; + ListCell *wlc; + + foreach(wlc, subquery->windowClause) + { + WindowClause *wc = lfirst_node(WindowClause, wlc); + + if (wc->winref == wfunc->winref && + wc->defineClause != NIL) + { + break; + } + } + if (wlc != NULL) + continue; + } + /* * OK, we don't need it. Replace the expression with a NULL constant. * Preserve the exposed type of the expression, in case something diff --git a/src/backend/parser/parse_rpr.c b/src/backend/parser/parse_rpr.c index 55283ab4bbe..db1309ca311 100644 --- a/src/backend/parser/parse_rpr.c +++ b/src/backend/parser/parse_rpr.c @@ -28,6 +28,7 @@ #include "miscadmin.h" #include "nodes/makefuncs.h" #include "nodes/nodeFuncs.h" +#include "optimizer/optimizer.h" #include "optimizer/rpr.h" #include "parser/parse_clause.h" #include "parser/parse_collate.h" @@ -310,9 +311,7 @@ transformDefineClause(ParseState *pstate, WindowClause *wc, WindowDef *windef, restargets = NIL; foreach(lc, windef->rpCommonSyntax->rpDefs) { - TargetEntry *te, - *teDefine; - int defineExprLocation; + TargetEntry *teDefine; restarget = (ResTarget *) lfirst(lc); name = restarget->name; @@ -335,57 +334,68 @@ transformDefineClause(ParseState *pstate, WindowClause *wc, WindowDef *windef, restargets = lappend(restargets, restarget); /* - * Transform the DEFINE expression (restarget->val) and add it to the - * targetlist as a TargetEntry if not already present, so the planner - * can propagate the referenced columns to the outer plan's - * targetlist. + * Transform the DEFINE expression. We must NOT add the whole + * expression to the query targetlist, because it may contain + * RPRNavExpr nodes (PREV/NEXT) that can only be evaluated inside the + * owning WindowAgg. * - * Note: findTargetlistEntrySQL99 transforms and clobbers - * restarget->val. + * Instead, we transform the expression directly and only ensure that + * the individual Var nodes it references are present in the + * targetlist, so the planner can propagate the referenced columns. */ + { + Node *expr; + List *vars; + ListCell *lc2; - /* - * Save the original expression location before transformation. - * findTargetlistEntrySQL99 may return an existing TargetEntry whose - * location points to where it was originally created (e.g., ORDER - * BY), not the DEFINE clause. We need to preserve the DEFINE location - * for accurate error reporting. - */ - defineExprLocation = exprLocation(restarget->val); + expr = transformExpr(pstate, restarget->val, + EXPR_KIND_RPR_DEFINE); + + /* + * Pull out Var nodes from the transformed expression and ensure + * each one is present in the targetlist. This is needed so the + * planner propagates the referenced columns through the plan + * tree, making them available to the WindowAgg's DEFINE + * evaluation. + */ + vars = pull_var_clause(expr, 0); + foreach(lc2, vars) + { + Var *var = (Var *) lfirst(lc2); + bool found = false; + ListCell *tl; - te = findTargetlistEntrySQL99(pstate, restarget->val, - targetlist, EXPR_KIND_RPR_DEFINE); + foreach(tl, *targetlist) + { + TargetEntry *tle = (TargetEntry *) lfirst(tl); - /* ------------------- - * Copy the TargetEntry for defineClause and always set the pattern - * variable name. We use copyObject so the original targetlist entry - * is not modified. - * - * Note: We must always set resname to the pattern variable name. - * findTargetlistEntrySQL99 creates new TEs with resname = NULL - * (resjunk entries), but returns existing TEs unchanged when the - * expression already exists in targetlist. - * - * Example: "SELECT id, flag, ... WINDOW w AS (... DEFINE T AS flag)" - * - * 1. SELECT list processing creates: TE{resname="flag", expr=flag} - * 2. DEFINE T AS flag: findTargetlistEntrySQL99 finds existing TE - * 3. te->resname is "flag" (from SELECT), not NULL - * 4. Without unconditionally setting resname, teDefine->resname - * would remain "flag" instead of pattern variable name "T" - * 5. buildRPRPattern builds defineVariableList from resname, so - * it would contain ["flag"] instead of ["T"] - * 6. Pattern variable "T" not found -> Assert failure crash - */ - teDefine = copyObject(te); - teDefine->resname = pstrdup(name); + if (IsA(tle->expr, Var) && + ((Var *) tle->expr)->varno == var->varno && + ((Var *) tle->expr)->varattno == var->varattno) + { + found = true; + break; + } + } + if (!found) + { + TargetEntry *newtle; - /* - * Update the expression location to point to the DEFINE clause. This - * ensures error messages reference the correct source location. - */ - if (defineExprLocation >= 0 && IsA(teDefine->expr, Var)) - ((Var *) teDefine->expr)->location = defineExprLocation; + newtle = makeTargetEntry((Expr *) copyObject(var), + list_length(*targetlist) + 1, + NULL, + true); + *targetlist = lappend(*targetlist, newtle); + } + } + list_free(vars); + + /* Build the defineClause entry directly from the transformed expr */ + teDefine = makeTargetEntry((Expr *) expr, + list_length(defineClause) + 1, + pstrdup(name), + true); + } /* build transformed DEFINE clause (list of TargetEntry) */ defineClause = lappend(defineClause, teDefine); -- 2.50.1 (Apple Git-155)