MIME-Version: 1.0
References: 
 <CAAAe_zAZDuHSiVGvz9c6h=Pe=aN+FKZOrdNPfbTOk3XV+WFKYQ@mail.gmail.com>
 <CAAAe_zDz3z2Paidk3jHOm9S3eMVLoXRxK0Lyo=5i_9-EfSH7fA@mail.gmail.com>
 <20260604.132108.405136284364833955.ishii@postgresql.org>
 <20260609.171307.1883356507067957349.ishii@postgresql.org>
 <CACJufxFnwdQSApt2vWwYCd0gtf+JjFDxT2hbxHi=+dhFJc+-1g@mail.gmail.com>
 <CAAAe_zATnkqsbLYDj8MJV1TriX9Wi0wShDg3nK3qYpiupKwhFA@mail.gmail.com>
 <CAAAe_zBL+J0AYmvmcJQT7Q-gp5aRH0deJ7SE7-N21g4hWExyJw@mail.gmail.com>
 <CACJufxHJFNBJ0vHCCLestWV5b7DF5e4VzfmovqGKBGgqg+rcGA@mail.gmail.com>
 <CAAAe_zBY0rrgf+tKXMUc-Y3nDDD69hddRBKopEKAZobhY=Cy-Q@mail.gmail.com>
 <CAAAe_zDYxq0d3exCDwvKncD0kaL2uehDir6HXo4r5DXMitKrSg@mail.gmail.com>
 <CACJufxG57=ddtbN=5RZCzhxWDYXvocKmB7NtZy+DoqZuhxb_DA@mail.gmail.com>
In-Reply-To: 
 <CACJufxG57=ddtbN=5RZCzhxWDYXvocKmB7NtZy+DoqZuhxb_DA@mail.gmail.com>
Reply-To: assam258@gmail.com
From: Henson Choi <assam258@gmail.com>
Date: Mon, 15 Jun 2026 10:56:12 +0900
Message-ID: 
 <CAAAe_zB4TMHnpaOrwYp7dKs553q2474ZXRytGfYOfYp4DdrgiQ@mail.gmail.com>
Subject: Re: Row pattern recognition
To: jian he <jian.universality@gmail.com>, Tatsuo Ishii <ishii@postgresql.org>
Cc: zsolt.parragi@percona.com, sjjang112233@gmail.com,
 vik@postgresfriends.org,
	er@xs4all.nl, jacob.champion@enterprisedb.com, david.g.johnston@gmail.com,
	peter@eisentraut.org, li.evan.chao@gmail.com, pgsql-hackers@postgresql.org
Content-Type: multipart/alternative; boundary="00000000000040539b06544122ab"
Archived-At: 
 <https://www.postgresql.org/message-id/CAAAe_zB4TMHnpaOrwYp7dKs553q2474ZXRytGfYOfYp4DdrgiQ%40mail.gmail.com>
Precedence: bulk

--00000000000040539b06544122ab
Content-Type: text/plain; charset="UTF-8"

Hi jian,

Thanks for the follow-up review patches. Once v48 is posted I'll base the
work on it and submit the accepted ones in v49. Reasoning per patch below,
with the disposition summarized at the end.

> v47-0001 Replace some "AST" to "clause".

Agreed on dropping "AST" -- it isn't our house term; "parse tree" is the
idiom across the tree, and "AST" barely shows up outside these RPR files
(just a jsonpath comment). One tweak: where the text names the data
structure (the README diagram and III-2 header, the parsenodes.h comments
on rpPattern), I'd use "PATTERN parse tree" rather than "clause", since
"clause" loses the tree-vs-flat-NFA contrast. Plain prose that just means
the SQL clause -> "clause", as you have it.

> v47-0002 group Op and trailing ALT into one IF condition, this might
> improve readability.

Here I'd lean toward keeping the current form. The complexity here comes
from the lexer (regex operators tokenized by the SQL operator lexer), and
grouping the branches doesn't remove it -- it just layers more cleverness
on top. I'd rather not add code complexity onto the lexer complexity; the
flat one-token-one-branch form stays easy to scan. A real simplification
would be lexer-side, out of scope here, so I'd leave 0002 as-is unless you
feel strongly.

> v47-0003 validate_rpr_define_volatility ... is removed.
> contain_volatile_functions() already covers both volatile FuncExpr
> callees and NextValueExpr ... The trade-off is that we lose the error
> cursor position, but that seems better than maintaining extra code.

