MIME-Version: 1.0
References: 
 <CAAAe_zCsaf=WedELLjqLe3BV_8dWiO1DPDGA9sXj4qhe+=-XXw@mail.gmail.com>
 <20260622.215536.1729984032454200237.ishii@postgresql.org>
 <CAAAe_zChW=nM9J18wdF2wMa2KesFO53fkbVRudkJ_bstdxzfOQ@mail.gmail.com>
 <20260629.183730.1799088184925453299.ishii@postgresql.org>
 <CAAAe_zB0X_DGOxng3wUgAY59X80VwwYBDZLvUJQVApqMWnGx3A@mail.gmail.com>
 <CACJufxHTma0-00CO2TQoTByBcXp_UNqJwTF4=Y9MFhiqUM7vrQ@mail.gmail.com>
In-Reply-To: 
 <CACJufxHTma0-00CO2TQoTByBcXp_UNqJwTF4=Y9MFhiqUM7vrQ@mail.gmail.com>
Reply-To: assam258@gmail.com
From: Henson Choi <assam258@gmail.com>
Date: Thu, 2 Jul 2026 09:23:28 +0900
Message-ID: 
 <CAAAe_zCtsuY0odaocp6PiR6HOPG5CCSzFoOCL3P4cTq93mcEFg@mail.gmail.com>
Subject: Re: Row pattern recognition
To: jian.universality@gmail.com
Cc: ishii@postgresql.org, pgsql-hackers@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
Content-Type: multipart/alternative; boundary="0000000000000aa767065595d230"
Archived-At: 
 <https://www.postgresql.org/message-id/CAAAe_zCtsuY0odaocp6PiR6HOPG5CCSzFoOCL3P4cTq93mcEFg%40mail.gmail.com>
Precedence: bulk

--0000000000000aa767065595d230
Content-Type: text/plain; charset="UTF-8"

Hi Jian,

Thanks for the review, and for basing it on the RPR branch. Responses
inline.

> ``(only possible during early development/testing).`` comment is not
necessary?

