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>
 <CAAAe_zB4TMHnpaOrwYp7dKs553q2474ZXRytGfYOfYp4DdrgiQ@mail.gmail.com>
 <CACJufxFAQhbOD9EVCTAy-VwDbG4446N10GsxCcgdpFnjHO1Efw@mail.gmail.com>
 <CACJufxG6rKfd0WtsVZgzcoh8vOEwo_8UDWkuOd888ATavNv_uw@mail.gmail.com>
In-Reply-To: 
 <CACJufxG6rKfd0WtsVZgzcoh8vOEwo_8UDWkuOd888ATavNv_uw@mail.gmail.com>
Reply-To: assam258@gmail.com
From: Henson Choi <assam258@gmail.com>
Date: Thu, 18 Jun 2026 16:48:54 +0900
Message-ID: 
 <CAAAe_zApTOBLS9TCDM=YEvjTEBLa84gzRKspqTQSw6NK9iB+Ug@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="00000000000040eda70654826954"
Archived-At: 
 <https://www.postgresql.org/message-id/CAAAe_zApTOBLS9TCDM%3DYEvjTEBLa84gzRKspqTQSw6NK9iB%2BUg%40mail.gmail.com>
Precedence: bulk

--00000000000040eda70654826954
Content-Type: text/plain; charset="UTF-8"

Hi Jian,

Thanks for the misc-refactoring patch (v48-0001-v48-misc-refactoring) and
the
whole-row Var note.  As before, now that v48 is posted I'll rebase on it
and send
these as incremental patches on top (v49); per-item disposition below, with
the
per-commit breakdown to come with the patch.


> RPRNavExpr->resulttype should also marked as
pg_node_attr(query_jumble_ignore)

Agreed -- every other derived result-type field in primnodes.h already
carries it,
and resulttype is derived from the (jumbled) arg type, so ignoring it loses
nothing.
I'll add it.


> collectPatternVariables is not needed.
> The parser already ensures every DEFINE variable appears in PATTERN ...
> See nfa_evaluate_row the for loop break.

Confirmed -- the parser already rejects a DEFINE variable not used in
PATTERN, so
every DEFINE var is in PATTERN; filtering is redundant and cost_windowagg
can
iterate defineClause directly, so I'll remove it.


> buildDefineVariableList is trivial. No need to export it as an external
function.

Agreed -- I'll drop it and inline the list-building into
create_windowagg_plan().


> Rename WindowAggState.defineClauseList to defineClauseExprs

Agreed (the elements are ExprState, so it matches the ...Exprs convention)
-- I'll
do it, including the additional site the dormant-match fix touches.


> Flatten a needlessly nested block in show_window_def().
> Replace a post-loop ListCell NULL check in
remove_unused_subquery_outputs()
> with a boolean flag.
> Reduce the number of arguments in make_windowagg.
> Minor refactoring of regress test comments.

I'll do all of these.

One exception: the parsenodes.h RPRPatternNode comment names the data
structure, so
rather than "clause" I'll change it to "parse tree node".


> dvar, var both can be whole-row Vars, but this seems to work for
whole-row vars.
> We need some simple regress tests for cases where both are whole-row vars
or one
> of them is a whole-row var.

Good catch -- I'll add regress coverage for the whole-row Var cases (both
sides
whole-row, and one side whole-row) in the needed_by_define check, in v49.

I'll send the items above as a first incremental patch on top of v48, and
take up
the later reviews after that.


Thanks,
Henson

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

<div dir=3D"ltr">Hi Jian,<br><br>Thanks for the misc-refactoring patch (v48=
-0001-v48-misc-refactoring) and the<br>whole-row Var note.=C2=A0 As before,=
 now that v48 is posted I&#39;ll rebase on it and send<br>these as incremen=
tal patches on top (v49); per-item disposition below, with the<br>per-commi=
t breakdown to come with the patch.<br><br><br>&gt; RPRNavExpr-&gt;resultty=
pe should also marked as pg_node_attr(query_jumble_ignore)<br><br>Agreed --=
 every other derived result-type field in primnodes.h already carries it,<b=
r>and resulttype is derived from the (jumbled) arg type, so ignoring it los=
es nothing.<br>I&#39;ll add it.<br><br><br>&gt; collectPatternVariables is =
not needed.<br>&gt; The parser already ensures every DEFINE variable appear=
s in PATTERN ...<br>&gt; See nfa_evaluate_row the for loop break.<br><br>Co=
nfirmed -- the parser already rejects a DEFINE variable not used in PATTERN=
, so<br>every DEFINE var is in PATTERN; filtering is redundant and cost_win=
dowagg can<br>iterate defineClause directly, so I&#39;ll remove it.<br><br>=
<br>&gt; buildDefineVariableList is trivial. No need to export it as an ext=
ernal function.<br><br>Agreed -- I&#39;ll drop it and inline the list-build=
ing into create_windowagg_plan().<br><br><br>&gt; Rename WindowAggState.def=
ineClauseList to defineClauseExprs<br><br>Agreed (the elements are ExprStat=
e, so it matches the ...Exprs convention) -- I&#39;ll<br>do it, including t=
he additional site the dormant-match fix touches.<br><br><br>&gt; Flatten a=
 needlessly nested block in show_window_def().<br>&gt; Replace a post-loop =
ListCell NULL check in remove_unused_subquery_outputs()<br>&gt; with a bool=
ean flag.<br>&gt; Reduce the number of arguments in make_windowagg.<br>&gt;=
 Minor refactoring of regress test comments.<br><br>I&#39;ll do all of thes=
e.<br><br>One exception: the parsenodes.h RPRPatternNode comment names the =
data structure, so<br>rather than &quot;clause&quot; I&#39;ll change it to =
&quot;parse tree node&quot;.<br><br><br>&gt; dvar, var both can be whole-ro=
w Vars, but this seems to work for whole-row vars.<br>&gt; We need some sim=
ple regress tests for cases where both are whole-row vars or one<br>&gt; of=
 them is a whole-row var.<br><br>Good catch -- I&#39;ll add regress coverag=
e for the whole-row Var cases (both sides<br>whole-row, and one side whole-=
row) in the needed_by_define check, in v49.<br><br>I&#39;ll send the items =
above as a first incremental patch on top of v48, and take up<br>the later =
reviews after that.<br><br><br>Thanks,<br>Henson<br></div>

--00000000000040eda70654826954--