MIME-Version: 1.0
References: <CAAAe_zBpgH_06Utd9r1rVYDLaF-UuTtT1_W69bVeEP4MBzvDBQ@mail.gmail.com>
 <20260209.160520.873483599710391856.ishii@postgresql.org> <CAAAe_zALWNDOcTMCOk71d3FD5MHQLiCsL-US5ok=7ZoX=xtHFg@mail.gmail.com>
 <20260209.190209.470246357440525054.ishii@postgresql.org>
In-Reply-To: <20260209.190209.470246357440525054.ishii@postgresql.org>
Reply-To: assam258@gmail.com
From: Henson Choi <assam258@gmail.com>
Date: Wed, 11 Feb 2026 10:42:56 +0900
Message-ID: <CAAAe_zDX24SnaQFwTNVUx_vK5887mpD+rXTaedjzdfFrGROFGA@mail.gmail.com>
Subject: Re: Row pattern recognition
To: Tatsuo Ishii <ishii@postgresql.org>
Cc: jacob.champion@enterprisedb.com, david.g.johnston@gmail.com, 
	vik@postgresfriends.org, er@xs4all.nl, peter@eisentraut.org, 
	pgsql-hackers@postgresql.org
Content-Type: multipart/alternative; boundary="0000000000006cac3b064a827e13"
Archived-At: <https://www.postgresql.org/message-id/CAAAe_zDX24SnaQFwTNVUx_vK5887mpD%2BrXTaedjzdfFrGROFGA%40mail.gmail.com>
Precedence: bulk

--0000000000006cac3b064a827e13
Content-Type: text/plain; charset="UTF-8"

Hi Ishii-san,

During code review of nodeWindowAgg.c, I discovered that
update_reduced_frame()
can receive the same pos value repeatedly for RANGE/GROUPS frames.


----------------------------------------------------------------------
PATTERN DISCOVERED
----------------------------------------------------------------------

For RANGE/GROUPS frames with peer rows (rows having the same ORDER BY
value):

  Data: id=1,2,3 all have val=10 (peer rows)
        id=4,5 both have val=20 (peer rows)

  RANGE frame processing:
  - id=1 processing: pos=0 (frameheadpos points to first peer)
  - id=2 processing: pos=0 (same frameheadpos)
  - id=3 processing: pos=0 (same frameheadpos)
  - id=4 processing: pos=3
  - id=5 processing: pos=3

ROWS frames always have sequential pos values (0,1,2,3,4...), but
RANGE/GROUPS
can have repeated pos values (0,0,0,3,3...).


----------------------------------------------------------------------
CURRENT IMPLEMENTATION ISSUE
----------------------------------------------------------------------

The current implementation appears to assume sequential processing only,
which
is valid for ROWS frames. Two specific issues found:

1. In update_reduced_frame():
   - The pos <= nfaLastProcessedRow check assumes "already processed pos
doesn't
     need reprocessing", but RANGE/GROUPS require processing the same pos
     multiple times.

2. In nfa_process_row():
   - Frame boundary calculation: ctxFrameEnd = matchStartRow + frameOffset
+ 1
   - This adds frameOffset directly to row position, which works for ROWS
but
     may be incorrect for RANGE (value-based) and GROUPS (group-based)
frames.


----------------------------------------------------------------------
TEST COVERAGE LIMITATION
----------------------------------------------------------------------

The existing RANGE/GROUPS test cases in rpr_base.sql only cover simple
scenarios
that don't reach the frame end, which is why this issue wasn't detected.

Most tests target ROWS frames, with only a small number testing RANGE/GROUPS
frames.


----------------------------------------------------------------------
PROPOSAL
----------------------------------------------------------------------

Rather than directly modifying these functions, we should consider an
alternative approach for non-UNBOUNDED frames:
- Strictly respect the frame head position
- Minimize lower-level optimizations (absorption/pruning)
- Avoid creating subsequent contexts for limited reuse benefit
- For bounded frames, the benefit of context reuse across rows may be
limited
  compared to UNBOUNDED frames where contexts can be reused extensively
- This would simplify RANGE/GROUPS handling at the cost of some optimization

To implement this properly, we need to broaden our understanding of the
execution structure through code review of Window clauses and Aggregate
functions.

Therefore, for this round of nodeWindowAgg.c review:
1. Perform code simplification/refactoring for ROWS frames only
2. Add FIXME comments to RANGE/GROUPS test cases
3. Defer RANGE/GROUPS handling to a future phase

There may be other issues beyond RANGE/GROUPS.
After this code review round is complete, I would like to regroup and
discuss
the direction forward.

