MIME-Version: 1.0
References: <20260410.183515.1561517749674643173.ishii@postgresql.org>
 <CAAAe_zB7rAEJtT6hXgF85=_Tj8Nti45ZHbQw26gxTF2DBs3hJw@mail.gmail.com>
 <20260417.171022.397021509290810631.ishii@postgresql.org>
 <20260417.191303.359804214243525887.ishii@postgresql.org>
In-Reply-To: <20260417.191303.359804214243525887.ishii@postgresql.org>
Reply-To: assam258@gmail.com
From: Henson Choi <assam258@gmail.com>
Date: Fri, 17 Apr 2026 21:08:12 +0900
Message-ID: 
 <CAAAe_zAkzx7bq-nM8YVg0ukzOnWoB-dNhfjDnRcvoOEFbnV+Vg@mail.gmail.com>
Subject: Re: Row pattern recognition
To: Tatsuo Ishii <ishii@postgresql.org>
Cc: 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, pgsql-hackers@postgresql.org
Content-Type: multipart/alternative; boundary="0000000000005e9b98064fa6ce76"
Archived-At: 
 <https://www.postgresql.org/message-id/CAAAe_zAkzx7bq-nM8YVg0ukzOnWoB-dNhfjDnRcvoOEFbnV%2BVg%40mail.gmail.com>
Precedence: bulk

--0000000000005e9b98064fa6ce76
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hi Tatsuo,

Thank you for the feedback.

Note that applying 0001-0006 produced compiler warning.
>
> execExprInterp.c: In function =E2=80=98ExecEvalRPRNavSet=E2=80=99:
> execExprInterp.c:6005:23: warning: =E2=80=98target_pos=E2=80=99 may be us=
ed uninitialized
> [-Wmaybe-uninitialized]
>  6005 |         target_slot =3D ExecRPRNavGetSlot(winstate, target_pos);
>       |                       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> execExprInterp.c:5959:25: note: =E2=80=98target_pos=E2=80=99 was declared=
 here
>  5959 |         int64           target_pos;
>       |                         ^~~~~~~~~~
>

`ExecRPRNavGetSlot()`, along with the enclosing `ExecEvalRPRNavSet()`
and its `int64 target_pos` declaration, is introduced by nocfbot-0017
(Implement 1-slot PREV/NEXT navigation for RPR), not at the 0001-0006
level.  Could you double-check the actual applied range on your side?
The function simply does not exist at 0001-0006.

Since the function is not present at 0001-0006, I suspect the
applied patch set on your side is in a different state than
expected.  Could you verify the range and contents of the patches
actually applied?

But regression test was ok.
> However, after applying 0007, regression test failed.
>
> not ok 196   + rpr_integration                           514 ms
> # (test process exited with exit code 2)
>

nocfbot-0006 was written specifically to prevent the crashes that
0007's tests expose, so with 0001-0007 applied in order the issue
should already be resolved.  On my authoritative base (cfbot/4460) all
rpr_integration tests pass through 0008.  I had been routinely
building only on macOS ARM64 recently, so I just re-verified on
Rocky 9 and Ubuntu 24 AMD64 as well; no build warning and no
rpr_integration failure on either.

For reference, here is my working tree layout.  Titles only for my
post-base commits (hashes on my side won't match yours); base
hashes are public and directly comparable on your side:

  [post-base, newest first]
   31. Remove unused include and fix header ordering in RPR files
(origin/RPR)
   30. Add inline comments for complex RPR algorithms and design notes
   29. Fix nav_slot pass-by-ref dangling pointer in RPR navigation
   28. Fix RPR documentation: synopsis, grammar, and terminology
   27. Fix comment typos, grammar, and inaccuracies in RPR code
   26. Fix RPR error message style: hint format, terminology, capitalizatio=
n
   25. Guard against int64 overflow in RPR bounded frame end computation
   24. Implement FIRST/LAST and compound navigation for RPR
   23. Add Nav Mark Lookback to EXPLAIN and fix compute_nav_max_offset()
   22. Add 2-arg PREV/NEXT test for row pattern navigation with host
variable
   21. Enable JIT compilation for PREV/NEXT navigation tests in RPR
   20. Update RPR code comments to reflect 1-slot navigation model
   19. Add tuplestore trim optimization for RPR PREV navigation
   18. Add JIT compilation support for RPR PREV/NEXT navigation
   17. Implement 1-slot PREV/NEXT navigation for RPR
   16. Normalize RPR element flag macros to return bool
   15. Narrow variable scope in ExecInitWindowAgg DEFINE clause loop
   14. Add CHECK_FOR_INTERRUPTS to RPR context cleanup and finalize loops
   13. Remove unused force_colno parameter from RPR deparse functions
   12. Fix execRPR.o ordering in executor Makefile to match meson.build
   11. Fix quote_identifier() for RPR pattern variable name deparse
   10. Rename rpr_explain test views from sequential numbers to descriptive
names
    9. Add fixed-length group absorption for RPR
    8. Replace reduced frame map with single match result
    7. Add RPR planner integration tests
    6. Fix DEFINE expression handling in RPR window planning
    5. Fix mark handling for last_value() under RPR
    4. Fix in-place modification of defineClause TargetEntry in setrefs.c
    3. Add CHECK_FOR_INTERRUPTS() to nfa_try_absorb_context() loop
    2. Add CHECK_FOR_INTERRUPTS() to nfa_add_state_unique() for state
explosion patterns
    1. Remove unused regex/regex.h include from nodeWindowAgg.c

  [base, public hashes]
  732acf9b7c6 (cfbot/cf/4460) [CF 4460] v46 - Implement row pattern
recognition feature
  6eb8841afaf Row pattern recognition patch (typedefs.list).
  5213dbb3c4f Row pattern recognition patch (tests: expected).
  bdc3a17982b Row pattern recognition patch (tests: SQL, data).
  596bcc434b5 Row pattern recognition patch (docs).
  e02872fd59c Row pattern recognition patch (executor and commands).
  742c6eb673e Row pattern recognition patch (planner).
  8514ca64f2d Row pattern recognition patch (rewriter).
  d26741d4f1d Row pattern recognition patch (parse/analysis).
  f85d2ee8f4a Row pattern recognition patch for raw parser.
  e484b0eea61 (origin/RPR-base, RPR-base) Fix two issues in fast-path FK
check introduced by commit 2da86c1ef9

Could you send me the equivalent `git log` for your working tree
covering the full v46 application and the commit immediately
preceding it?  In particular, could you also check whether the
recent master commit titled "Fix integer overflow in
nodeWindowAgg.c" (backpatched to 14) is included in your base?  It
touches the same ROWS/GROUPS frame boundary code path that RPR
interacts with, so its presence or absence will help narrow things
down.  Once I can match the same base locally, I will reproduce and
include the fix in the next revision.

A quick status update:

  - nocfbot-0006: self-review complete, not yet sent (comment tweak per
review)
  - nocfbot-0007: under self-review (comment details per review)
  - nocfbot-0008: in progress

I would like to resolve the crash first before sending the next
revision, so that the full set goes out together once our bases are
aligned and A3 passes on your side.

One process question: so far I have been folding review feedback
into the relevant existing patches (i.e. amending nocfbot-0008 to
reflect your comments on it).  Would you prefer that I instead
append the review fixes as new patches at the end of the series
(e.g. nocfbot-0032, 0033, ...), so the history of reviewed changes
stays visible?  Happy to follow whichever workflow you find easier
to review.

Regards,
Henson

--0000000000005e9b98064fa6ce76
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 dir=3D"ltr">Thank you for the feedback.<br></div><div class=3D"gmail=
_quote gmail_quote_container"><div dir=3D"ltr" class=3D"gmail_attr"><br></d=
iv><blockquote class=3D"gmail_quote" style=3D"margin:0px 0px 0px 0.8ex;bord=
er-left:1px solid rgb(204,204,204);padding-left:1ex">Note that applying 000=
1-0006 produced compiler warning.<br>
<br>
execExprInterp.c: In function =E2=80=98ExecEvalRPRNavSet=E2=80=99:<br>
execExprInterp.c:6005:23: warning: =E2=80=98target_pos=E2=80=99 may be used=
 uninitialized [-Wmaybe-uninitialized]<br>
=C2=A06005 |=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0target_slot =3D ExecRPRNavGet=
Slot(winstate, target_pos);<br>
=C2=A0 =C2=A0 =C2=A0 |=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~<br>
execExprInterp.c:5959:25: note: =E2=80=98target_pos=E2=80=99 was declared h=
ere<br>
=C2=A05959 |=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0int64=C2=A0 =C2=A0 =C2=A0 =C2=
=A0 =C2=A0 =C2=A0target_pos;<br>
=C2=A0 =C2=A0 =C2=A0 |=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0^~~~~~~~~~<br></blockquote><div>=C2=
=A0</div><div>`ExecRPRNavGetSlot()`, along with the enclosing `ExecEvalRPRN=
avSet()`<br>and its `int64 target_pos` declaration, is introduced by nocfbo=
t-0017<br>(Implement 1-slot PREV/NEXT navigation for RPR), not at the 0001-=
0006<br>level.=C2=A0 Could you double-check the actual applied range on you=
r side?<br>The function simply does not exist at 0001-0006.<br><br>Since th=
e function is not present at 0001-0006, I suspect the<br>applied patch set =
on your side is in a different state than<br>expected.=C2=A0 Could you veri=
fy the range and contents of the patches<br>actually applied?<br></div><div=
><br></div><blockquote class=3D"gmail_quote" style=3D"margin:0px 0px 0px 0.=
8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
But regression test was ok.<br>
However, after applying 0007, regression test failed.<br>
<br>
not ok 196=C2=A0 =C2=A0+ rpr_integration=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0514 ms<br>
# (test process exited with exit code 2)<br></blockquote><div><br>nocfbot-0=
006 was written specifically to prevent the crashes that<br>0007&#39;s test=
s expose, so with 0001-0007 applied in order the issue<br>should already be=
 resolved.=C2=A0 On my authoritative base (cfbot/4460) all<br>rpr_integrati=
on tests pass through 0008.=C2=A0 I had been routinely<br>building only on =
macOS ARM64 recently, so I just re-verified on<br>Rocky 9 and Ubuntu 24 AMD=
64 as well; no build warning and no<br>rpr_integration failure on either.<b=
r><br>For reference, here is my working tree layout.=C2=A0 Titles only for =
my<br>post-base commits (hashes on my side won&#39;t match yours); base<br>=
hashes are public and directly comparable on your side:<br><br>=C2=A0 [post=
-base, newest first]<br>=C2=A0 =C2=A031. Remove unused include and fix head=
er ordering in RPR files =C2=A0 (origin/RPR)<br>=C2=A0 =C2=A030. Add inline=
 comments for complex RPR algorithms and design notes<br>=C2=A0 =C2=A029. F=
ix nav_slot pass-by-ref dangling pointer in RPR navigation<br>=C2=A0 =C2=A0=
28. Fix RPR documentation: synopsis, grammar, and terminology<br>=C2=A0 =C2=
=A027. Fix comment typos, grammar, and inaccuracies in RPR code<br>=C2=A0 =
=C2=A026. Fix RPR error message style: hint format, terminology, capitaliza=
tion<br>=C2=A0 =C2=A025. Guard against int64 overflow in RPR bounded frame =
end computation<br>=C2=A0 =C2=A024. Implement FIRST/LAST and compound navig=
ation for RPR<br>=C2=A0 =C2=A023. Add Nav Mark Lookback to EXPLAIN and fix =
compute_nav_max_offset()<br>=C2=A0 =C2=A022. Add 2-arg PREV/NEXT test for r=
ow pattern navigation with host variable<br>=C2=A0 =C2=A021. Enable JIT com=
pilation for PREV/NEXT navigation tests in RPR<br>=C2=A0 =C2=A020. Update R=
PR code comments to reflect 1-slot navigation model<br>=C2=A0 =C2=A019. Add=
 tuplestore trim optimization for RPR PREV navigation<br>=C2=A0 =C2=A018. A=
dd JIT compilation support for RPR PREV/NEXT navigation<br>=C2=A0 =C2=A017.=
 Implement 1-slot PREV/NEXT navigation for RPR<br>=C2=A0 =C2=A016. Normaliz=
e RPR element flag macros to return bool<br>=C2=A0 =C2=A015. Narrow variabl=
e scope in ExecInitWindowAgg DEFINE clause loop<br>=C2=A0 =C2=A014. Add CHE=
CK_FOR_INTERRUPTS to RPR context cleanup and finalize loops<br>=C2=A0 =C2=
=A013. Remove unused force_colno parameter from RPR deparse functions<br>=
=C2=A0 =C2=A012. Fix execRPR.o ordering in executor Makefile to match meson=
.build<br>=C2=A0 =C2=A011. Fix quote_identifier() for RPR pattern variable =
name deparse<br>=C2=A0 =C2=A010. Rename rpr_explain test views from sequent=
ial numbers to descriptive names<br>=C2=A0 =C2=A0 9. Add fixed-length group=
 absorption for RPR<br>=C2=A0 =C2=A0 8. Replace reduced frame map with sing=
le match result<br>=C2=A0 =C2=A0 7. Add RPR planner integration tests<br>=
=C2=A0 =C2=A0 6. Fix DEFINE expression handling in RPR window planning<br>=
=C2=A0 =C2=A0 5. Fix mark handling for last_value() under RPR<br>=C2=A0 =C2=
=A0 4. Fix in-place modification of defineClause TargetEntry in setrefs.c<b=
r>=C2=A0 =C2=A0 3. Add CHECK_FOR_INTERRUPTS() to nfa_try_absorb_context() l=
oop<br>=C2=A0 =C2=A0 2. Add CHECK_FOR_INTERRUPTS() to nfa_add_state_unique(=
) for state explosion patterns<br>=C2=A0 =C2=A0 1. Remove unused regex/rege=
x.h include from nodeWindowAgg.c<br><br>=C2=A0 [base, public hashes]<br>=C2=
=A0 732acf9b7c6 (cfbot/cf/4460) [CF 4460] v46 - Implement row pattern recog=
nition feature<br>=C2=A0 6eb8841afaf Row pattern recognition patch (typedef=
s.list).<br>=C2=A0 5213dbb3c4f Row pattern recognition patch (tests: expect=
ed).<br>=C2=A0 bdc3a17982b Row pattern recognition patch (tests: SQL, data)=
.<br>=C2=A0 596bcc434b5 Row pattern recognition patch (docs).<br>=C2=A0 e02=
872fd59c Row pattern recognition patch (executor and commands).<br>=C2=A0 7=
42c6eb673e Row pattern recognition patch (planner).<br>=C2=A0 8514ca64f2d R=
ow pattern recognition patch (rewriter).<br>=C2=A0 d26741d4f1d Row pattern =
recognition patch (parse/analysis).<br>=C2=A0 f85d2ee8f4a Row pattern recog=
nition patch for raw parser.<br>=C2=A0 e484b0eea61 (origin/RPR-base, RPR-ba=
se) Fix two issues in fast-path FK check introduced by commit 2da86c1ef9<br=
><br>Could you send me the equivalent `git log` for your working tree<br>co=
vering the full v46 application and the commit immediately<br>preceding it?=
=C2=A0 In particular, could you also check whether the<br>recent master com=
mit titled &quot;Fix integer overflow in<br>nodeWindowAgg.c&quot; (backpatc=
hed to 14) is included in your base?=C2=A0 It<br>touches the same ROWS/GROU=
PS frame boundary code path that RPR<br>interacts with, so its presence or =
absence will help narrow things<br>down.=C2=A0 Once I can match the same ba=
se locally, I will reproduce and<br>include the fix in the next revision.<b=
r><br>A quick status update:<br><br>=C2=A0 - nocfbot-0006: self-review comp=
lete, not yet sent (comment tweak per review)<br>=C2=A0 - nocfbot-0007: und=
er self-review (comment details per review)<br>=C2=A0 - nocfbot-0008: in pr=
ogress<br><br>I would like to resolve the crash first before sending the ne=
xt<br>revision, so that the full set goes out together once our bases are<b=
r>aligned and A3 passes on your side.<br><br>One process question: so far I=
 have been folding review feedback<br>into the relevant existing patches (i=
.e. amending nocfbot-0008 to<br>reflect your comments on it).=C2=A0 Would y=
ou prefer that I instead<br>append the review fixes as new patches at the e=
nd of the series<br>(e.g. nocfbot-0032, 0033, ...), so the history of revie=
wed changes<br>stays visible?=C2=A0 Happy to follow whichever workflow you =
find easier<br>to review.<br><br>Regards,<br>Henson<br></div></div></div>

--0000000000005e9b98064fa6ce76--