You're right the detection is equivalent. But this restructures the
parse-side define_walker, which the upcoming PREV/NEXT name-binding fix
also reworks -- it even removes the spot where this check incidentally
backstops a nav mis-binding. I'd rather make the simplify-vs-keep call
against that final walker shape than churn it twice, so I'll keep the
current planner-side check for now and revisit once that fix lands.

> v47-0004 validateRPRPatternVarCount() ... the `rpDefs != NULL`
> sentinel ... is awkward. That cross-check only needs to run once;
> better expressed in the caller, transformDefineClause().

Agreed. The cross-check is a one-shot, list-level test -- a different kind
of thing from the recursive per-node count, and not really what
validateRPRPatternVarCount is for (validating the pattern var count).
Moving it to the caller runs it once by construction and leaves the
function true to its name, a pure count. Rejection behavior is unchanged.

> v47-0005 ... The more intuitive order should be: transformExpr ->
> coerce_to_boolean -> pull_var_clause, so pull_var_clause always sees
> the final expression form.

Agreed, that order is more intuitive. I'll make the change and watch for
any change in error output or behavior -- coerce_to_boolean now runs
per-define before pull_var_clause rather than in a second pass -- with
guard tests for domain-over-bool, nav Var preserved, and non-coercible
error, and submit it in v49 if those and the regression pass clean.

> v47-0006 ... has_column_ref is not necessary. Column reference checks
> can use contain_var_clause, and it's cheap. Also the message change:
> -row pattern navigation offset must be a run-time constant
> +row pattern navigation offset expression must not contain column
>  references

Agreed contain_var_clause is simpler and the new message reads better.
But this restructures define_walker too, which the nav name-binding fix
also reworks -- so as with 0003 I'd rather settle it against the post-fix
walker shape than churn it twice, and revisit once that fix lands.

Summary:

  0001  -> v49   (use "parse tree" where it names the structure, not
"clause")
  0002  -> keep the current flat form
  0003  -> revisit after the nav name-binding fix
  0004  -> v49
  0005  -> v49   (after guard tests + regression pass)
  0006  -> revisit after the nav name-binding fix

For 0003 and 0006 -- both swap a hand-rolled walker for a standard helper
(contain_volatile_functions, contain_var_clause) -- I'll think this
through properly when I rework the walker: the convention of reaching for
those helpers against what the current checks buy us (the error cursor,
the specific messages), rather than deciding it by reflex.

More broadly, the substantive correctness defects take priority: the nav
name-binding fix and the rest of that series come ahead of these cleanup
refactors. Once v48 is posted I'll write up the current list of known
issues and send it to the thread.

Thanks again for the careful pass.

Best,
Henson

--00000000000040539b06544122ab
Content-Type: text/html; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

<div dir=3D"ltr">Hi jian,<br><br>Thanks for the follow-up review patches. O=
nce v48 is posted I&#39;ll base the<br>work on it and submit the accepted o=
nes in v49. Reasoning per patch below,<br>with the disposition summarized a=
t the end.<br><br>&gt; v47-0001 Replace some &quot;AST&quot; to &quot;claus=
e&quot;.<br><br>Agreed on dropping &quot;AST&quot; -- it isn&#39;t our hous=
e term; &quot;parse tree&quot; is the<br>idiom across the tree, and &quot;A=
ST&quot; barely shows up outside these RPR files<br>(just a jsonpath commen=
t). One tweak: where the text names the data<br>structure (the README diagr=
am and III-2 header, the parsenodes.h comments<br>on rpPattern), I&#39;d us=
e &quot;PATTERN parse tree&quot; rather than &quot;clause&quot;, since<br>&=
quot;clause&quot; loses the tree-vs-flat-NFA contrast. Plain prose that jus=
t means<br>the SQL clause -&gt; &quot;clause&quot;, as you have it.<br><br>=
&gt; v47-0002 group Op and trailing ALT into one IF condition, this might<b=
r>&gt; improve readability.<br><br>Here I&#39;d lean toward keeping the cur=
rent form. The complexity here comes<br>from the lexer (regex operators tok=
enized by the SQL operator lexer), and<br>grouping the branches doesn&#39;t=
 remove it -- it just layers more cleverness<br>on top. I&#39;d rather not =
add code complexity onto the lexer complexity; the<br>flat one-token-one-br=
anch form stays easy to scan. A real simplification<br>would be lexer-side,=
 out of scope here, so I&#39;d leave 0002 as-is unless you<br>feel strongly=
.<br><br>&gt; v47-0003 validate_rpr_define_volatility ... is removed.<br>&g=
t; contain_volatile_functions() already covers both volatile FuncExpr<br>&g=
t; callees and NextValueExpr ... The trade-off is that we lose the error<br=
>&gt; cursor position, but that seems better than maintaining extra code.<b=
r><br>You&#39;re right the detection is equivalent. But this restructures t=
he<br>parse-side define_walker, which the upcoming PREV/NEXT name-binding f=
ix<br>also reworks -- it even removes the spot where this check incidentall=
y<br>backstops a nav mis-binding. I&#39;d rather make the simplify-vs-keep =
call<br>against that final walker shape than churn it twice, so I&#39;ll ke=
ep the<br>current planner-side check for now and revisit once that fix land=
s.<br><br>&gt; v47-0004 validateRPRPatternVarCount() ... the `rpDefs !=3D N=
ULL`<br>&gt; sentinel ... is awkward. That cross-check only needs to run on=
ce;<br>&gt; better expressed in the caller, transformDefineClause().<br><br=
>Agreed. The cross-check is a one-shot, list-level test -- a different kind=
<br>of thing from the recursive per-node count, and not really what<br>vali=
dateRPRPatternVarCount is for (validating the pattern var count).<br>Moving=
 it to the caller runs it once by construction and leaves the<br>function t=
rue to its name, a pure count. Rejection behavior is unchanged.<br><br>&gt;=
 v47-0005 ... The more intuitive order should be: transformExpr -&gt;<br>&g=
t; coerce_to_boolean -&gt; pull_var_clause, so pull_var_clause always sees<=
br>&gt; the final expression form.<br><br>Agreed, that order is more intuit=
ive. I&#39;ll make the change and watch for<br>any change in error output o=
r behavior -- coerce_to_boolean now runs<br>per-define before pull_var_clau=
se rather than in a second pass -- with<br>guard tests for domain-over-bool=
, nav Var preserved, and non-coercible<br>error, and submit it in v49 if th=
ose and the regression pass clean.<br><br>&gt; v47-0006 ... has_column_ref =
is not necessary. Column reference checks<br>&gt; can use contain_var_claus=
e, and it&#39;s cheap. Also the message change:<br>&gt; -row pattern naviga=
tion offset must be a run-time constant<br>&gt; +row pattern navigation off=
set expression must not contain column<br>&gt; =C2=A0references<br><br>Agre=
ed contain_var_clause is simpler and the new message reads better.<br>But t=
his restructures define_walker too, which the nav name-binding fix<br>also =
reworks -- so as with 0003 I&#39;d rather settle it against the post-fix<br=
>walker shape than churn it twice, and revisit once that fix lands.<br><br>=
Summary:<br><br><font face=3D"monospace">=C2=A0 0001 =C2=A0-&gt; v49 =C2=A0=
 (use &quot;parse tree&quot; where it names the structure, not &quot;clause=
&quot;)<br>=C2=A0 0002 =C2=A0-&gt; keep the current flat form<br>=C2=A0 000=
3 =C2=A0-&gt; revisit after the nav name-binding fix<br>=C2=A0 0004 =C2=A0-=
&gt; v49<br>=C2=A0 0005 =C2=A0-&gt; v49 =C2=A0 (after guard tests + regress=
ion pass)<br>=C2=A0 0006 =C2=A0-&gt; revisit after the nav name-binding fix=
</font><br><br>For 0003 and 0006 -- both swap a hand-rolled walker for a st=
andard helper<br>(contain_volatile_functions, contain_var_clause) -- I&#39;=
ll think this<br>through properly when I rework the walker: the convention =
of reaching for<br>those helpers against what the current checks buy us (th=
e error cursor,<br>the specific messages), rather than deciding it by refle=
x.<br><br>More broadly, the substantive correctness defects take priority: =
the nav<br>name-binding fix and the rest of that series come ahead of these=
 cleanup<br>refactors. Once v48 is posted I&#39;ll write up the current lis=
t of known<br>issues and send it to the thread.<br><br>Thanks again for the=
 careful pass.<br><br>Best,<br>Henson<br></div>

--00000000000040539b06544122ab--