MIME-Version: 1.0
References: 
 <CAAAe_zBH=6nh6Yg9EuohthTgzHgtbhtvLYAnabg6mCHQPLLpqQ@mail.gmail.com>
 <20260306.153837.2137322608184587391.ishii@postgresql.org>
 <CAAAe_zDgbDQf=RMtGw_axaNm1d_HY-e4=0ddDy809p7kTuUwHQ@mail.gmail.com>
 <20260307.120151.1477244845022229828.ishii@postgresql.org>
In-Reply-To: <20260307.120151.1477244845022229828.ishii@postgresql.org>
Reply-To: assam258@gmail.com
From: Henson Choi <assam258@gmail.com>
Date: Sun, 8 Mar 2026 01:02:41 +0900
Message-ID: 
 <CAAAe_zC6DEf-3mRYh6KJGO8ighVq13gpAhY0NGhTGVvCUKweGw@mail.gmail.com>
Subject: Re: Row pattern recognition
To: Tatsuo Ishii <ishii@postgresql.org>
Cc: sjjang112233@gmail.com, vik@postgresfriends.org, er@xs4all.nl,
	jacob.champion@enterprisedb.com, david.g.johnston@gmail.com,
	peter@eisentraut.org, pgsql-hackers@postgresql.org
Content-Type: multipart/alternative; boundary="000000000000608df3064c714deb"
Archived-At: 
 <https://www.postgresql.org/message-id/CAAAe_zC6DEf-3mRYh6KJGO8ighVq13gpAhY0NGhTGVvCUKweGw%40mail.gmail.com>
Precedence: bulk

--000000000000608df3064c714deb
Content-Type: text/plain; charset="UTF-8"

Hi Tatsuo,

I found Trino uses "empty match" too [2]. So for SQL users, I guess
> "empty match" is more familiar wording.
>

Agreed.  Since RPR is a feature defined by the SQL standard, using the
standard's own terminology is the right choice: it lets users
cross-reference the specification directly.  That said, "zero-length
match" is widely used in the regex literature (PCRE2, Perl, Python,
Java, etc.), so developers researching the topic should treat the two
terms as synonymous.  In the SQL context, "empty match" is the preferred
term.


> >   (a) Reorder within nodeWindowAgg.c: move the nfa_* functions up and
> >       keep the "API exposed to window functions" section at the bottom,
> >       matching master's layout.
> >
> >   (b) Separate file under src/backend/executor/, keeping it close to
> >       nodeWindowAgg.c while making the boundary explicit.
> >
> >   (c) A dedicated src/backend/rpr/ directory modeled on
> >       src/backend/regex/, giving the NFA engine its own namespace.
> >       This could also be an opportunity to consolidate the existing
> >       src/backend/optimizer/plan/rpr.c into the same directory.
>
> I prefer (a) or (b) for now, at least for the first commit. The reason
> is, current nfa functions take a WindowAggState argument. If we prefer
> (c), I think we need to change some of (or most of) nfa functions so
> that they do not take the WindowAggState argument. What do you think?
>

That is a valid concern.  (c) will be necessary in the long run, but
MATCH_RECOGNIZE (R010) is not yet on the horizon, and restructuring the
NFA layer now -- before that requirement is concrete -- carries a real
risk of introducing defects during the refactoring.  The risks outweigh the
benefits at this stage.

Instead, I'll try (b): a separate file under src/backend/executor/
can retain the WindowAggState argument while making the NFA boundary
explicit, and the change is still reviewable.  If (b) turns out to be
harder than expected or surfaces unexpected issues, I'll fall back to
(a).

If (b) works out, I'll add a structured header comment to the new file
to preserve the algorithm description for future reviewers.


I am not familiar with ECPG. Do you know if ECPG has Window clause
> tests? If ECPG does not have any Window clause tests, is it worth to
> add RPR tests to ECPG?
>

I checked src/interfaces/ecpg/test/ and confirmed there are no Window
clause tests there at all.  If ECPG has no Window clause coverage, I
see no reason to add RPR tests there at this stage.  I think it would be
better to leave ECPG tests out of this patch set for now.


Best regards,
Henson

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

<div dir=3D"ltr"><div dir=3D"ltr">Hi Tatsuo,</div><div dir=3D"ltr"><br></di=
v><div class=3D"gmail_quote gmail_quote_container"><blockquote class=3D"gma=
il_quote" style=3D"margin:0px 0px 0px 0.8ex;border-left-width:1px;border-le=
ft-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">
I found Trino uses &quot;empty match&quot; too [2]. So for SQL users, I gue=
ss<br>
&quot;empty match&quot; is more familiar wording.<br></blockquote><div><br>=
</div><div>Agreed.=C2=A0 Since RPR is a feature defined by the SQL standard=
, using the<br>standard&#39;s own terminology is the right choice: it lets =
users<br>cross-reference the specification directly.=C2=A0 That said, &quot=
;zero-length<br>match&quot; is widely used in the regex literature (PCRE2, =
Perl, Python,<br>Java, etc.), so developers researching the topic should tr=
eat the two<br>terms as synonymous.=C2=A0 In the SQL context, &quot;empty m=
atch&quot; is the preferred term.<br><br></div><div>=C2=A0</div><blockquote=
 class=3D"gmail_quote" style=3D"margin:0px 0px 0px 0.8ex;border-left-width:=
1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left=
:1ex">&gt;=C2=A0 =C2=A0(a) Reorder within nodeWindowAgg.c: move the nfa_* f=
unctions up and<br>
&gt;=C2=A0 =C2=A0 =C2=A0 =C2=A0keep the &quot;API exposed to window functio=
ns&quot; section at the bottom,<br>
&gt;=C2=A0 =C2=A0 =C2=A0 =C2=A0matching master&#39;s layout.<br>
&gt; <br>
&gt;=C2=A0 =C2=A0(b) Separate file under src/backend/executor/, keeping it =
close to<br>
&gt;=C2=A0 =C2=A0 =C2=A0 =C2=A0nodeWindowAgg.c while making the boundary ex=
plicit.<br>
&gt; <br>
&gt;=C2=A0 =C2=A0(c) A dedicated src/backend/rpr/ directory modeled on<br>
&gt;=C2=A0 =C2=A0 =C2=A0 =C2=A0src/backend/regex/, giving the NFA engine it=
s own namespace.<br>
&gt;=C2=A0 =C2=A0 =C2=A0 =C2=A0This could also be an opportunity to consoli=
date the existing<br>
&gt;=C2=A0 =C2=A0 =C2=A0 =C2=A0src/backend/optimizer/plan/rpr.c into the sa=
me directory.<br>
<br>
I prefer (a) or (b) for now, at least for the first commit. The reason<br>
is, current nfa functions take a WindowAggState argument. If we prefer<br>
(c), I think we need to change some of (or most of) nfa functions so<br>
that they do not take the WindowAggState argument. What do you think?<br></=
blockquote><div>=C2=A0</div><div>That is a valid concern. =C2=A0(c) will be=
 necessary in the long run, but<br>MATCH_RECOGNIZE (R010) is not yet on the=
 horizon, and restructuring the<br>NFA layer now -- before that requirement=
 is concrete -- carries a real<br>risk of introducing defects during the re=
factoring.=C2=A0 The risks outweigh the<br>benefits at this stage.<br><br>I=
nstead, I&#39;ll try (b): a separate file under src/backend/executor/<br>ca=
n retain the WindowAggState argument while making the NFA boundary<br>expli=
cit, and the change is still reviewable.=C2=A0 If (b) turns out to be<br>ha=
rder than expected or surfaces unexpected issues, I&#39;ll fall back to<br>=
(a).<br><br>If (b) works out, I&#39;ll add a structured header comment to t=
he new file<br>to preserve the algorithm description for future reviewers.<=
br></div><div><br></div><div><br></div><blockquote class=3D"gmail_quote" st=
yle=3D"margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:sol=
id;border-left-color:rgb(204,204,204);padding-left:1ex">I am not familiar w=
ith ECPG. Do you know if ECPG has Window clause<br>
tests? If ECPG does not have any Window clause tests, is it worth to<br>
add RPR tests to ECPG?<br></blockquote><div>=C2=A0</div><div>I checked src/=
interfaces/ecpg/test/ and confirmed there are no Window<br>clause tests the=
re at all.=C2=A0 If ECPG has no Window clause coverage, I<br>see no reason =
to add RPR tests there at this stage.=C2=A0 I think it would be better to l=
eave ECPG tests out of this patch set for now.<br><br><br>Best regards,<br>=
Henson<br></div></div></div>

--000000000000608df3064c714deb--