Best regards,
Henson

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

<div dir=3D"ltr"><div dir=3D"ltr">Hi Ishii-san,<br><br>During code review o=
f nodeWindowAgg.c, I discovered that update_reduced_frame()<br>can receive =
the same pos value repeatedly for RANGE/GROUPS frames.<br><br><br><font fac=
e=3D"monospace">-----------------------------------------------------------=
-----------<br>PATTERN DISCOVERED<br>--------------------------------------=
--------------------------------</font><br><br>For RANGE/GROUPS frames with=
 peer rows (rows having the same ORDER BY value):<br><br>=C2=A0 Data: id=3D=
1,2,3 all have val=3D10 (peer rows)<br>=C2=A0 =C2=A0 =C2=A0 =C2=A0 id=3D4,5=
 both have val=3D20 (peer rows)<br><br>=C2=A0 RANGE frame processing:<br>=
=C2=A0 - id=3D1 processing: pos=3D0 (frameheadpos points to first peer)<br>=
=C2=A0 - id=3D2 processing: pos=3D0 (same frameheadpos)<br>=C2=A0 - id=3D3 =
processing: pos=3D0 (same frameheadpos)<br>=C2=A0 - id=3D4 processing: pos=
=3D3<br>=C2=A0 - id=3D5 processing: pos=3D3<br><br>ROWS frames always have =
sequential pos values (0,1,2,3,4...), but RANGE/GROUPS<br>can have repeated=
 pos values (0,0,0,3,3...).<br><br><br><font face=3D"monospace">-----------=
-----------------------------------------------------------<br>CURRENT IMPL=
EMENTATION ISSUE<br>-------------------------------------------------------=
---------------</font><br><br>The current implementation appears to assume =
sequential processing only, which<br>is valid for ROWS frames. Two specific=
 issues found:<br><br>1. In update_reduced_frame():<br>=C2=A0 =C2=A0- The p=
os &lt;=3D nfaLastProcessedRow check assumes &quot;already processed pos do=
esn&#39;t<br>=C2=A0 =C2=A0 =C2=A0need reprocessing&quot;, but RANGE/GROUPS =
require processing the same pos<br>=C2=A0 =C2=A0 =C2=A0multiple times.<br><=
br>2. In nfa_process_row():<br>=C2=A0 =C2=A0- Frame boundary calculation: c=
txFrameEnd =3D matchStartRow + frameOffset + 1<br>=C2=A0 =C2=A0- This adds =
frameOffset directly to row position, which works for ROWS but<br>=C2=A0 =
=C2=A0 =C2=A0may be incorrect for RANGE (value-based) and GROUPS (group-bas=
ed) frames.<br><br><br><font face=3D"monospace">---------------------------=
-------------------------------------------<br>TEST COVERAGE LIMITATION<br>=
----------------------------------------------------------------------</fon=
t><br><br>The existing RANGE/GROUPS test cases in rpr_base.sql only cover s=
imple scenarios<br>that don&#39;t reach the frame end, which is why this is=
sue wasn&#39;t detected.<br><br>Most tests target ROWS frames, with only a =
small number testing RANGE/GROUPS<br>frames.<br><br><br><font face=3D"monos=
pace">---------------------------------------------------------------------=
-<br>PROPOSAL<br>----------------------------------------------------------=
------------<br></font><br>Rather than directly modifying these functions, =
we should consider an<br>alternative approach for non-UNBOUNDED frames:<br>=
- Strictly respect the frame head position<br>- Minimize lower-level optimi=
zations (absorption/pruning)<br>- Avoid creating subsequent contexts for li=
mited reuse benefit<br>- For bounded frames, the benefit of context reuse a=
cross rows may be limited<br>=C2=A0 compared to UNBOUNDED frames where cont=
exts can be reused extensively<br>- This would simplify RANGE/GROUPS handli=
ng at the cost of some optimization<br><br></div><div dir=3D"ltr">To implem=
ent this properly, we need to broaden our understanding of the<br>execution=
 structure through code review of Window clauses and Aggregate<br>functions=
.<br><br>Therefore, for this round of nodeWindowAgg.c review:<br>1. Perform=
 code simplification/refactoring for ROWS frames only<br>2. Add FIXME comme=
nts to RANGE/GROUPS test cases<br>3. Defer RANGE/GROUPS handling to a futur=
e phase<br><br>There may be other issues beyond RANGE/GROUPS.<br>After this=
 code review round is complete, I would like to regroup and discuss<br>the =
direction forward.<br><br>Best regards,<br><div>Henson</div></div></div>

--0000000000006cac3b064a827e13--