Re: Row pattern recognition

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