MIME-Version: 1.0
References: 
 <CACJufxFnwdQSApt2vWwYCd0gtf+JjFDxT2hbxHi=+dhFJc+-1g@mail.gmail.com>
 <CAAAe_zATnkqsbLYDj8MJV1TriX9Wi0wShDg3nK3qYpiupKwhFA@mail.gmail.com>
 <CAAAe_zBL+J0AYmvmcJQT7Q-gp5aRH0deJ7SE7-N21g4hWExyJw@mail.gmail.com>
 <20260610.231902.1271202110858446443.ishii@postgresql.org>
In-Reply-To: <20260610.231902.1271202110858446443.ishii@postgresql.org>
Reply-To: assam258@gmail.com
From: Henson Choi <assam258@gmail.com>
Date: Thu, 11 Jun 2026 10:20:45 +0900
Message-ID: 
 <CAAAe_zA4tuFvW70sTqdUchpa6bzGr3fptQ8orpc=4axvx=eYOg@mail.gmail.com>
Subject: Re: Row pattern recognition
To: Tatsuo Ishii <ishii@postgresql.org>, 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="0000000000001af5f70653f02c05"
Archived-At: 
 <https://www.postgresql.org/message-id/CAAAe_zA4tuFvW70sTqdUchpa6bzGr3fptQ8orpc%3D4axvx%3DeYOg%40mail.gmail.com>
Precedence: bulk

--0000000000001af5f70653f02c05
Content-Type: text/plain; charset="UTF-8"

Hi Tatsuo,

== 1. DEFINE evaluation use-after-free (to Tatsuo) ==

>> 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.
>>
>> 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?
>
> Can we use winstate->tmpcontext instead?

I don't think we can.  tmpcontext is idle where update_reduced_frame
would reset it, but not *during* DEFINE evaluation: a DEFINE with NEXT()
re-enters the spooler from inside its ExecEvalExpr (ExecEvalRPRNavSet
-> window_gettupleslot -> spool_tuples), and spool_tuples resets
winstate->tmpcontext (via ExecQualAndReset on partEqfunction) for every
input row it pulls under a PARTITION BY.  ExecEvalRPRNavRestore parks a
pass-by-ref nav result in that per-tuple memory to survive until the
next reset.  To give one example,

  -- the cast forces a pass-by-ref result; a by-value bigint is safe
  DEFINE b AS PREV(price::numeric) < NEXT(price::numeric)

frees PREV's result when NEXT spools the next row, before numeric_lt()
reads it -- the same use-after-free as nocfbot-0039, just on a different
context.  It is the normal forward-scan path (the NFA runs ahead of the
spool), and after a tuplestore spill even NEXT-free DEFINEs hit it.

More generally, tmpcontext fits its existing users because each one sets
its slots, evaluates, and resets within a shallow, straight-line region;
it is best kept to that limited, low-depth usage.  DEFINE evaluation
re-enters the spooler mid-expression, so it cannot honor that contract.

So I think the dedicated DEFINE ExprContext is the right call: a
once-per-NFA-row reset is a third cadence, distinct from tmpcontext
(per-input-tuple) and ps_ExprContext (per-output-tuple) -- the same
one-context-per-cadence pattern nodeWindowAgg and nodeAgg already use.

Thanks,
Henson

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

<div dir=3D"ltr">Hi Tatsuo,<br><br>=3D=3D 1. DEFINE evaluation use-after-fr=
ee (to Tatsuo) =3D=3D<br><br>&gt;&gt; nocfbot-0039 (the DEFINE memory-leak =
fix) added a ResetExprContext() in<br>&gt;&gt; update_reduced_frame, but it=
 resets the wrong context.<br>&gt;&gt;<br>&gt;&gt; ps_ExprContext is the pe=
r-output-tuple context that ExecWindowAgg resets<br>&gt;&gt; once per outpu=
t row. =C2=A0update_reduced_frame now resets it once per NFA row,<br>&gt;&g=
t; while the output row is still being formed -- so a pass-by-ref window<br=
>&gt;&gt; function result already datum-copied into that per-tuple memory (=
when<br>&gt;&gt; numfuncs &gt; 1) is freed before ExecProject reads it.<br>=
&gt;&gt;<br>&gt;&gt; Neither v47 nor the patch is the answer on its own: v4=
7 had no reset here,<br>&gt;&gt; so no use-after-free, but the DEFINE scrat=
ch accumulated over the whole<br>&gt;&gt; forward scan (the leak nocfbot-00=
39 fixed); nocfbot-0039 added the per-row<br>&gt;&gt; reset but on the shar=
ed per-output-tuple context.=C2=A0 We do want a per-row<br>&gt;&gt; reset -=
- just not on that context.<br>&gt;&gt;<br>&gt;&gt; So I think this needs a=
 dedicated ExprContext for DEFINE evaluation, reset<br>&gt;&gt; once per NF=
A row: it keeps the memory bounded without touching the<br>&gt;&gt; per-out=
put-tuple results.<br>&gt;&gt;<br>&gt;&gt; Question: does a dedicated DEFIN=
E ExprContext look right to you?<br>&gt;<br>&gt; Can we use winstate-&gt;tm=
pcontext instead?<br><br>I don&#39;t think we can. =C2=A0tmpcontext is idle=
 where update_reduced_frame<br>would reset it, but not *during* DEFINE eval=
uation: a DEFINE with NEXT()<br>re-enters the spooler from inside its ExecE=
valExpr (ExecEvalRPRNavSet<br>-&gt; window_gettupleslot -&gt; spool_tuples)=
, and spool_tuples resets<br>winstate-&gt;tmpcontext (via ExecQualAndReset =
on partEqfunction) for every<br>input row it pulls under a PARTITION BY.=C2=
=A0 ExecEvalRPRNavRestore parks a<br>pass-by-ref nav result in that per-tup=
le memory to survive until the<br>next reset.=C2=A0 To give one example,<br=
><br>=C2=A0 -- the cast forces a pass-by-ref result; a by-value bigint is s=
afe<br>=C2=A0 DEFINE b AS PREV(price::numeric) &lt; NEXT(price::numeric)<br=
><br>frees PREV&#39;s result when NEXT spools the next row, before numeric_=
lt()<br>reads it -- the same use-after-free as nocfbot-0039, just on a diff=
erent<br>context.=C2=A0 It is the normal forward-scan path (the NFA runs ah=
ead of the<br>spool), and after a tuplestore spill even NEXT-free DEFINEs h=
it it.<br><br>More generally, tmpcontext fits its existing users because ea=
ch one sets<br>its slots, evaluates, and resets within a shallow, straight-=
line region;<br>it is best kept to that limited, low-depth usage.=C2=A0 DEF=
INE evaluation<br>re-enters the spooler mid-expression, so it cannot honor =
that contract.<br><br>So I think the dedicated DEFINE ExprContext is the ri=
ght call: a<br>once-per-NFA-row reset is a third cadence, distinct from tmp=
context<br>(per-input-tuple) and ps_ExprContext (per-output-tuple) -- the s=
ame<br>one-context-per-cadence pattern nodeWindowAgg and nodeAgg already us=
e.<br><br>Thanks,<br>Henson<br></div>

--0000000000001af5f70653f02c05--