MIME-Version: 1.0
References: <20260502.140304.670813149418899420.ishii@postgresql.org>
 <20260505.090124.365339750969814137.ishii@postgresql.org>
 <CACJufxEWL_ZnC-bs_yrg-Ys6ZUD3Ut_p1Ebj0bAcbzj67+HDAQ@mail.gmail.com>
 <20260517.190023.159085681032648582.ishii@postgresql.org>
 <CACJufxH-DZePhbdJM=8nNYceQiSbbXXLTw54iLhxiynQ+4hbBA@mail.gmail.com>
 <CAAAe_zDephfiDA_A3FN0hCymJRogEr=Rt3QoCTf4qMYDLk+xNA@mail.gmail.com>
 <CACJufxGX17thWuEOq1tM5xbRHz2HXm1asooZC3GV25MYGmYqLQ@mail.gmail.com>
In-Reply-To: 
 <CACJufxGX17thWuEOq1tM5xbRHz2HXm1asooZC3GV25MYGmYqLQ@mail.gmail.com>
Reply-To: assam258@gmail.com
From: Henson Choi <assam258@gmail.com>
Date: Fri, 29 May 2026 10:28:29 +0900
Message-ID: 
 <CAAAe_zAH0MvP0TBmW3PTLeHjEpiyBz0473zJRM9pwLpseefMNw@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="000000000000cfb4fd0652eac355"
Archived-At: 
 <https://www.postgresql.org/message-id/CAAAe_zAH0MvP0TBmW3PTLeHjEpiyBz0473zJRM9pwLpseefMNw%40mail.gmail.com>
Precedence: bulk

--000000000000cfb4fd0652eac355
Content-Type: text/plain; charset="UTF-8"

Hi Jian,

Thanks for the careful read of execRPR.c and the NFA -- several of these
landed on real gaps in our comments/docs. Going through your points
inline below, with a summary of decisions at the end.


> In src/include/nodes/execnodes.h, we're adding quite a few fields to
> WindowAggState that are only used for RPR queries.
> Should we consolidate these fields behind a single pointer (named
> RPRContext) to keep the WindowAggState size smaller for non-RPR
> queries?

The size win is real -- it's roughly 450-500 bytes per WindowAggState
that every non-RPR window query carries today. But it takes a wide
code change to get there: it reshapes every RPR access path, a
sizable (if mostly mechanical) diff across nodeWindowAgg.c,
execRPR.c, and the explain side.

Tatsuo, as co-author -- do you want this in v48? If you do, I'll
prepare the RPRContext consolidation as an incremental patch for you to
fold into v48. It isn't blocking either way.


> In function get_reduced_frame_status, I made the following changes:
> [conditionA..D probe elided]
> I re-ran the regression tests and noticed it's entirely possible for
> multiple conditions to be true at once. Because of this, all the IF
> blocks in get_reduced_frame_status (after the initial one) are
> order-dependent. If I move one IF statement above another,
> get_reduced_frame_status may return a different result.
> Is this the expected behavior? It seems like a potential logic flaw.

You're right that the conditions aren't mutually exclusive and that
reordering the branches changes the result -- but that's the
cascade-with-early-return idiom, standard in C and used throughout the
backend, not a logic flaw. Each branch is the minimal test given the
negations the earlier returns have already established.

heapam_visibility.c is the canonical example: the HeapTupleSatisfies*
family cascades through ordered early returns, with the later XMAX
branches correct only because the XMIN-committed invariant above holds.

The order is correct, not just idiomatic: update_reduced_frame() only
ever records (matched, length) as (false, 1), (true, 0), or (true, >=1),
and A->B->C->D is the order that classifies exactly those three cases --
reorder it and one of them gets misclassified.

I'll add short "by here, ..." comments between the branches to make the
invariants visible, but keep the structure. If you feel strongly it
should be restructured, I'd defer to Tatsuo on that.


> "boundary chk" should be "boundary check".
> column "match_start dep.", I don't understand the meaning of "direct"
> and "boundary check".
> "count-dominance" seems like an invented word, but I could not find
> the explanation.
> "match_start-dependent" occurred many times, it would also deserve an
> explanation. We can also rename it as match_start_dependent.

All fair. v48 fixes the typo and adds a terminology block to README.rpr
defining count-dominance (the VIII-3 cover condition, named there),
match_start_dependent (renamed to the underscore spelling, to match the
defineMatchStartDependent identifier), and the "direct" vs "boundary
check" navigation distinction.


