Re: Row pattern recognition

public inbox for [email protected]  
help / color / mirror / Atom feed

From: Henson Choi <[email protected]>
To: Tatsuo Ishii <[email protected]>
To: jian he <[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: Wed, 10 Jun 2026 15:10:19 +0900
Message-ID: <CAAAe_zBL+J0AYmvmcJQT7Q-gp5aRH0deJ7SE7-N21g4hWExyJw@mail.gmail.com> (raw)
In-Reply-To: <CAAAe_zATnkqsbLYDj8MJV1TriX9Wi0wShDg3nK3qYpiupKwhFA@mail.gmail.com>
References: <CAAAe_zAZDuHSiVGvz9c6h=Pe=aN+FKZOrdNPfbTOk3XV+WFKYQ@mail.gmail.com>
	<CAAAe_zDz3z2Paidk3jHOm9S3eMVLoXRxK0Lyo=5i_9-EfSH7fA@mail.gmail.com>
	<[email protected]>
	<[email protected]>
	<CACJufxFnwdQSApt2vWwYCd0gtf+JjFDxT2hbxHi=+dhFJc+-1g@mail.gmail.com>
	<CAAAe_zATnkqsbLYDj8MJV1TriX9Wi0wShDg3nK3qYpiupKwhFA@mail.gmail.com>

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

view thread (109+ messages)

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_zBL+J0AYmvmcJQT7Q-gp5aRH0deJ7SE7-N21g4hWExyJw@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