public inbox for [email protected]  

help / color / mirror / Atom feed

From: Henson Choi <[email protected]>
To: Tatsuo Ishii <[email protected]>
To: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Subject: Re: Row pattern recognition
Date: Thu, 11 Jun 2026 10:20:45 +0900
Message-ID: <CAAAe_zA4tuFvW70sTqdUchpa6bzGr3fptQ8orpc=4axvx=eYOg@mail.gmail.com> (raw)
In-Reply-To: <[email protected]>
References: <CACJufxFnwdQSApt2vWwYCd0gtf+JjFDxT2hbxHi=+dhFJc+-1g@mail.gmail.com>
	<CAAAe_zATnkqsbLYDj8MJV1TriX9Wi0wShDg3nK3qYpiupKwhFA@mail.gmail.com>
	<CAAAe_zBL+J0AYmvmcJQT7Q-gp5aRH0deJ7SE7-N21g4hWExyJw@mail.gmail.com>
	<[email protected]>

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

view thread (118+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
  Subject: Re: Row pattern recognition
  In-Reply-To: <CAAAe_zA4tuFvW70sTqdUchpa6bzGr3fptQ8orpc=4axvx=eYOg@mail.gmail.com>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox