Re: Row pattern recognition

Hi hackers,

This is an increment on top of v49: it lands the two fixes I left as
still-to-come there -- the DEFINE-evaluation use-after-free, now a dedicated
ExprContext, and the PREV/NEXT/FIRST/LAST namespace collision -- adds a
correctness fix found while reviewing the tuplestore spool (dormant
matches), and applies Jian He's and Tatsuo Ishii's review of the v48 series
as a set of mostly behavior-neutral commits.

Before the patch list, a note on CI: cfbot has been red here, but the
failure is not RPR -- it is the libLLVM 19 + ASAN JIT crash (CF 6870), which
reproduces on plain master.  The build-system fix (exclude sanitizer flags
from JIT bitcode generation) is Matheus Alcantara's; the meson half is
v3-0001-Exclude-sanitizer-flags-from-LLVM-JIT-bitcode-gen.patch:


https://postgr.es/m/CAAAe_zBX5uV9K0ikuROLgdNvDCgGqHRskT-73L+oX9=3aXR2AQ@mail.gmail.com

For v50, what would you think about folding that meson patch in as a
temporary prerequisite at the front of the series, so cfbot's ASAN build
gets past the JIT crash and actually exercises RPR?

First, two cleanups splitting changes unrelated to RPR out of the feature
patch:

  nocfbot-0001  Remove blank-line changes unrelated to row pattern
                recognition
  nocfbot-0002  Remove unnecessary includes from the row pattern
                recognition patch

The fixes -- the two I left as still-to-come in v48, plus the dormant-match
fix found since (nocfbot-0003..0005, all behavior-changing):

  nocfbot-0003  Recognize row pattern navigation operations by name in
                DEFINE
      The placeholder PREV/NEXT/FIRST/LAST pg_proc functions polluted the
      ordinary function namespace and could be silently misbound to
      same-named user functions.  They are dropped; an unqualified
      PREV/NEXT/FIRST/LAST inside a DEFINE clause is recognized as
      navigation by name, while a schema-qualified call still reaches an
      ordinary function.  A navigation operation inside a navigation offset
      -- which must be a run-time constant -- is now rejected instead of
      crashing the planner.  (f).prev follows attribute notation as an
      ordinary function, the same inside and outside DEFINE.

  nocfbot-0004  Use a dedicated ExprContext for RPR DEFINE clause
                evaluation
      nocfbot-0039's interim leak fix had turned into a use-after-free and
      was voided in v48.  DEFINE evaluation now has its own ExprContext,
      reset once per row and separate from both the per-output-tuple context
      and tmpcontext, resolving the leak and the use-after-free together.

  nocfbot-0005  Drive RPR row pattern matching once per row
      Matching only advanced when a window function read the frame, so a row
      whose only window function skips the frame (e.g. nth_value() with a
      NULL offset) left the match behind the current row -- silently wrong
      results and a spurious "cannot fetch row ... before WindowObject's
mark
      position" error.
      The match is now driven once per row, before the window functions run;
      the navigation mark advances from the frontier the match reached, so
      the tuplestore is trimmed sooner.

Review of v48 (Jian He, and Tatsuo Ishii), as nocfbot-0006..0013 --
behavior-neutral except where tagged [behavior change]:

  nocfbot-0006  Tidy up row pattern recognition plumbing
      Remove the dead collectPatternVariables()/buildDefineVariableList()
      helpers; drop the redundant rpSkipTo/defineClause arguments of
      make_windowagg(); mark RPRNavExpr.resulttype query_jumble_ignore;
      rename WindowAggState.defineClauseList to defineClauseExprs; assorted
      block flattening.  No change to planner or executor output.

  nocfbot-0007  Further tidy up row pattern recognition plumbing
      Drop the now-unused WindowClause argument of transformDefineClause();
      use foreach_node()/foreach_current_index() in the DEFINE walkers and
      drop their redundant end-of-list break tests; minor include and
      comment fixups.  No output change.

  nocfbot-0008  Refactor transformDefineClause in row pattern recognition
                [behavior change]
      Hoist the "DEFINE variable not used in PATTERN" cross-check out of the
      recursive walker into its caller, and reorder per-variable processing
      to transformExpr -> coerce_to_boolean -> pull_var_clause, dropping the
      separate second coercion pass.  The only observable change is one
      error-cursor position: the duplicate-variable error now points at the
      later definition.  New regression coverage for DEFINE coercion and Var
      propagation is added.

  nocfbot-0009  Replace a bare block with an else in the RPR DEFINE clause
                walker
      Cosmetic flattening of define_walker()'s phase dispatch into an
      if / else if / else chain.

  nocfbot-0010  Rename loop index variables in row pattern deparse helpers
      Tatsuo's suggestion; descriptive names for the deparse index/loop
      variables, no change to deparsed output.

  nocfbot-0011  Rename absorption "judgment point" to "comparison point" in
                comments
      Comment and executor-README wording only; identifiers unchanged.

  nocfbot-0012  Improve comments, documentation, and naming for row pattern
                recognition
      A batch of comment/doc clarity fixes -- the AST-vs-parse-tree wording,
      the Run Condition EXPLAIN test, the contain_rpr_walker comment, the
      ALT-marker and quantifier comments, the RPCommonSyntax.location "or
-1"
      convention, and the transformDefineClause header -- plus renaming the
      saturated-count sentinel RPR_COUNT_MAX to RPR_COUNT_INF for
      consistency with RPR_QUANTITY_INF.

  nocfbot-0013  Document eval_nav_offset_helper's NULL/negative offset
                handling
      Comment only.  The NEEDS_EVAL offset branch is reachable (a Param
      offset can be NULL or negative at run time), so it stays a graceful
      return rather than an assertion.


For traceability, where each patch came from:

  patch          proposer        proposal
  -------------  --------------
 -------------------------------------------------
  nocfbot-0001   Henson          drop blank-line churn unrelated to RPR
  nocfbot-0002   Henson          drop unnecessary includes
  nocfbot-0003   Henson          nav namespace collision; (f).prev as
ordinary function
  nocfbot-0004   Henson          dedicated ExprContext for DEFINE evaluation
  nocfbot-0005   Henson          drive row pattern matching once per row
  nocfbot-0006   Jian He         tidy RPR plumbing
  nocfbot-0007   Henson          further plumbing tidy
  nocfbot-0008   Jian He         refactor transformDefineClause
  nocfbot-0009   Jian He         bare block -> else in the DEFINE walker
  nocfbot-0010   Tatsuo Ishii    rename deparse loop/index variables
  nocfbot-0011   Jian He         "judgment point" -> "comparison point"
wording
  nocfbot-0012   Jian He         comment/doc clarity batch + RPR_COUNT_INF
rename
  nocfbot-0013   Jian He         document eval_nav_offset NULL/negative
offset handling


Still to come -- Jian He's follow-ups on this thread (2026-06-19), to fold
once each approach is agreed (all under review):

  topic                          proposed change
  -----------------------------  -------------------------------------------
  DEFINE qualified column-ref    make the differing pg_temp.t.c /
   error messages                 public.t.c / t.c messages consistent, on
                                  the standard "invalid reference to
                                  FROM-clause entry for table"
  validateRPRPatternVarCount     rename to preprocessRPRPattern
  quantifier INF bound           abstract behind an RPR_QUANTITY_INF macro
  "nullable" variables           rename to match_empty
  splitRPRTrailingAlt            use foreach_node / foreach_current_index
  buildRPRPattern signature      pass the WindowClause directly
  collectDefineVariables /       inline and drop the helpers
   tryUnwrapSingleChild
  variable-limit error msg       fold the maximum into the primary message


Quality work to run alongside, on Linux: Valgrind (leak and use-after-free,
to exercise the DEFINE ExprContext fix, whose reproducer is cassert-only)
and standing gcov coverage to catch untested planner paths.


Longer term, and out of scope for this CF entry:

  - Smaller documentation and error-message follow-ups (glossary, the
    bounded-quantifier message, README wording).
  - Short-circuit / tri-state DEFINE evaluation, as a separate series.
  - SEEK clause support (SQL:2016).
  - Empty pattern PATTERN () -- correctly rejected today; deferred because
    its empty-match semantics (SHOW/OMIT EMPTY MATCHES) are tied to the
    still-out-of-scope MEASURES clause.
  - Relaxing the DEFINE-subquery over-rejection where the standard permits
    it (the subquery does no RPR of its own and makes no outer reference).
  - Prefix-pattern absorption (an optimization; designed and intentionally
    split out as its own series).
  - R010 (MATCH_RECOGNIZE in the FROM clause) and the shared RPRContext it
    would back.


Please let me know if any of the slicing or grouping looks off.

On a personal note, some fatigue has built up, so I'll be easing off the
pace a little for a while and may be slower to follow up on this thread
than I have been.  The work continues -- just at a gentler pace.

Thanks again to Jian and Tatsuo for the careful review.

Best regards,
Henson