Agreed, and it is actually misleading: varMatched == NULL is a live
signal, not a dev-only artifact. nfa_match() is called with NULL to force
every VAR to not-match at a frame boundary (execRPR.c, "Frame boundary
reached: force mismatch"). I will rewrite the comment to describe that
meaning instead of the "early development/testing" note.

> nfa_match desperately needs an extra i(nt64 currentPos) argument. [...]
> adding currentPos makes it much easier to understand the current
> execution state when debugging with GDB.

Makes sense -- nfa_match() has only the state list in scope, no row
index, which is exactly the thing that is hard to place in GDB. I will
thread currentPos through.

> The comments above in nfa_match are unnecessary because in ``if
> (RPRElemIsAbsorbableBranch(elem)`` we already have many comments, and
> the meaning seems the same.

Agreed -- the loop-top overview's second sentence restates the inline
END-chain block. I will drop it and keep "Evaluate VAR elements against
current row" as a one-line lead.

> Similar to REG_DEBUG, we really need an RPR_DEBUG option. [...] verify
> that ctx->states is not a circular linked list [...] without affecting
> release builds.

Fair point, and there is some history: RPR_DEBUG did exist earlier as
debug logging, and I am the one who proposed removing it. My reason was
narrow, though. I lean on the debugger, and with AI assistance the cost of
inserting, watching, and then removing ad-hoc logs has dropped sharply --
so I treated those logs as ephemeral and optimized for a clean committed
tree rather than for someone reading execRPR.c cold.

For a new contributor that trade-off is the wrong one, and you have
convinced me: a retained set of observation points, plus the Assert-style
sanity checks you describe (non-circular state list, count and
context-ordering invariants), would genuinely lower the barrier to this
code. So I agree with the direction. Rather than settle on a shape now, I
would leave it open: which observation points and checks are actually
worth retaining behind #ifdef RPR_DEBUG can be picked out later as a
separate follow-up, and it is really Tatsuo's call since he suggested the
first version.

> <para> The SQL standard defines more subclauses: MEASURES and SUBSET
> [...] more variations in AFTER MATCH clause. </para>
> This part should stay in the Compatibility section.

Agreed. It is a standard-deviation note, and the SELECT Compatibility
section currently has no row-pattern text, so I will move this paragraph
there.

Unless Tatsuo has a different view by the weekend, I will proceed with the
concrete changes above, leaving the RPR_DEBUG shape open as noted.

Best regards,
Henson

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

<div dir=3D"ltr"><div dir=3D"ltr"><div dir=3D"auto"><font face=3D"monospace=
">Hi Jian,<br><br>Thanks for the review, and for basing it on the RPR branc=
h. Responses<br>inline.<br><br>&gt; ``(only possible during early developme=
nt/testing).`` comment is not necessary?<br><br>Agreed, and it is actually =
misleading: varMatched =3D=3D NULL is a live<br>signal, not a dev-only arti=
fact. nfa_match() is called with NULL to force<br>every VAR to not-match at=
 a frame boundary (execRPR.c, &quot;Frame boundary<br>reached: force mismat=
ch&quot;). I will rewrite the comment to describe that<br>meaning instead o=
f the &quot;early development/testing&quot; note.<br><br>&gt; nfa_match des=
perately needs an extra i(nt64 currentPos) argument. [...]<br>&gt; adding c=
urrentPos makes it much easier to understand the current<br>&gt; execution =
state when debugging with GDB.<br><br>Makes sense -- nfa_match() has only t=
he state list in scope, no row<br>index, which is exactly the thing that is=
 hard to place in GDB. I will<br>thread currentPos through.<br><br>&gt; The=
 comments above in nfa_match are unnecessary because in ``if<br>&gt; (RPREl=
emIsAbsorbableBranch(elem)`` we already have many comments, and<br>&gt; the=
 meaning seems the same.<br><br>Agreed -- the loop-top overview&#39;s secon=
d sentence restates the inline<br>END-chain block. I will drop it and keep =
&quot;Evaluate VAR elements against<br>current row&quot; as a one-line lead=
.<br><br>&gt; Similar to REG_DEBUG, we really need an RPR_DEBUG option. [..=
.] verify<br>&gt; that ctx-&gt;states is not a circular linked list [...] w=
ithout affecting<br>&gt; release builds.<br><br>Fair point, and there is so=
me history: RPR_DEBUG did exist earlier as<br>debug logging, and I am the o=
ne who proposed removing it. My reason was<br>narrow, though. I lean on the=
 debugger, and with AI assistance the cost of<br>inserting, watching, and t=
hen removing ad-hoc logs has dropped sharply --<br>so I treated those logs =
as ephemeral and optimized for a clean committed<br>tree rather than for so=
meone reading execRPR.c cold.<br><br>For a new contributor that trade-off i=
s the wrong one, and you have<br>convinced me: a retained set of observatio=
n points, plus the Assert-style<br>sanity checks you describe (non-circular=
 state list, count and<br>context-ordering invariants), would genuinely low=
er the barrier to this<br>code. So I agree with the direction. Rather than =
settle on a shape now, I<br>would leave it open: which observation points a=
nd checks are actually<br>worth retaining behind #ifdef RPR_DEBUG can be pi=
cked out later as a<br>separate follow-up, and it is really Tatsuo&#39;s ca=
ll since he suggested the<br>first version.<br><br>&gt; &lt;para&gt; The SQ=
L standard defines more subclauses: MEASURES and SUBSET<br>&gt; [...] more =
variations in AFTER MATCH clause. &lt;/para&gt;<br>&gt; This part should st=
ay in the Compatibility section.<br><br>Agreed. It is a standard-deviation =
note, and the SELECT Compatibility<br>section currently has no row-pattern =
text, so I will move this paragraph<br>there.<br><br>Unless Tatsuo has a di=
fferent view by the weekend, I will proceed with the<br>concrete changes ab=
ove, leaving the RPR_DEBUG shape open as noted.<br><br>Best regards,<br>Hen=
son</font><br></div></div></div>

--0000000000000aa767065595d230--