public inbox for [email protected]
help / color / mirror / Atom feedFrom: Henson Choi <[email protected]>
To: jian he <[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]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Subject: Re: Row pattern recognition
Date: Wed, 10 Jun 2026 00:15:31 +0900
Message-ID: <CAAAe_zATnkqsbLYDj8MJV1TriX9Wi0wShDg3nK3qYpiupKwhFA@mail.gmail.com> (raw)
In-Reply-To: <CACJufxFnwdQSApt2vWwYCd0gtf+JjFDxT2hbxHi=+dhFJc+-1g@mail.gmail.com>
References: <CAAAe_zAZDuHSiVGvz9c6h=Pe=aN+FKZOrdNPfbTOk3XV+WFKYQ@mail.gmail.com>
<CAAAe_zDz3z2Paidk3jHOm9S3eMVLoXRxK0Lyo=5i_9-EfSH7fA@mail.gmail.com>
<[email protected]>
<[email protected]>
<CACJufxFnwdQSApt2vWwYCd0gtf+JjFDxT2hbxHi=+dhFJc+-1g@mail.gmail.com>
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
view thread (109+ messages) latest in thread
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], [email protected], [email protected], [email protected], [email protected]
Subject: Re: Row pattern recognition
In-Reply-To: <CAAAe_zATnkqsbLYDj8MJV1TriX9Wi0wShDg3nK3qYpiupKwhFA@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