MIME-Version: 1.0
References: 
 <CACJufxHVADC8e77pnQxSZRk7SYHCZFk6ZCM2HfTsKyD_kUji0A@mail.gmail.com>
 <CAAAe_zAZDuHSiVGvz9c6h=Pe=aN+FKZOrdNPfbTOk3XV+WFKYQ@mail.gmail.com>
 <CAAAe_zDz3z2Paidk3jHOm9S3eMVLoXRxK0Lyo=5i_9-EfSH7fA@mail.gmail.com>
 <20260604.132108.405136284364833955.ishii@postgresql.org>
 <CACJufxFpfVRSHMtB=L8ojW7NeCpkuq4PpRMMR_XFmf=uZ9xBTA@mail.gmail.com>
In-Reply-To: 
 <CACJufxFpfVRSHMtB=L8ojW7NeCpkuq4PpRMMR_XFmf=uZ9xBTA@mail.gmail.com>
Reply-To: assam258@gmail.com
From: Henson Choi <assam258@gmail.com>
Date: Thu, 4 Jun 2026 21:15:40 +0900
Message-ID: 
 <CAAAe_zBH-HtmPoTUPEsvozjyLptvX_rTRMOaPbyTnptj+mXoPw@mail.gmail.com>
Subject: Re: Row pattern recognition
To: jian he <jian.universality@gmail.com>
Cc: Tatsuo Ishii <ishii@postgresql.org>, 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="000000000000623c7106536c81f5"
Archived-At: 
 <https://www.postgresql.org/message-id/CAAAe_zBH-HtmPoTUPEsvozjyLptvX_rTRMOaPbyTnptj%2BmXoPw%40mail.gmail.com>
Precedence: bulk

--000000000000623c7106536c81f5
Content-Type: text/plain; charset="UTF-8"

> Please check the attached regression test refactoring and gram.y changes

Thanks for the patch -- the substance all looks right: the gram.y reformat
is a fair consistency fix, the ecb2508aaf convention (no hardcoded error
text in regress comments) is one we should follow, and the added coverage
(split-token quantifier errors, SRF in DEFINE) fills real gaps.

I'll work these in together with the patches I'm lining up right before the
v48 merge, rather than mid-stream -- the large "Expected: ERROR" comment
cleanup would otherwise churn against the other test edits still in flight.
They'll all land in that v48 round.

Answers to your three questions:

> In src/test/regress/sql/rpr_base.sql, wording such as ``Jacob's
> Patterns`` should be removed?

Those came in from Jacob's branch, but agreed they're better changed:
"Jacob's Patterns" / "from jacob branch" are dev-time provenance notes, not
something to keep in committed tests. I'll give the section a descriptive
header (the tests themselves stay) and do it together with the v48 round
above.

> --   Serialization/Deserialization Tests (objects kept for
pg_upgrade/pg_dump)
> I am not sure what this refers to.

Those are views (with RPR in their definition) left undropped on purpose,
so the pg_dump/pg_upgrade tests pick them up: dumping and restoring the
regression DB deparses the RPR window clause and re-parses it, exercising
the serialization round trip. rpr_explain.sql keeps one for the same
reason. I'll reword the comment to say that.

> errmsg("quantifier bound must be between 0 and %d", INT_MAX - 1),
> errmsg("quantifier bound must be between 1 and %d", INT_MAX - 1),
> Will these cause consistency issues?

No, it's intentional. INT_MAX is the unbounded sentinel (RPR_QUANTITY_INF),
so an explicit bound is capped at INT_MAX - 1 to avoid colliding with it;
the "0 vs 1" split just matches each form ({n}/{,m} need >= 1, {n,} allows
0).

Regards,
Henson

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

<div dir=3D"ltr">&gt; Please check the attached regression test refactoring=
 and gram.y changes<br><br>Thanks for the patch -- the substance all looks =
right: the gram.y reformat<br>is a fair consistency fix, the ecb2508aaf con=
vention (no hardcoded error<br>text in regress comments) is one we should f=
ollow, and the added coverage<br>(split-token quantifier errors, SRF in DEF=
INE) fills real gaps.<br><br>I&#39;ll work these in together with the patch=
es I&#39;m lining up right before the<br>v48 merge, rather than mid-stream =
-- the large &quot;Expected: ERROR&quot; comment<br>cleanup would otherwise=
 churn against the other test edits still in flight.<br>They&#39;ll all lan=
d in that v48 round.<br><br>Answers to your three questions:<br><br>&gt; In=
 src/test/regress/sql/rpr_base.sql, wording such as ``Jacob&#39;s<br>&gt; P=
atterns`` should be removed?<br><br>Those came in from Jacob&#39;s branch, =
but agreed they&#39;re better changed:<br>&quot;Jacob&#39;s Patterns&quot; =
/ &quot;from jacob branch&quot; are dev-time provenance notes, not<br>somet=
hing to keep in committed tests. I&#39;ll give the section a descriptive<br=
>header (the tests themselves stay) and do it together with the v48 round<b=
r>above.<br><br>&gt; -- =C2=A0 Serialization/Deserialization Tests (objects=
 kept for pg_upgrade/pg_dump)<br>&gt; I am not sure what this refers to.<br=
><br>Those are views (with RPR in their definition) left undropped on purpo=
se,<br>so the pg_dump/pg_upgrade tests pick them up: dumping and restoring =
the<br>regression DB deparses the RPR window clause and re-parses it, exerc=
ising<br>the serialization round trip. rpr_explain.sql keeps one for the sa=
me<br>reason. I&#39;ll reword the comment to say that.<br><br>&gt; errmsg(&=
quot;quantifier bound must be between 0 and %d&quot;, INT_MAX - 1),<br>&gt;=
 errmsg(&quot;quantifier bound must be between 1 and %d&quot;, INT_MAX - 1)=
,<br>&gt; Will these cause consistency issues?<br><br>No, it&#39;s intentio=
nal. INT_MAX is the unbounded sentinel (RPR_QUANTITY_INF),<br>so an explici=
t bound is capped at INT_MAX - 1 to avoid colliding with it;<br>the &quot;0=
 vs 1&quot; split just matches each form ({n}/{,m} need &gt;=3D 1, {n,} all=
ows 0).<br><br>Regards,<br>Henson</div>

--000000000000623c7106536c81f5--