MIME-Version: 1.0
References: 
 <CAAAe_zAZDuHSiVGvz9c6h=Pe=aN+FKZOrdNPfbTOk3XV+WFKYQ@mail.gmail.com>
 <CAAAe_zDz3z2Paidk3jHOm9S3eMVLoXRxK0Lyo=5i_9-EfSH7fA@mail.gmail.com>
 <20260604.132108.405136284364833955.ishii@postgresql.org>
 <20260609.171307.1883356507067957349.ishii@postgresql.org>
 <CACJufxFnwdQSApt2vWwYCd0gtf+JjFDxT2hbxHi=+dhFJc+-1g@mail.gmail.com>
 <CAAAe_zATnkqsbLYDj8MJV1TriX9Wi0wShDg3nK3qYpiupKwhFA@mail.gmail.com>
In-Reply-To: 
 <CAAAe_zATnkqsbLYDj8MJV1TriX9Wi0wShDg3nK3qYpiupKwhFA@mail.gmail.com>
Reply-To: assam258@gmail.com
From: Henson Choi <assam258@gmail.com>
Date: Wed, 10 Jun 2026 15:10:19 +0900
Message-ID: 
 <CAAAe_zBL+J0AYmvmcJQT7Q-gp5aRH0deJ7SE7-N21g4hWExyJw@mail.gmail.com>
Subject: Re: Row pattern recognition
To: Tatsuo Ishii <ishii@postgresql.org>, jian he <jian.universality@gmail.com>
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, li.evan.chao@gmail.com, pgsql-hackers@postgresql.org
Content-Type: multipart/alternative; boundary="000000000000d2a7a90653e01909"
Archived-At: 
 <https://www.postgresql.org/message-id/CAAAe_zBL%2BJ0AYmvmcJQT7Q-gp5aRH0deJ7SE7-N21g4hWExyJw%40mail.gmail.com>
Precedence: bulk

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

Hi Tatsuo, Jian,

While doing a self-review pass over the incremental fixes on top of v47 I
ran into two issues where I'd rather agree on an approach with you before
I pick one.  One of them is a regression I introduced myself in the DEFINE
memory-leak fix; the other is an original design point from v47.  There is
also a third bug which I plan to handle together with the second one, since
it can be affected by that change -- I describe it at the end.

I have verified both of the issues below on an assert-enabled build (and a
non-assert build where relevant).


== 1. DEFINE evaluation reuses the per-output-tuple context
(use-after-free) ==

nocfbot-0039 (the DEFINE memory-leak fix) added a ResetExprContext() in
update_reduced_frame, but it resets the wrong context.

ps_ExprContext is the per-output-tuple context that ExecWindowAgg resets
once per output row.  update_reduced_frame now resets it once per NFA row,
while the output row is still being formed -- so a pass-by-ref window
function result already datum-copied into that per-tuple memory (when
numfuncs > 1) is freed before ExecProject reads it.

Minimal trigger -- a pass-by-ref window function plus a second one over an
RPR window:

  SELECT lag(company) OVER w, count(*) OVER w FROM stock
   WINDOW w AS (PARTITION BY company ORDER BY tdate
                ROWS BETWEEN CURRENT ROW AND UNBOUNDED FOLLOWING
                AFTER MATCH SKIP PAST LAST ROW INITIAL
                PATTERN (START UP+)
                DEFINE START AS TRUE, UP AS price > PREV(price));

On a CLOBBER_FREED_MEMORY build the lag column comes out as 0x7F garbage;
in production it is garbage or a crash.  (An aggregate is not required --
lag + first_value hits the same reset via the frame-access path.)

Neither v47 nor the patch is the answer on its own: v47 had no reset here,
so no use-after-free, but the DEFINE scratch accumulated over the whole
forward scan (the leak nocfbot-0039 fixed); nocfbot-0039 added the per-row
reset but on the shared per-output-tuple context.  We do want a per-row
reset -- just not on that context.

So I think this needs a dedicated ExprContext for DEFINE evaluation, reset
once per NFA row: it keeps the memory bounded without touching the
per-output-tuple results.

Question: does a dedicated DEFINE ExprContext look right to you?


== 2. PREV/NEXT/FIRST/LAST placeholders collide with user functions ==

The nav operations are polymorphic pg_catalog functions (anyelement, OIDs
8126-8133) recognized by funcid in parse_func.c, which collides with
same-name user functions.

Outside DEFINE, a same-name function masks or clashes with the placeholder:
with public.last(anyelement), SELECT last(123) fails "cannot use last
outside a DEFINE clause"; with public.next(numeric), SELECT next(10) fails
"function next(integer) is not unique"; and even with no user function,
last(123) errors instead of "function last(integer) does not exist".

Inside DEFINE, a same-name function with an exact-type match beats the
anyelement placeholder, so PREV(price) silently becomes a plain FuncExpr
instead of an RPRNavExpr -- a wrong match result with no error (reproduced
for numeric, text and int).  And ruleutils deparses a bare PREV(, so
reparsing a view under a search_path with public.prev rebinds it (pg_dump
is safe via search_path = '').

This is original v47 design, not a regression.  Per the standard,
PREV/NEXT/FIRST/LAST are navigation operations with dedicated syntax, not
general-namespace functions -- the collision comes from mapping them onto
catalog functions plus search-path resolution.

I haven't found a clean approach yet.  Inside DEFINE these names have to be
the navigation operation (per the standard), yet outside DEFINE they
shouldn't shadow or break same-name user functions the way the catalog
placeholders do -- and since the deparse output is unqualified (a bare
PREV(...)), whatever we choose also has to round-trip cleanly.  I'm not
sure how best to reconcile those.

My rough leaning is to not add catalog functions for these at all: leave
resolution outside DEFINE exactly as it is today, and only inside DEFINE
adjust the function-resolution path itself to recognize the navigation
operations.  But that is still quite abstract.

Question: how would you approach this?


== Note: a third bug, to be handled together with item 2 ==

A navigation operation nested inside another nav's offset argument -- e.g.
PREV(price, NEXT(2::bigint, 0)) in a DEFINE clause -- slips past the parser
but trips Assert(!IsA(nav->offset_arg, RPRNavExpr)) in the planner.  So it
aborts at plan time on an assert build; without asserts, the backward form
PREV(price, PREV(2::bigint, 5)) reaches a runtime "cannot fetch row ...
before mark position".

The fix is to reject a nav inside an offset argument in the DEFINE walk.
But since item 2 may reshape that walker substantially from how it works
today, I'll do it together with item 2 and add it as a regression test
there.

Thanks,
Henson

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

<div dir=3D"ltr">Hi Tatsuo, Jian,<br><br>While doing a self-review pass ove=
r the incremental fixes on top of v47 I<br>ran into two issues where I&#39;=
d rather agree on an approach with you before<br>I pick one.=C2=A0 One of t=
hem is a regression I introduced myself in the DEFINE<br>memory-leak fix; t=
he other is an original design point from v47.=C2=A0 There is<br>also a thi=
rd bug which I plan to handle together with the second one, since<br>it can=
 be affected by that change -- I describe it at the end.<br><br>I have veri=
fied both of the issues below on an assert-enabled build (and a<br>non-asse=
rt build where relevant).<br><br><br>=3D=3D 1. DEFINE evaluation reuses the=
 per-output-tuple context (use-after-free) =3D=3D<br><br>nocfbot-0039 (the =
DEFINE memory-leak fix) added a ResetExprContext() in<br>update_reduced_fra=
me, but it resets the wrong context.<br><br>ps_ExprContext is the per-outpu=
t-tuple context that ExecWindowAgg resets<br>once per output row. =C2=A0upd=
ate_reduced_frame now resets it once per NFA row,<br>while the output row i=
s still being formed -- so a pass-by-ref window<br>function result already =
datum-copied into that per-tuple memory (when<br>numfuncs &gt; 1) is freed =
before ExecProject reads it.<br><br>Minimal trigger -- a pass-by-ref window=
 function plus a second one over an<br>RPR window:<br><br>=C2=A0 SELECT lag=
(company) OVER w, count(*) OVER w FROM stock<br>=C2=A0 =C2=A0WINDOW w AS (P=
ARTITION BY company ORDER BY tdate<br>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 =C2=A0 =C2=A0 ROWS BETWEEN CURRENT ROW AND UNBOUNDED FOLLOWING<br>=
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 AFTER MATCH SKIP PA=
ST LAST ROW INITIAL<br>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
=A0 PATTERN (START UP+)<br>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0=
 =C2=A0 DEFINE START AS TRUE, UP AS price &gt; PREV(price));<br><br>On a CL=
OBBER_FREED_MEMORY build the lag column comes out as 0x7F garbage;<br>in pr=
oduction it is garbage or a crash. =C2=A0(An aggregate is not required --<b=
r>lag + first_value hits the same reset via the frame-access path.)<br><br>=
Neither v47 nor the patch is the answer on its own: v47 had no reset here,<=
br>so no use-after-free, but the DEFINE scratch accumulated over the whole<=
br>forward scan (the leak nocfbot-0039 fixed); nocfbot-0039 added the per-r=
ow<br>reset but on the shared per-output-tuple context.=C2=A0 We do want a =
per-row<br>reset -- just not on that context.<br><br>So I think this needs =
a dedicated ExprContext for DEFINE evaluation, reset<br>once per NFA row: i=
t keeps the memory bounded without touching the<br>per-output-tuple results=
.<br><br>Question: does a dedicated DEFINE ExprContext look right to you?<b=
r><br><br>=3D=3D 2. PREV/NEXT/FIRST/LAST placeholders collide with user fun=
ctions =3D=3D<br><br>The nav operations are polymorphic pg_catalog function=
s (anyelement, OIDs<br>8126-8133) recognized by funcid in parse_func.c, whi=
ch collides with<br>same-name user functions.<br><br>Outside DEFINE, a same=
-name function masks or clashes with the placeholder:<br>with public.last(a=
nyelement), SELECT last(123) fails &quot;cannot use last<br>outside a DEFIN=
E clause&quot;; with public.next(numeric), SELECT next(10) fails<br>&quot;f=
unction next(integer) is not unique&quot;; and even with no user function,<=
br>last(123) errors instead of &quot;function last(integer) does not exist&=
quot;.<br><br>Inside DEFINE, a same-name function with an exact-type match =
beats the<br>anyelement placeholder, so PREV(price) silently becomes a plai=
n FuncExpr<br>instead of an RPRNavExpr -- a wrong match result with no erro=
r (reproduced<br>for numeric, text and int).=C2=A0 And ruleutils deparses a=
 bare PREV(, so<br>reparsing a view under a search_path with public.prev re=
binds it (pg_dump<br>is safe via search_path =3D &#39;&#39;).<br><br>This i=
s original v47 design, not a regression.=C2=A0 Per the standard,<br>PREV/NE=
XT/FIRST/LAST are navigation operations with dedicated syntax, not<br>gener=
al-namespace functions -- the collision comes from mapping them onto<br>cat=
alog functions plus search-path resolution.<br><br>I haven&#39;t found a cl=
ean approach yet.=C2=A0 Inside DEFINE these names have to be<br>the navigat=
ion operation (per the standard), yet outside DEFINE they<br>shouldn&#39;t =
shadow or break same-name user functions the way the catalog<br>placeholder=
s do -- and since the deparse output is unqualified (a bare<br>PREV(...)), =
whatever we choose also has to round-trip cleanly.=C2=A0 I&#39;m not<br>sur=
e how best to reconcile those.<br><br>My rough leaning is to not add catalo=
g functions for these at all: leave<br>resolution outside DEFINE exactly as=
 it is today, and only inside DEFINE<br>adjust the function-resolution path=
 itself to recognize the navigation<br>operations.=C2=A0 But that is still =
quite abstract.<br><br>Question: how would you approach this?<br><br><br>=
=3D=3D Note: a third bug, to be handled together with item 2 =3D=3D<br><br>=
A navigation operation nested inside another nav&#39;s offset argument -- e=
.g.<br>PREV(price, NEXT(2::bigint, 0)) in a DEFINE clause -- slips past the=
 parser<br>but trips Assert(!IsA(nav-&gt;offset_arg, RPRNavExpr)) in the pl=
anner.=C2=A0 So it<br>aborts at plan time on an assert build; without asser=
ts, the backward form<br>PREV(price, PREV(2::bigint, 5)) reaches a runtime =
&quot;cannot fetch row ...<br>before mark position&quot;.<br><br>The fix is=
 to reject a nav inside an offset argument in the DEFINE walk.<br>But since=
 item 2 may reshape that walker substantially from how it works<br>today, I=
&#39;ll do it together with item 2 and add it as a regression test there.<b=
r><br>Thanks,<br>Henson<br></div>

--000000000000d2a7a90653e01909--