Re: Row pattern recognition

Hi Henson,

> 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?

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

I need more time think about this.

Regards,
--
Tatsuo Ishii
SRA OSS K.K.
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp