Re: Row pattern recognition

Hi Henson,

> Hi Tatsuo,
>
> Sorry for sending these before your next revision of v45 --
> I'll be busy with a project proposal for the next couple of
> weeks, so I wanted to share them before that.

No problem at all. Thank you for your work.

> Here are the patches on top of v45, incorporating the fixes and
> improvements I mentioned earlier, plus additional changes from a
> thorough code review. There are quite a few patches since I went
> ahead without prior discussion -- please review each one individually
> and feel free to drop any that you disagree with.
>
> Here's a summary of each patch (to be applied on top of v45):
>
> 0001: Fix mergeGroupPrefixSuffix() max increment
> When absorbing prefix/suffix, child->max was not incremented,
> causing incorrect quantifier bounds. (reported by Zsolt)

Looks good to me.

> 0002: Fix RPR error codes and GROUPS typo
> Use ERRCODE_WINDOWING_ERROR instead of ERRCODE_SYNTAX_ERROR
> for semantic window-frame violations. Fix "GROUP" typo to
> "GROUPS" in frame validation error message. (reported by Zsolt)

Looks good to me.

> 0003: Add check_stack_depth() and CHECK_FOR_INTERRUPTS()
> Add stack depth protection to recursive pattern optimization
> and interrupt checks to NFA engine loops.

Looks good to me.

> 0004: Fix window_last_value set_mark during RPR
> Restore set_mark=true in window_last_value (normal behavior)
> and add an RPR-specific override inside WinGetFuncArgInFrame.
> This keeps the RPR workaround localized rather than changing
> the caller's semantics unconditionally.

So, if RPR is active, WinSetMarkPosition() is not called at all? That
seems too strong limitation. Can't we set mark at frameheadpos even
if RPR is active? It seems safe since the only allowed franme start
option is ROWS BETWEEN CURRENT ROW and we never step back to the rows
before frameheadpos.

> 0005: Fix row_is_in_reduced_frame in WINDOW_SEEK_TAIL
> Pass frameheadpos directly to row_is_in_reduced_frame
> instead of frameheadpos+relpos. Currently only last_value()
> calls this path with relpos=0 so no actual bug, but the
> old expression would be incorrect for negative relpos.
> Also add a bounds check for future callers.

Looks good to me.

> 0006: Clarify ST_NONE intent
> Add comment explaining ST_NONE = 0 is the default for
> non-RPR windows.

Looks good to me.

> 0007: Clarify inverse transition optimization comment
> Document why RPR disables inverse transition: the reduced
> frame changes row by row.

Looks good to me.

> 0008: Reject unused DEFINE variables
> I know you preferred silently ignoring unused DEFINE
> variables [1], and I agree the standard doesn't mandate
> an error. However, if we later add qualified column
> references (e.g. B AS A.price > 100), B's expression
> depends on A being present. If A is not used in PATTERN
> and the planner silently removes it, B's reference to A
> becomes dangling. I worry that silently allowing this now
> could create forward-compatibility problems once qualified
> references are introduced. For that reason, I'm inclined
> to think rejecting them now may be safer than changing
> behavior later, which would be a user-visible compatibility
> break. This is also consistent with Oracle's behavior
> (ORA-62503), as SungJun reported, and it helps catch user
> typos in DEFINE variable names at parse time.

Ok, let's throw an error in the case then.

> 0009: Clarify RPR documentation in advanced.sgml
> Improve absorption explanation and clarify non-match row
> aggregation behavior with concrete examples.

Looks good to me.

> 0010: Fix typos in RPR comments and parser README

Looks good to me.

> 0011: Clarify excludeLocation and empty quantifier in gram.y
> Add comments explaining the conditional location assignment
> pattern and the empty quantifier rule.

Looks good to me.

> 0012: Clarify RPR_VARID_MAX definition
> Document varId range 0-250 and reserved control element
> values 252+.

Looks good to me.

> 0013: Move local variables to function scope
> In row_is_in_reduced_frame, move declarations out of
> switch case blocks.

Looks good to me.

> 0014: Reset reduced_frame_map pointer in release_partition
> Set reduced_frame_map = NULL and alloc_sz = 0 to prevent
> dangling pointer after partition context reset.

Looks good to me.

> 0015: Remove redundant list manipulation in nfa_add_matched_state
> Simplify doubly-linked list operation that was duplicated
> by the subsequent ExecRPRFreeContext() call.

Looks good to me.

> experimental: Implement 1-slot PREV/NEXT navigation

Great work!

> The slot-based PREV/NEXT approach I mentioned earlier.
> This no longer needs attno_map, so the plan cache mutation
> issue is gone. PREV/NEXT now also accepts an optional
> second argument for offset, e.g. PREV(price, 2).
>
> Note: all existing regression tests pass with this patch,
> but my understanding of TupleSlot internals and JIT is still
> shallow -- the implementation largely copies surrounding
> patterns, so there may be issues I haven't caught yet. I'd
> like to keep this as a separate patch on top of v46 until it
> is trustworthy enough, rather than folding it into the main
> series. I'll continue reviewing and hardening it.

Agreed. Let's keep this as a separate patch.

> One design decision regarding PREV/NEXT: the SQL standard
> requires that the first argument of PREV/NEXT contain at least
> one column reference [2] -- e.g. PREV(1) is a syntax error.
> Beyond the standard's rationale (no starting row for offsetting),
> there is a practical reason: when the target row falls outside
> the frame, PREV should return NULL, but a constant expression
> like PREV(42) would still evaluate to 42 since it never reads
> from the slot. I think it's correct to follow the standard and
> reject PREV/NEXT without a column reference. What do you think?

I agree with you. Let's follow the standard/

I am going to work on v46 patch sets.

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