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>
In-Reply-To: 
 <CACJufxFnwdQSApt2vWwYCd0gtf+JjFDxT2hbxHi=+dhFJc+-1g@mail.gmail.com>
Reply-To: assam258@gmail.com
From: Henson Choi <assam258@gmail.com>
Date: Wed, 10 Jun 2026 00:15:31 +0900
Message-ID: 
 <CAAAe_zATnkqsbLYDj8MJV1TriX9Wi0wShDg3nK3qYpiupKwhFA@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="000000000000c9cdc90653d39981"
Archived-At: 
 <https://www.postgresql.org/message-id/CAAAe_zATnkqsbLYDj8MJV1TriX9Wi0wShDg3nK3qYpiupKwhFA%40mail.gmail.com>
Precedence: bulk

--000000000000c9cdc90653d39981
Content-Type: text/plain; charset="UTF-8"

Re: Row pattern recognition
To: jian he, Tatsuo Ishii (+ pgsql-hackers)

Hi jian, Tatsuo,

Thanks for the careful review -- replies inline. One question up front,
for Tatsuo: I'd like your take on the LOWPRICE/UP/DOWN casing below
(details at that point). The rest are replies to jian's points.

> The below in advanced.sgml actually seems better suited for
> explain.sgml (which currently has no changes).
> At the very least, explain.sgml should mention it.

Agreed; I'll move the EXPLAIN marker description over to explain.sgml.

> LOWPRICE, UP, DOWN should be lower case, since they are not keywords?

They're user identifiers, and the standard and Oracle docs show them in
upper case, so I lean toward leaving the example as written -- though
EXPLAIN and deparse do lower-case them today. Tatsuo, I'd defer the
casing to you.

> scanRPRPatternRecursive
> It would be better to replace all ``non-trivial quantifier`` to
> ``non-trivial quantifier (not {1,1}).``

Agreed, that's clearer; will do.

> I want to rename allocateRPRPattern to makeRPRPattern, what do you think?

Good idea -- makeRPRPattern fits the makeNode/makeXxx convention.

> RPRPatternNode.reluctant_location is not necessary.

Agreed. The field is only written and propagated, never read back, so
I'll drop it and keep the reluctance as a plain bool.

> Is nav_volatile_func_checker really necessary? ... The convention is
> not to check expression volatility during parse analysis stage.

Good point -- I'll move the volatility rejection out of parse analysis
and into the planner, in line with that convention.

> query_jumble_ignore is not needed because it's only manipulated in
> gram.y ...

Right; I'll drop the query_jumble_ignore attribute.

> v47-0001 ... Change errmsg to errmsg("invalid token \"%s\" after range
> quantifier", $5) will make the error message clearer.

Agreed, that reads better; I'll adjust the message accordingly.

> v47-0002 ... We should just drop these comments from the regress tests.

Agreed -- the internal function names don't belong in the tests; I'll
remove them.

> v47-0003 ... mutate them in place using list_delete_nth_cell ...
> for overflow handling ... pg_mul_s32_overflow, pg_add_s32_overflow.

The overflow helpers are a clear win. The in-place list mutation is
behavior-equivalent, but I'd rather not reshape that logic right now
without a concrete need, so I'll hold off on it for the moment. The new
tests are good and I'll fold them in.

More broadly, I see the patch as being in its final verification stage
at this point, so I'd prefer to limit logic changes to those backed by a
clear review or test finding.

> v47-0004 ... Some misc refactoring ...

Mostly agree -- the rpSkipTo/initial comments read more accurately as
flag assignments. I'd keep the "PATTERN AST" wording (it's an AST, not a
flag) and the deparse note, but the rest is fine.

Best,
Henson

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

<div dir=3D"ltr">Re: Row pattern recognition<br>To: jian he, Tatsuo Ishii (=
+ pgsql-hackers)<br><br>Hi jian, Tatsuo,<br><br>Thanks for the careful revi=
ew -- replies inline. One question up front,<br>for Tatsuo: I&#39;d like yo=
ur take on the LOWPRICE/UP/DOWN casing below<br>(details at that point). Th=
e rest are replies to jian&#39;s points.<br><br>&gt; The below in advanced.=
sgml actually seems better suited for<br>&gt; explain.sgml (which currently=
 has no changes).<br>&gt; At the very least, explain.sgml should mention it=
.<br><br>Agreed; I&#39;ll move the EXPLAIN marker description over to expla=
in.sgml.<br><br>&gt; LOWPRICE, UP, DOWN should be lower case, since they ar=
e not keywords?<br><br>They&#39;re user identifiers, and the standard and O=
racle docs show them in<br>upper case, so I lean toward leaving the example=
 as written -- though<br>EXPLAIN and deparse do lower-case them today. Tats=
uo, I&#39;d defer the<br>casing to you.<br><br>&gt; scanRPRPatternRecursive=
<br>&gt; It would be better to replace all ``non-trivial quantifier`` to<br=
>&gt; ``non-trivial quantifier (not {1,1}).``<br><br>Agreed, that&#39;s cle=
arer; will do.<br><br>&gt; I want to rename allocateRPRPattern to makeRPRPa=
ttern, what do you think?<br><br>Good idea -- makeRPRPattern fits the makeN=
ode/makeXxx convention.<br><br>&gt; RPRPatternNode.reluctant_location is no=
t necessary.<br><br>Agreed. The field is only written and propagated, never=
 read back, so<br>I&#39;ll drop it and keep the reluctance as a plain bool.=
<br><br>&gt; Is nav_volatile_func_checker really necessary? ... The convent=
ion is<br>&gt; not to check expression volatility during parse analysis sta=
ge.<br><br>Good point -- I&#39;ll move the volatility rejection out of pars=
e analysis<br>and into the planner, in line with that convention.<br><br>&g=
t; query_jumble_ignore is not needed because it&#39;s only manipulated in<b=
r>&gt; gram.y ...<br><br>Right; I&#39;ll drop the query_jumble_ignore attri=
bute.<br><br>&gt; v47-0001 ... Change errmsg to errmsg(&quot;invalid token =
\&quot;%s\&quot; after range<br>&gt; quantifier&quot;, $5) will make the er=
ror message clearer.<br><br>Agreed, that reads better; I&#39;ll adjust the =
message accordingly.<br><br>&gt; v47-0002 ... We should just drop these com=
ments from the regress tests.<br><br>Agreed -- the internal function names =
don&#39;t belong in the tests; I&#39;ll<br>remove them.<br><br>&gt; v47-000=
3 ... mutate them in place using list_delete_nth_cell ...<br>&gt; for overf=
low handling ... pg_mul_s32_overflow, pg_add_s32_overflow.<br><br>The overf=
low helpers are a clear win. The in-place list mutation is<br>behavior-equi=
valent, but I&#39;d rather not reshape that logic right now<br>without a co=
ncrete need, so I&#39;ll hold off on it for the moment. The new<br>tests ar=
e good and I&#39;ll fold them in.<br><br>More broadly, I see the patch as b=
eing in its final verification stage<br>at this point, so I&#39;d prefer to=
 limit logic changes to those backed by a<br>clear review or test finding.<=
br><br>&gt; v47-0004 ... Some misc refactoring ...<br><br>Mostly agree -- t=
he rpSkipTo/initial comments read more accurately as<br>flag assignments. I=
&#39;d keep the &quot;PATTERN AST&quot; wording (it&#39;s an AST, not a<br>=
flag) and the deparse note, but the rest is fine.<br><br>Best,<br>Henson<br=
></div>

--000000000000c9cdc90653d39981--