public inbox for [email protected]
help / color / mirror / Atom feedFrom: Henson Choi <[email protected]>
To: Tatsuo Ishii <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Subject: Re: Row pattern recognition
Date: Tue, 24 Feb 2026 13:38:32 +0900
Message-ID: <CAAAe_zAc4CQDMP5WVuNZq5q6A_CymN5xwrva38df3okrtYdYsw@mail.gmail.com> (raw)
In-Reply-To: <[email protected]>
References: <CAAAe_zDcrF9_m5FnWFgdrCpxjTqdH2ZMDmpKXevMG4KBHJsw4w@mail.gmail.com>
<[email protected]>
<CAAAe_zAxEqdyoOgx4u0A2RXu829CE-RJbJjg1jVz00j3aSYryQ@mail.gmail.com>
<[email protected]>
Hi Tatsuo,
> BTW, in create_windowagg_plan (createplan.c),
> around:
> /* Build RPR pattern and filter defineClause */
>
> collectPatternVariables, filterDefineClause and buildRPRPattern are
> called in a block without any if or any other conditional
> statements. This is an unusual codiing style in PostgreSQL. I suggest
> to fix this. Attached is a proposed patch for this.
Good catch, thank you! I've renumbered your patch as nocfbot-0007
and extended the same cleanup to rpr.c and parse_rpr.c as
nocfbot-0008.
nocfbot-0007: Refactor create_windowagg_plan to remove bare
variable-scoping block (your patch, renumbered)
nocfbot-0008: Remove bare variable-scoping blocks in RPR code
Applies the same cleanup to rpr.c and parse_rpr.c, with minor
pgindent formatting fixes.
I'll keep this coding style point in mind for future code as well.
Best regards,
Henson
Attachments:
[application/octet-stream] nocfbot-0007-Refactor-create_windowagg_plan-to-remove-bare-variab.patch (3.3K, 3-nocfbot-0007-Refactor-create_windowagg_plan-to-remove-bare-variab.patch)
download | inline diff:
From 07a9c78d237359ae64a10af407d9629c195e06dc Mon Sep 17 00:00:00 2001
From: Henson Choi <[email protected]>
Date: Tue, 24 Feb 2026 12:35:14 +0900
Subject: [PATCH 7/8] Refactor create_windowagg_plan to remove bare
variable-scoping block
---
src/backend/optimizer/plan/createplan.c | 79 ++++++++++++-------------
1 file changed, 39 insertions(+), 40 deletions(-)
diff --git a/src/backend/optimizer/plan/createplan.c b/src/backend/optimizer/plan/createplan.c
index bbc2c7e71f4..e7aafc89700 100644
--- a/src/backend/optimizer/plan/createplan.c
+++ b/src/backend/optimizer/plan/createplan.c
@@ -2484,6 +2484,10 @@ create_windowagg_plan(PlannerInfo *root, WindowAggPath *best_path)
Oid *ordOperators;
Oid *ordCollations;
ListCell *lc;
+ List *defineVariableList = NIL;
+ List *filteredDefineClause = NIL;
+ RPRPattern *compiledPattern = NULL;
+
/*
* Choice of tlist here is motivated by the fact that WindowAgg will be
@@ -2535,50 +2539,45 @@ create_windowagg_plan(PlannerInfo *root, WindowAggPath *best_path)
}
/* Build RPR pattern and filter defineClause */
+ if (wc->rpPattern)
{
- List *defineVariableList = NIL;
- List *filteredDefineClause = NIL;
- RPRPattern *compiledPattern = NULL;
-
- if (wc->rpPattern)
- {
- List *patternVars;
+ List *patternVars;
- /*
- * Filter defineClause to include only variables used in PATTERN.
- * This eliminates unnecessary DEFINE evaluations at runtime.
- */
- patternVars = collectPatternVariables(wc->rpPattern);
- filteredDefineClause = filterDefineClause(wc->defineClause,
- patternVars,
- &defineVariableList);
-
- compiledPattern = buildRPRPattern(wc->rpPattern,
- defineVariableList,
- wc->rpSkipTo,
- wc->frameOptions);
- }
-
- /* And finally we can make the WindowAgg node */
- plan = make_windowagg(tlist,
- wc,
- partNumCols,
- partColIdx,
- partOperators,
- partCollations,
- ordNumCols,
- ordColIdx,
- ordOperators,
- ordCollations,
- best_path->runCondition,
- wc->rpSkipTo,
- compiledPattern,
- filteredDefineClause,
- best_path->qual,
- best_path->topwindow,
- subplan);
+ /*
+ * Filter defineClause to include only variables used in PATTERN. This
+ * eliminates unnecessary DEFINE evaluations at runtime.
+ */
+ patternVars = collectPatternVariables(wc->rpPattern);
+ filteredDefineClause = filterDefineClause(wc->defineClause,
+ patternVars,
+ &defineVariableList);
+
+ /* Compile and optimize RPR patterns */
+ compiledPattern = buildRPRPattern(wc->rpPattern,
+ defineVariableList,
+ wc->rpSkipTo,
+ wc->frameOptions);
}
+ /* And finally we can make the WindowAgg node */
+ plan = make_windowagg(tlist,
+ wc,
+ partNumCols,
+ partColIdx,
+ partOperators,
+ partCollations,
+ ordNumCols,
+ ordColIdx,
+ ordOperators,
+ ordCollations,
+ best_path->runCondition,
+ wc->rpSkipTo,
+ compiledPattern,
+ filteredDefineClause,
+ best_path->qual,
+ best_path->topwindow,
+ subplan);
+
copy_generic_path_info(&plan->plan, (Path *) best_path);
return plan;
--
2.50.1 (Apple Git-155)
[application/octet-stream] nocfbot-0008-Remove-bare-variable-scoping-blocks-in-RPR-code.patch (7.4K, 4-nocfbot-0008-Remove-bare-variable-scoping-blocks-in-RPR-code.patch)
download | inline diff:
From 93f98d31fc2a9ca58cd973b671b5b241c6d5d461 Mon Sep 17 00:00:00 2001
From: Henson Choi <[email protected]>
Date: Tue, 24 Feb 2026 12:38:05 +0900
Subject: [PATCH 8/8] Remove bare variable-scoping blocks in RPR code
---
src/backend/optimizer/plan/rpr.c | 66 ++++++++++++++--------------
src/backend/parser/parse_rpr.c | 74 ++++++++++++++++----------------
2 files changed, 68 insertions(+), 72 deletions(-)
diff --git a/src/backend/optimizer/plan/rpr.c b/src/backend/optimizer/plan/rpr.c
index 987f595d434..4414b71b5ec 100644
--- a/src/backend/optimizer/plan/rpr.c
+++ b/src/backend/optimizer/plan/rpr.c
@@ -503,6 +503,7 @@ mergeGroupPrefixSuffix(List *children)
List *groupContent = child->children;
int groupChildCount;
int prefixLen = list_length(result);
+ List *trimmed;
/*
* If GROUP has single SEQ child, compare with SEQ's children.
@@ -546,17 +547,14 @@ mergeGroupPrefixSuffix(List *children)
child->min += 1;
/* Rebuild result without matched prefix */
+ trimmed = NIL;
+ for (j = 0; j < prefixLen - groupChildCount; j++)
{
- List *trimmed = NIL;
-
- for (j = 0; j < prefixLen - groupChildCount; j++)
- {
- trimmed = lappend(trimmed,
- list_nth(result, j));
- }
- result = trimmed;
- prefixLen = list_length(result);
+ trimmed = lappend(trimmed,
+ list_nth(result, j));
}
+ result = trimmed;
+ prefixLen = list_length(result);
}
else
{
@@ -1226,9 +1224,11 @@ static void
fillRPRPatternAlt(RPRPatternNode *node, RPRPattern *pat, int *idx, RPRDepth depth)
{
ListCell *lc;
+ ListCell *lc2;
RPRPatternElement *elem;
List *altBranchStarts = NIL;
List *altEndPositions = NIL;
+ int afterAltIdx;
/* Add alternation start marker */
elem = &pat->elements[*idx];
@@ -1262,38 +1262,36 @@ fillRPRPatternAlt(RPRPatternNode *node, RPRPattern *pat, int *idx, RPRDepth dept
}
/* Set next on last element of each alternative to after the alternation */
+ afterAltIdx = *idx;
+ lc2 = list_head(altBranchStarts);
+
+ foreach(lc, altEndPositions)
{
- int afterAltIdx = *idx;
- ListCell *lc2 = list_head(altBranchStarts);
+ int endPos = lfirst_int(lc);
+ int branchStart = lfirst_int(lc2);
- foreach(lc, altEndPositions)
+ if (pat->elements[endPos].next != RPR_ELEMIDX_INVALID)
{
- int endPos = lfirst_int(lc);
- int branchStart = lfirst_int(lc2);
-
- if (pat->elements[endPos].next != RPR_ELEMIDX_INVALID)
- {
- /*
- * An inner ALT already set next on this element. Redirect
- * all elements in this branch that share the same target to
- * point to after this ALT instead.
- */
- int oldTarget = pat->elements[endPos].next;
- int j;
+ /*
+ * An inner ALT already set next on this element. Redirect all
+ * elements in this branch that share the same target to point to
+ * after this ALT instead.
+ */
+ int oldTarget = pat->elements[endPos].next;
+ int j;
- for (j = branchStart; j <= endPos; j++)
- {
- if (pat->elements[j].next == oldTarget)
- pat->elements[j].next = afterAltIdx;
- }
- }
- else
+ for (j = branchStart; j <= endPos; j++)
{
- pat->elements[endPos].next = afterAltIdx;
+ if (pat->elements[j].next == oldTarget)
+ pat->elements[j].next = afterAltIdx;
}
-
- lc2 = lnext(altBranchStarts, lc2);
}
+ else
+ {
+ pat->elements[endPos].next = afterAltIdx;
+ }
+
+ lc2 = lnext(altBranchStarts, lc2);
}
list_free(altBranchStarts);
diff --git a/src/backend/parser/parse_rpr.c b/src/backend/parser/parse_rpr.c
index 9e1fa228759..027a3d2500a 100644
--- a/src/backend/parser/parse_rpr.c
+++ b/src/backend/parser/parse_rpr.c
@@ -267,6 +267,7 @@ transformDefineClause(ParseState *pstate, WindowClause *wc, WindowDef *windef,
{
TargetEntry *te,
*teDefine;
+ int defineExprLocation;
restarget = (ResTarget *) lfirst(lc);
name = restarget->name;
@@ -306,44 +307,41 @@ transformDefineClause(ParseState *pstate, WindowClause *wc, WindowDef *windef,
* BY), not the DEFINE clause. We need to preserve the DEFINE location
* for accurate error reporting.
*/
- {
- int defineExprLocation = exprLocation(restarget->val);
-
- te = findTargetlistEntrySQL99(pstate, restarget->val,
- targetlist, EXPR_KIND_RPR_DEFINE);
-
- /* -------------------
- * 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);
-
- /*
- * 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;
- }
+ defineExprLocation = exprLocation(restarget->val);
+
+ te = findTargetlistEntrySQL99(pstate, restarget->val,
+ targetlist, EXPR_KIND_RPR_DEFINE);
+
+ /* -------------------
+ * 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);
+
+ /*
+ * 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;
/* build transformed DEFINE clause (list of TargetEntry) */
defineClause = lappend(defineClause, teDefine);
--
2.50.1 (Apple Git-155)
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: Row pattern recognition
In-Reply-To: <CAAAe_zAc4CQDMP5WVuNZq5q6A_CymN5xwrva38df3okrtYdYsw@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox