MIME-Version: 1.0
References: 
 <CAAAe_zAFv0E_EhFxZueJ1n9pZpPaTVChMhDmndzugzovgz0o2Q@mail.gmail.com>
 <20260312.092125.2016894123875469735.ishii@postgresql.org>
 <CAAAe_zBswm=JGsBi3Hn-8kVbH3LgAcOvWJ3nnoAeWbiwR7NOig@mail.gmail.com>
 <20260312.105500.450470685315646651.ishii@postgresql.org>
In-Reply-To: <20260312.105500.450470685315646651.ishii@postgresql.org>
Reply-To: assam258@gmail.com
From: Henson Choi <assam258@gmail.com>
Date: Fri, 13 Mar 2026 09:53:39 +0900
Message-ID: 
 <CAAAe_zC001P9_rpKwAiq=+hOvTV_orBchbzipZnRRzdU-UWg1A@mail.gmail.com>
Subject: Re: Row pattern recognition
To: Tatsuo Ishii <ishii@postgresql.org>
Cc: zsolt.parragi@percona.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/mixed; boundary="0000000000007cb481064cdd4d24"
Archived-At: 
 <https://www.postgresql.org/message-id/CAAAe_zC001P9_rpKwAiq%3D%2BhOvTV_orBchbzipZnRRzdU-UWg1A%40mail.gmail.com>
Precedence: bulk

--0000000000007cb481064cdd4d24
Content-Type: multipart/alternative; boundary="0000000000007cb47f064cdd4d22"

--0000000000007cb47f064cdd4d22
Content-Type: text/plain; charset="UTF-8"

Hi Tatsuo,

Thank you for your thoughtful feedback on the tle identity concern.

Ok. we should be consistent with WHERE etc. unless the standard
> requres DEFINE stricter. But if the tle is in the target list
> (possibly with resjunk attribute), it could be different from the one
> in the DEFINE because of the casting by coerce_to_boolean(). i.e.:
>
> - tle in the target list has no cast node
> - tle in the DEFINE may have cast node
>
> I don't know how this could affect subsequent RPR process but I would
> like to keep the prerequisites that tle in DEINE and the traget list
> is identical.
>

> is identical.

That is a very good point regarding tle identity.

On the implementation side, the DEFINE clause TargetEntries are
already deep-copied via copyObject in the parser (parse_rpr.c),
so adding a cast node to the DEFINE tle does not affect the
target list tle -- they are independent copies at that point.

On the semantic side, the two serve different purposes: the target
list produces output columns (where the user expects the original
type), while the DEFINE expression is evaluated as a boolean for
pattern matching. This is the same pattern as WHERE, where the
WHERE expression may carry a cast node but the target list for the
same column does not.

That said, if you would prefer to keep them structurally identical,
an alternative would be to reject non-boolean types in DEFINE with
a clear error (using exprType() check). However, this would be
stricter than WHERE/HAVING behavior, which might surprise users.

I would suggest that allowing coercion is the right choice for
consistency, but I am happy to go with whichever approach you
prefer.


On a separate topic, I have found a server crash bug in the current
PREV/NEXT implementation. Nesting navigation functions causes the
backend to crash:

    CREATE TABLE t (id int, price int);
    INSERT INTO t VALUES (1,10),(2,20),(3,15),(4,25),(5,30);

    SELECT * FROM t WINDOW w AS (
        ORDER BY id
        ROWS BETWEEN CURRENT ROW AND UNBOUNDED FOLLOWING
        PATTERN (A B+)
        DEFINE B AS prev(prev(b.price)) > 0
    );
    -- server process crashes

The SQL standard (ISO/IEC 19075-5, Section 5.6.2) prohibits nesting
PREV/NEXT within PREV/NEXT. The first argument of PREV/NEXT must
contain a column reference, not another navigation function call.
Currently there is no parser-level check for this, so the nested
call passes through to the executor and crashes.

I have attached a test file (rpr_nav_nesting_test.txt) containing
6 test cases that cover the prohibited nesting patterns:

  - PREV(PREV(price))        direct same-kind nesting
  - NEXT(NEXT(price))        direct same-kind nesting
  - PREV(NEXT(price))        cross nesting
  - NEXT(PREV(price))        cross nesting
  - PREV(ABS(PREV(price)))   nesting hidden inside another function
  - NEXT(ABS(NEXT(price)))   nesting hidden inside another function

All of these currently crash the server. After a fix, they should
all produce a syntax error.


Finally, while reviewing the standard and the current
PREV/NEXT implementation, I have some thoughts on the
design.

The current approach uses all three ExprContext slots
(scan/outer/inner) with varno rewriting, which fixes offsets at
+/-1. The standard requires variable offsets like PREV(price, 3),
and the three-slot model cannot support this.

I think a simpler approach would be to use just one slot
(ecxt_outertuple) and swap it during expression evaluation:

  For `price > PREV(price, 2)`:
        ^^^^    ^^^^  ^^^^^
         |       |      |
         v       v      v
    1. [price]       read from current slot   --> datum_a
    2. [PREV( ,2)]   swap slot to (currentpos - 2)
    3.   [price]     read from swapped slot   --> datum_b
    4. [)]           restore slot
    5. [>]           evaluate datum_a > datum_b

Since step 1 already extracts the value as a Datum before the
swap in step 2, the result is safe. No varno rewriting is needed
-- all column reads just use the one slot, and the swap/restore
controls which row they see.

One thing I noticed is that the SQL standard explicitly requires
all column references inside a navigation function to be qualified
by the same row pattern variable (Section 5.2):

  "All row pattern column references in an aggregate or row
   pattern navigation operation are qualified by the same row
   pattern variable."

This means the navigation argument is always evaluated in a single
row context -- which is exactly what makes the one-slot swap model
work. It seems like the standard may have been designed with this
kind of implementation in mind.

With this approach, the existing varno rewriting infrastructure and
the extra slots for PREV/NEXT would no longer be needed, which
should simplify the code considerably.

I also believe this design would scale well to future extensions.
Variable offsets (PREV(price, N)) would work naturally, and when
we add FIRST/LAST navigation or row pattern variable qualifiers,
only the target row calculation would change -- the swap/restore
mechanism itself would remain the same.

What do you think about this approach?

Best regards,
Henson

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

<div dir=3D"ltr"><div dir=3D"ltr"><div dir=3D"ltr">Hi Tatsuo,<br><br>Thank =
you for your thoughtful feedback on the tle identity concern.</div><div dir=
=3D"ltr"><br></div><div class=3D"gmail_quote"><blockquote class=3D"gmail_qu=
ote" style=3D"margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-st=
yle:solid;border-left-color:rgb(204,204,204);padding-left:1ex">
Ok. we should be consistent with WHERE etc. unless the standard<br>
requres DEFINE stricter. But if the tle is in the target list<br>
(possibly with resjunk attribute), it could be different from the one<br>
in the DEFINE because of the casting by coerce_to_boolean(). i.e.:<br>
<br>
- tle in the target list has no cast node<br>
- tle in the DEFINE may have cast node<br>
<br>
I don&#39;t know how this could affect subsequent RPR process but I would<b=
r>
like to keep the prerequisites that tle in DEINE and the traget list<br>
is identical.<br></blockquote><div><br></div>&gt; is identical.<br><br>That=
 is a very good point regarding tle identity.<br><br>On the implementation =
side, the DEFINE clause TargetEntries are<br>already deep-copied via copyOb=
ject in the parser (parse_rpr.c),<br>so adding a cast node to the DEFINE tl=
e does not affect the<br>target list tle -- they are independent copies at =
that point.<br><br>On the semantic side, the two serve different purposes: =
the target<br>list produces output columns (where the user expects the orig=
inal<br>type), while the DEFINE expression is evaluated as a boolean for<br=
>pattern matching. This is the same pattern as WHERE, where the<br>WHERE ex=
pression may carry a cast node but the target list for the<br>same column d=
oes not.<br><br>That said, if you would prefer to keep them structurally id=
entical,<br>an alternative would be to reject non-boolean types in DEFINE w=
ith<br>a clear error (using exprType() check). However, this would be<br>st=
ricter than WHERE/HAVING behavior, which might surprise users.<br><br>I wou=
ld suggest that allowing coercion is the right choice for<br>consistency, b=
ut I am happy to go with whichever approach you<br>prefer.<br><br><br>On a =
separate topic, I have found a server crash bug in the current<br>PREV/NEXT=
 implementation. Nesting navigation functions causes the<br>backend to cras=
h:<br><br>=C2=A0 =C2=A0 CREATE TABLE t (id int, price int);<br>=C2=A0 =C2=
=A0 INSERT INTO t VALUES (1,10),(2,20),(3,15),(4,25),(5,30);<br><br>=C2=A0 =
=C2=A0 SELECT * FROM t WINDOW w AS (<br>=C2=A0 =C2=A0 =C2=A0 =C2=A0 ORDER B=
Y id<br>=C2=A0 =C2=A0 =C2=A0 =C2=A0 ROWS BETWEEN CURRENT ROW AND UNBOUNDED =
FOLLOWING<br>=C2=A0 =C2=A0 =C2=A0 =C2=A0 PATTERN (A B+)<br>=C2=A0 =C2=A0 =
=C2=A0 =C2=A0 DEFINE B AS prev(prev(b.price)) &gt; 0<br>=C2=A0 =C2=A0 );<br=
>=C2=A0 =C2=A0 -- server process crashes<br><br>The SQL standard (ISO/IEC 1=
9075-5, Section 5.6.2) prohibits nesting<br>PREV/NEXT within PREV/NEXT. The=
 first argument of PREV/NEXT must<br>contain a column reference, not anothe=
r navigation function call.<br>Currently there is no parser-level check for=
 this, so the nested<br>call passes through to the executor and crashes.<br=
><br>I have attached a test file (rpr_nav_nesting_test.txt) containing<br>6=
 test cases that cover the prohibited nesting patterns:<br><br>=C2=A0 - PRE=
V(PREV(price)) =C2=A0 =C2=A0 =C2=A0 =C2=A0direct same-kind nesting<br>=C2=
=A0 - NEXT(NEXT(price)) =C2=A0 =C2=A0 =C2=A0 =C2=A0direct same-kind nesting=
<br>=C2=A0 - PREV(NEXT(price)) =C2=A0 =C2=A0 =C2=A0 =C2=A0cross nesting<br>=
=C2=A0 - NEXT(PREV(price)) =C2=A0 =C2=A0 =C2=A0 =C2=A0cross nesting<br>=C2=
=A0 - PREV(ABS(PREV(price))) =C2=A0 nesting hidden inside another function<=
br>=C2=A0 - NEXT(ABS(NEXT(price))) =C2=A0 nesting hidden inside another fun=
ction<br><br>All of these currently crash the server. After a fix, they sho=
uld<br>all produce a syntax error.<br><br><br>Finally, while reviewing the =
standard and the current<br>PREV/NEXT implementation, I have some thoughts =
on the<br>design.<br><br>The current approach uses all three ExprContext sl=
ots<br>(scan/outer/inner) with varno rewriting, which fixes offsets at<br>+=
/-1. The standard requires variable offsets like PREV(price, 3),<br>and the=
 three-slot model cannot support this.<br><br>I think a simpler approach wo=
uld be to use just one slot<br>(ecxt_outertuple) and swap it during express=
ion evaluation:<br><br><font face=3D"monospace">=C2=A0 For `price &gt; PREV=
(price, 2)`:<br>=C2=A0 =C2=A0 =C2=A0 =C2=A0 ^^^^ =C2=A0 =C2=A0^^^^ =C2=A0^^=
^^^<br>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0| =C2=A0 =C2=A0 =C2=A0 | =C2=A0 =
=C2=A0 =C2=A0|<br>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0v =C2=A0 =C2=A0 =C2=A0 =
v =C2=A0 =C2=A0 =C2=A0v<br>=C2=A0 =C2=A0 1. [price] =C2=A0 =C2=A0 =C2=A0 re=
ad from current slot =C2=A0 --&gt; datum_a<br>=C2=A0 =C2=A0 2. [PREV( ,2)] =
=C2=A0 swap slot to (currentpos - 2)<br>=C2=A0 =C2=A0 3. =C2=A0 [price] =C2=
=A0 =C2=A0 read from swapped slot =C2=A0 --&gt; datum_b<br>=C2=A0 =C2=A0 4.=
 [)] =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 restore slot<br>=C2=A0 =C2=A0 5. [&=
gt;] =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 evaluate datum_a &gt; datum_b</font=
><br><br>Since step 1 already extracts the value as a Datum before the<br>s=
wap in step 2, the result is safe. No varno rewriting is needed<br>-- all c=
olumn reads just use the one slot, and the swap/restore<br>controls which r=
ow they see.<br><br>One thing I noticed is that the SQL standard explicitly=
 requires<br>all column references inside a navigation function to be quali=
fied<br>by the same row pattern variable (Section 5.2):<br><br>=C2=A0 &quot=
;All row pattern column references in an aggregate or row<br>=C2=A0 =C2=A0p=
attern navigation operation are qualified by the same row<br>=C2=A0 =C2=A0p=
attern variable.&quot;<br><br>This means the navigation argument is always =
evaluated in a single<br>row context -- which is exactly what makes the one=
-slot swap model<br>work. It seems like the standard may have been designed=
 with this<br>kind of implementation in mind.<br><br>With this approach, th=
e existing varno rewriting infrastructure and<br>the extra slots for PREV/N=
EXT would no longer be needed, which<br>should simplify the code considerab=
ly.<br><br>I also believe this design would scale well to future extensions=
.<br>Variable offsets (PREV(price, N)) would work naturally, and when<br>we=
 add FIRST/LAST navigation or row pattern variable qualifiers,<br>only the =
target row calculation would change -- the swap/restore<br>mechanism itself=
 would remain the same.<br><br>What do you think about this approach?<br><b=
r>Best regards,<br>Henson=C2=A0</div></div>
</div>

--0000000000007cb47f064cdd4d22--

--0000000000007cb481064cdd4d24
Content-Type: text/plain; charset="US-ASCII"; name="rpr_nav_nesting_test.txt"
Content-Disposition: attachment; filename="rpr_nav_nesting_test.txt"
Content-Transfer-Encoding: base64
Content-ID: <f_mmo6mi370>
X-Attachment-Id: f_mmo6mi370

LS0gPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09
PT09PT09Ci0tIFJQUiBOYXZpZ2F0aW9uIE5lc3RpbmcgVGVzdHMKLS0gVGVzdHMgZm9yIHByb2hp
Yml0ZWQgbmVzdGluZyBvZiBQUkVWL05FWFQgKFNRTCBzdGFuZGFyZCA1LjYpCi0tID09PT09PT09
PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLQot
LSBUaGUgU1FMIHN0YW5kYXJkIHByb2hpYml0czoKLS0gICAtIFBSRVYvTkVYVCBuZXN0ZWQgd2l0
aGluIFBSRVYvTkVYVAotLSAgIC0gUFJFVi9ORVhUIG5lc3RlZCB3aXRoaW4gb3RoZXIgZnVuY3Rp
b25zIGluc2lkZSBQUkVWL05FWFQKLS0KLS0gVGhlc2Ugc2hvdWxkIGFsbCBwcm9kdWNlIHN5bnRh
eCBlcnJvcnMsIG5vdCBzZXJ2ZXIgY3Jhc2hlcy4KLS0gPT09PT09PT09PT09PT09PT09PT09PT09
PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09CgpDUkVBVEUgVEVNUCBUQUJMRSBu
YXZfdGVzdCAoaWQgSU5ULCBwcmljZSBJTlQpOwpJTlNFUlQgSU5UTyBuYXZfdGVzdCBWQUxVRVMg
KDEsIDEwKSwgKDIsIDIwKSwgKDMsIDE1KSwgKDQsIDI1KSwgKDUsIDMwKTsKCi0tIFBSRVYoUFJF
VihwcmljZSkpIC0gZGlyZWN0IG5lc3RpbmcKU0VMRUNUICogRlJPTSBuYXZfdGVzdCBXSU5ET1cg
dyBBUyAoCiAgICBPUkRFUiBCWSBpZAogICAgUk9XUyBCRVRXRUVOIENVUlJFTlQgUk9XIEFORCBV
TkJPVU5ERUQgRk9MTE9XSU5HCiAgICBQQVRURVJOIChBIEIrKQogICAgREVGSU5FIEIgQVMgcHJl
dihwcmV2KGIucHJpY2UpKSA+IDAKKTsKCi0tIE5FWFQoTkVYVChwcmljZSkpIC0gZGlyZWN0IG5l
c3RpbmcKU0VMRUNUICogRlJPTSBuYXZfdGVzdCBXSU5ET1cgdyBBUyAoCiAgICBPUkRFUiBCWSBp
ZAogICAgUk9XUyBCRVRXRUVOIENVUlJFTlQgUk9XIEFORCBVTkJPVU5ERUQgRk9MTE9XSU5HCiAg
ICBQQVRURVJOIChBIEIrKQogICAgREVGSU5FIEIgQVMgbmV4dChuZXh0KGIucHJpY2UpKSA+IDAK
KTsKCi0tIFBSRVYoTkVYVChwcmljZSkpIC0gY3Jvc3MgbmVzdGluZwpTRUxFQ1QgKiBGUk9NIG5h
dl90ZXN0IFdJTkRPVyB3IEFTICgKICAgIE9SREVSIEJZIGlkCiAgICBST1dTIEJFVFdFRU4gQ1VS
UkVOVCBST1cgQU5EIFVOQk9VTkRFRCBGT0xMT1dJTkcKICAgIFBBVFRFUk4gKEEgQispCiAgICBE
RUZJTkUgQiBBUyBwcmV2KG5leHQoYi5wcmljZSkpID4gMAopOwoKLS0gTkVYVChQUkVWKHByaWNl
KSkgLSBjcm9zcyBuZXN0aW5nClNFTEVDVCAqIEZST00gbmF2X3Rlc3QgV0lORE9XIHcgQVMgKAog
ICAgT1JERVIgQlkgaWQKICAgIFJPV1MgQkVUV0VFTiBDVVJSRU5UIFJPVyBBTkQgVU5CT1VOREVE
IEZPTExPV0lORwogICAgUEFUVEVSTiAoQSBCKykKICAgIERFRklORSBCIEFTIG5leHQocHJldihi
LnByaWNlKSkgPiAwCik7CgotLSBQUkVWIGluc2lkZSBleHByZXNzaW9uIGluc2lkZSBQUkVWClNF
TEVDVCAqIEZST00gbmF2X3Rlc3QgV0lORE9XIHcgQVMgKAogICAgT1JERVIgQlkgaWQKICAgIFJP
V1MgQkVUV0VFTiBDVVJSRU5UIFJPVyBBTkQgVU5CT1VOREVEIEZPTExPV0lORwogICAgUEFUVEVS
TiAoQSBCKykKICAgIERFRklORSBCIEFTIHByZXYoYWJzKHByZXYoYi5wcmljZSkpKSA+IDAKKTsK
Ci0tIE5FWFQgaW5zaWRlIGV4cHJlc3Npb24gaW5zaWRlIE5FWFQKU0VMRUNUICogRlJPTSBuYXZf
dGVzdCBXSU5ET1cgdyBBUyAoCiAgICBPUkRFUiBCWSBpZAogICAgUk9XUyBCRVRXRUVOIENVUlJF
TlQgUk9XIEFORCBVTkJPVU5ERUQgRk9MTE9XSU5HCiAgICBQQVRURVJOIChBIEIrKQogICAgREVG
SU5FIEIgQVMgbmV4dChhYnMobmV4dChiLnByaWNlKSkpID4gMAopOwoKRFJPUCBUQUJMRSBuYXZf
dGVzdDsK
--0000000000007cb481064cdd4d24--