> Attached is a minor refactoring of ExecRPRProcessRow, no need for
> boolean hasLimitedFrame.
> ``if (hasLimitedFrame && winstate->endOffsetValue != 0)`` seems wrong
> to me, endOffsetValue is a Datum, and you directly compare it with 0.
> see also calculate_frame_offsets.

Good eye on the Datum comparison, and I agree your version reads more
clearly -- gating on the frame-option flag and going through
DatumGetInt64() rather than comparing a Datum to 0, plus the two new
Asserts. (The existing code is correct, since hasLimitedFrame carries
the limited/unbounded distinction separately, but the refactor is the
clearer shape.) I'll take it.

One thing to watch when dropping hasLimitedFrame, though: a zero offset
is still a valid limited frame -- ROWS BETWEEN CURRENT ROW AND CURRENT
ROW, and ... AND 0 FOLLOWING -- so the new check must not treat offset 0
as unbounded, otherwise the boundary check is skipped and a quantified
pattern (A+) silently absorbs the whole partition. The limited/unbounded
distinction lives in the frame-option flag, not the offset value, so the
refactor has to keep that flag in the picture.

We don't currently cover the offset-0 quantified case -- the two
zero-offset frame tests in rpr_base both use a plain PATTERN (A) -- so
I'll add an A+ regression test for it.


> [nfa_evaluate_row's window_gettupleslot returning false]
> indicates that it's not possible for (currentPos > ctxFrameEnd).
> therefore [if (currentPos >= ctxFrameEnd) ...]
> should change to ``if (currentPos == ctxFrameEnd)``.
> With that, the comment wording "exceeded" seems wrong too.

Agreed. `>` is unreachable by the loop invariant -- currentPos advances
by exactly one per row, and once a context is finalized the
states == NULL guard skips it -- so >= and == behave identically here,
and == states the intent better. The `>=` was a defensive guard against
an overshoot that can't actually happen; v48 tightens it to ==, changes
the comment from "exceeded" to "reached", and moves that defense into an
Assert(currentPos <= ctxFrameEnd) so a future change that breaks the
invariant trips immediately instead of silently.


> Since I couldn't generate a coverage report, I am using elog(INFO) to
> test reachability. Rerun the regress tests, you will find out that the
> ELSE IF branch below is not reached by current regress tests.
>     else if (targetCtx->states == NULL)
>     {
>         /* Context already completed - skip to result registration */
>         goto register_result;
>         elog(INFO, "XXXX reached");
>     }

The elog in your snippet sits *after* the goto, so it can never print
even though the branch runs. Moving it above the goto shows it firing
across the suite. Easy mistake; I've put a probe right after a
goto/return more times than I'd like to admit.

The branch is correct as written: under SKIP TO NEXT ROW an overlapping
context can finish (and be preserved by cleanup, since
matchEndRow >= matchStartRow) before the next update_reduced_frame()
call lands on its start row. So no new test is needed -- I'll just add a
comment above it explaining why the head context can already be complete
there.


Summary of decisions, in the order above:


  RPRContext consolidation         Tatsuo's call -- non-blocking for v48
  get_reduced_frame_status order   Keep -- cascade idiom; add comments
  README terminology + typo        Accept -- definitions block + rename
  ExecRPRProcessRow refactor       Accept -- but fix frameOffset
  currentPos >= -> ==              Accept -- plus comment + Assert
  states == NULL branch coverage   Already reached (155x) -- add comment


I'll post an incremental patch shortly with the accepted changes and the
comment additions; the RPRContext question is for Tatsuo.

Thanks again,
Henson

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

<div dir=3D"ltr">Hi Jian,<br><br>Thanks for the careful read of execRPR.c a=
nd the NFA -- several of these<br>landed on real gaps in our comments/docs.=
 Going through your points<br>inline below, with a summary of decisions at =
the end.<br><br><br>&gt; In src/include/nodes/execnodes.h, we&#39;re adding=
 quite a few fields to<br>&gt; WindowAggState that are only used for RPR qu=
eries.<br>&gt; Should we consolidate these fields behind a single pointer (=
named<br>&gt; RPRContext) to keep the WindowAggState size smaller for non-R=
PR<br>&gt; queries?<br><br>The size win is real -- it&#39;s roughly 450-500=
 bytes per WindowAggState<br>that every non-RPR window query carries today.=
 But it takes a wide<br>code change to get there: it reshapes every RPR acc=
ess path, a<br>sizable (if mostly mechanical) diff across nodeWindowAgg.c,<=
br>execRPR.c, and the explain side.<br><br>Tatsuo, as co-author -- do you w=
ant this in v48? If you do, I&#39;ll<br>prepare the RPRContext consolidatio=
n as an incremental patch for you to<br>fold into v48. It isn&#39;t blockin=
g either way.<br><br><br>&gt; In function get_reduced_frame_status, I made =
the following changes:<br>&gt; [conditionA..D probe elided]<br>&gt; I re-ra=
n the regression tests and noticed it&#39;s entirely possible for<br>&gt; m=
ultiple conditions to be true at once. Because of this, all the IF<br>&gt; =
blocks in get_reduced_frame_status (after the initial one) are<br>&gt; orde=
r-dependent. If I move one IF statement above another,<br>&gt; get_reduced_=
frame_status may return a different result.<br>&gt; Is this the expected be=
havior? It seems like a potential logic flaw.<br><br>You&#39;re right that =
the conditions aren&#39;t mutually exclusive and that<br>reordering the bra=
nches changes the result -- but that&#39;s the<br>cascade-with-early-return=
 idiom, standard in C and used throughout the<br>backend, not a logic flaw.=
 Each branch is the minimal test given the<br>negations the earlier returns=
 have already established.<br><br>heapam_visibility.c is the canonical exam=
ple: the HeapTupleSatisfies*<br>family cascades through ordered early retur=
ns, with the later XMAX<br>branches correct only because the XMIN-committed=
 invariant above holds.<br><br>The order is correct, not just idiomatic: up=
date_reduced_frame() only<br>ever records (matched, length) as (false, 1), =
(true, 0), or (true, &gt;=3D1),<br>and A-&gt;B-&gt;C-&gt;D is the order tha=
t classifies exactly those three cases --<br>reorder it and one of them get=
s misclassified.<br><br>I&#39;ll add short &quot;by here, ...&quot; comment=
s between the branches to make the<br>invariants visible, but keep the stru=
cture. If you feel strongly it<br>should be restructured, I&#39;d defer to =
Tatsuo on that.<br><br><br>&gt; &quot;boundary chk&quot; should be &quot;bo=
undary check&quot;.<br>&gt; column &quot;match_start dep.&quot;, I don&#39;=
t understand the meaning of &quot;direct&quot;<br>&gt; and &quot;boundary c=
heck&quot;.<br>&gt; &quot;count-dominance&quot; seems like an invented word=
, but I could not find<br>&gt; the explanation.<br>&gt; &quot;match_start-d=
ependent&quot; occurred many times, it would also deserve an<br>&gt; explan=
ation. We can also rename it as match_start_dependent.<br><br>All fair. v48=
 fixes the typo and adds a terminology block to README.rpr<br>defining coun=
t-dominance (the VIII-3 cover condition, named there),<br>match_start_depen=
dent (renamed to the underscore spelling, to match the<br>defineMatchStartD=
ependent identifier), and the &quot;direct&quot; vs &quot;boundary<br>check=
&quot; navigation distinction.<br><br><br>&gt; Attached is a minor refactor=
ing of ExecRPRProcessRow, no need for<br>&gt; boolean hasLimitedFrame.<br>&=
gt; ``if (hasLimitedFrame &amp;&amp; winstate-&gt;endOffsetValue !=3D 0)`` =
seems wrong<br>&gt; to me, endOffsetValue is a Datum, and you directly comp=
are it with 0.<br>&gt; see also calculate_frame_offsets.<br><br>Good eye on=
 the Datum comparison, and I agree your version reads more<br>clearly -- ga=
ting on the frame-option flag and going through<br>DatumGetInt64() rather t=
han comparing a Datum to 0, plus the two new<br>Asserts. (The existing code=
 is correct, since hasLimitedFrame carries<br>the limited/unbounded distinc=
tion separately, but the refactor is the<br>clearer shape.) I&#39;ll take i=
t.<br><br>One thing to watch when dropping hasLimitedFrame, though: a zero =
offset<br>is still a valid limited frame -- ROWS BETWEEN CURRENT ROW AND CU=
RRENT<br>ROW, and ... AND 0 FOLLOWING -- so the new check must not treat of=
fset 0<br>as unbounded, otherwise the boundary check is skipped and a quant=
ified<br>pattern (A+) silently absorbs the whole partition. The limited/unb=
ounded<br>distinction lives in the frame-option flag, not the offset value,=
 so the<br>refactor has to keep that flag in the picture.<br><br>We don&#39=
;t currently cover the offset-0 quantified case -- the two<br>zero-offset f=
rame tests in rpr_base both use a plain PATTERN (A) -- so<br>I&#39;ll add a=
n A+ regression test for it.<br><br><br>&gt; [nfa_evaluate_row&#39;s window=
_gettupleslot returning false]<br>&gt; indicates that it&#39;s not possible=
 for (currentPos &gt; ctxFrameEnd).<br>&gt; therefore [if (currentPos &gt;=
=3D ctxFrameEnd) ...]<br>&gt; should change to ``if (currentPos =3D=3D ctxF=
rameEnd)``.<br>&gt; With that, the comment wording &quot;exceeded&quot; see=
ms wrong too.<br><br>Agreed. `&gt;` is unreachable by the loop invariant --=
 currentPos advances<br>by exactly one per row, and once a context is final=
ized the<br>states =3D=3D NULL guard skips it -- so &gt;=3D and =3D=3D beha=
ve identically here,<br>and =3D=3D states the intent better. The `&gt;=3D` =
was a defensive guard against<br>an overshoot that can&#39;t actually happe=
n; v48 tightens it to =3D=3D, changes<br>the comment from &quot;exceeded&qu=
ot; to &quot;reached&quot;, and moves that defense into an<br>Assert(curren=
tPos &lt;=3D ctxFrameEnd) so a future change that breaks the<br>invariant t=
rips immediately instead of silently.<br><br><br>&gt; Since I couldn&#39;t =
generate a coverage report, I am using elog(INFO) to<br>&gt; test reachabil=
ity. Rerun the regress tests, you will find out that the<br>&gt; ELSE IF br=
anch below is not reached by current regress tests.<br>&gt; =C2=A0 =C2=A0 e=
lse if (targetCtx-&gt;states =3D=3D NULL)<br>&gt; =C2=A0 =C2=A0 {<br>&gt; =
=C2=A0 =C2=A0 =C2=A0 =C2=A0 /* Context already completed - skip to result r=
egistration */<br>&gt; =C2=A0 =C2=A0 =C2=A0 =C2=A0 goto register_result;<br=
>&gt; =C2=A0 =C2=A0 =C2=A0 =C2=A0 elog(INFO, &quot;XXXX reached&quot;);<br>=
&gt; =C2=A0 =C2=A0 }<br><br>The elog in your snippet sits *after* the goto,=
 so it can never print<br>even though the branch runs. Moving it above the =
goto shows it firing<br>across the suite. Easy mistake; I&#39;ve put a prob=
e right after a<br>goto/return more times than I&#39;d like to admit.<br><b=
r>The branch is correct as written: under SKIP TO NEXT ROW an overlapping<b=
r>context can finish (and be preserved by cleanup, since<br>matchEndRow &gt=
;=3D matchStartRow) before the next update_reduced_frame()<br>call lands on=
 its start row. So no new test is needed -- I&#39;ll just add a<br>comment =
above it explaining why the head context can already be complete<br>there.<=
br><br><br>Summary of decisions, in the order above:<br><br><br><font face=
=3D"monospace">=C2=A0 RPRContext consolidation =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
Tatsuo&#39;s call -- non-blocking for v48<br>=C2=A0 get_reduced_frame_statu=
s order =C2=A0 Keep -- cascade idiom; add comments<br>=C2=A0 README termino=
logy + typo =C2=A0 =C2=A0 =C2=A0 =C2=A0Accept -- definitions block + rename=
<br>=C2=A0 ExecRPRProcessRow refactor =C2=A0 =C2=A0 =C2=A0 Accept -- but fi=
x frameOffset<br>=C2=A0 currentPos &gt;=3D -&gt; =3D=3D =C2=A0 =C2=A0 =C2=
=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0Accept -- plus comment + Assert<br>=C2=A0 st=
ates =3D=3D NULL branch coverage =C2=A0 Already reached (155x) -- add comme=
nt</font><br><br><br>I&#39;ll post an incremental patch shortly with the ac=
cepted changes and the<br>comment additions; the RPRContext question is for=
 Tatsuo.<br><br>Thanks again,<br>Henson<br></div>

--000000000000cfb4fd0652eac355--