Re: Row pattern recognition

Hi Tatsuo,

Thank you very much for the careful review covering 0001-0031 in a
single pass.

0001 was already reviewed (LGTM).
> 0002-0003 looks good to me.
> 0004 was already reviewed (LGTM).
> 0005 thanks for review.
>

Thank you for confirming.

> Comment to A1:
>
> +-- A1. Frame optimization bypass
> :
> :
> +-- Non-RPR window with default frame -> frame optimization applied
> +EXPLAIN (COSTS OFF)
> +SELECT count(*) OVER w FROM rpr_integ
> +WINDOW w AS (ORDER BY id);
> + QUERY PLAN
> +-----------------------------------
> + WindowAgg
> + Window: w AS (ORDER BY id)
> + -> Sort
> + Sort Key: id
> + -> Seq Scan on rpr_integ
> +(5 rows)
>
> The comment says frame optimization applied. But I don't see any frame
> option changes (see how the first query changes the frame options).
>
> I thought a frame optimization is something like this.
> EXPLAIN(COSTS OFF)
> SELECT row_number() OVER W FROM generate_series(1,5) s(i)
> WINDOW w AS (
> ORDER BY i
> ROWS BETWEEN CURRENT ROW AND UNBOUNDED FOLLOWING
> );
> QUERY PLAN
> ------------------------------------------------------
> WindowAgg
> Window: w AS (ORDER BY i ROWS UNBOUNDED PRECEDING)
> -> Sort
> Sort Key: i
> -> Function Scan on generate_series s
> (5 rows)
>
> EXPLAIN(COSTS OFF)
> SELECT row_number() OVER W FROM generate_series(1,5) s(i)
> WINDOW w AS (
> ORDER BY i
> ROWS BETWEEN CURRENT ROW AND UNBOUNDED FOLLOWING
> PATTERN (A+)
> DEFINE A AS i > 2
> );
> QUERY PLAN
>
>
> ------------------------------------------------------------------------------
> WindowAgg
> Window: w AS (ORDER BY i ROWS BETWEEN CURRENT ROW AND UNBOUNDED
> FOLLOWING)
> Pattern: a+"
> Nav Mark Lookback: 0
> -> Sort
> Sort Key: i
> -> Function Scan on generate_series s
> (7 rows)
>

Thank you for pointing this out. I will analyse A1 and reflect
the findings in the next revision.

> Comment to 0009:
>
> + /* Mark VAR in visited before duplicate check to prevent DFS loops
> */
> + winstate->nfaVisitedElems[WORDNUM(state->elemIdx)] |=
> + ((bitmapword) 1 << BITNUM(state->elemIdx));
>
> Why do not use bms here? Maybe winstate->nfaVisitedElems could become
> quite big?
>

Thank you for the suggestion. I was not aware of Bitmapset when
this was written; I will switch nfaVisitedElems to Bitmapset.

On sizing, nfa->nelems is structurally bounded by RPR_ELEMIDX_MAX
(INT16_MAX), since RPRNFAState.elemIdx is int16 and scanRPRPattern()
rejects anything past that with "pattern too complex". So the
bitmap is at most 4 KB per WindowAgg even in the worst case.

> 0010-0016 look good to me.
>

Confirmed.

> Comment to 0017:
> +
> +/*
> + * check_rpr_nav_expr
> + * Validate a single RPRNavExpr node by walking its arg and
> offset_arg
> + * subtrees in a single pass each. Checks for nested
> PREV/NEXT, missing
> + * column references, and non-constant offset expressions.
> + */
>
> "Checks for" --> "Check for" (no "s").
>

Thank you, will fix.

> + ereport(ERROR,
> + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> + errmsg("PREV and NEXT cannot be nested"),
> + parser_errposition(pstate,
> nav->location)));
>
> errmsg("PREV and NEXT cannot be nested") ->
> errmsg("cannot nest PREV and NEXT") (avoid passive voice in error
> messages)
>
> + ereport(ERROR,
> + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> + errmsg("next() can only be used in a DEFINE
> clause")));
>
> errmsg("next() can only be used in a DEFINE clause") ->
> errmsg("cannot use next() other than in a DEFINE clause")
>
> The same reason above. There are some places where similar changing
> necessary.
>

Thank you for the guidance. I will fix the two messages called out
here and also look over the surrounding RPR errmsg() strings for
similar passive-voice phrasings, and rewrite those in the active
voice consistent with the PostgreSQL message style guide.

> + if (offset > maxOffset)
> + maxOffset = offset;
>
> + if (offset > *maxOffset)
> + *maxOffset = offset;
>
> You can use Max macro here.
>

Thank you, will switch both sites to Max().

0018-0023 look good to me.
>

Confirmed.

> Comment to 0024:
>
> + <function>first</function> ( <parameter>value</parameter>
> <type>anyelement</type> [, <parameter>offset</parameter>
> <type>bigint</type> ] )
> + <returnvalue>anyelement</returnvalue>
> + </para>
> + <para>
> + Returns the column value at the row <parameter>offset</parameter>
> + rows after the match start row;
> + returns NULL if the target row is beyond the current row.
> + <parameter>offset</parameter> defaults to 0 if omitted, referring
> to the
> + match start row itself.
> + <parameter>offset</parameter> must be a non-negative integer.
> + <parameter>offset</parameter> must not be NULL.
> + Can only be used in a <literal>DEFINE</literal> clause.
> + </para></entry>
>
> "Returns the column value at the row" is not appropriate because
> first() accepts any expression as the first argument. e.g. first(col1
> + col2). Instead what about "Returns value evaluated at the row"?
> Same thing can be said to other row pattern navigation operations.
> I think they inherits my mistakes in my earlier patch. Sorry for this.
>

Thank you, and please do not apologise -- I should have caught this
when I carried the wording across. I will reword the synopses for
first(), last(), prev(), next() (and any other entry that inherited
the same phrasing) along the line you suggested.

> --- a/src/test/regress/expected/rpr.out
> +++ b/src/test/regress/expected/rpr.out
>
> +-- match_start=3(30): A=id3, B=id4, FIRST(val)=30≠10 → no match
>
> Non ASCII letter(≠) should not be used.
>

Thank you, will fix and sweep rpr*.out for any other non-ASCII
characters (the → on the same line is also non-ASCII).

> 0025-0031 look good to me.
>

Confirmed.

> > On volatile functions in DEFINE (your 2026-04-13 mail): the SQL
> > standard itself is relatively narrow here -- the only explicit
> > restriction it places is that a navigation function's offset must
> > be a runtime constant. Volatile expressions in the DEFINE
> > predicate are not forbidden by the standard.
> >
> > That said, once we move into the pattern-matching machinery, the
> > observable semantics of a volatile DEFINE become quite murky.
> > Depending on the situation, the DEFINE predicate may be evaluated
> > once per row, or separately per active matching context, or some
> > combination of both as the NFA explores alternatives. A user
> > writing "random() > 0.5" in DEFINE would have a hard time
> > predicting, let alone relying on, the evaluation count -- and I
> > have not been able to think of a realistic use case that actually
> > benefits from this flexibility.
>
> Agreed. The flexibility does not bring benefits to users.
>
> > Given the gap between "permitted by the standard" and "meaningful
> > to the user," my own leaning is toward prohibiting volatile
> > functions in DEFINE at transformation time, and -- if we go that
> > way -- also examining whether the new check would make the
> > existing runtime-constant check on the navigation-function offset
> > redundant, or whether the two could be folded into a single check
> > pass. That said, this is ultimately a policy call, and I would rather
> defer
> > the final decision to your judgment as a senior member of the
> > community: does prohibition seem like the right direction, or
> > would you prefer to keep the current behaviour in place?
>
> Let's prohibit volatile functions in DEFINE. Although this needs extra
> checking, it would make proceeding codes and tests simpler. If
> others, especially Vik and Jacob, think differently, please join the
> discussion.
>

Thank you for the decision. Since this is a policy change that may
still attract further community discussion, I would like to handle
it as a lower-priority, self-contained follow-up on top of 0031, so
it can be picked up, held, or revised independently. Tentative
plan: reject volatile functions in DEFINE during parse-analysis via
contain_volatile_functions(), fold with the existing offset
runtime-constant check where the two share traversal, and -- if the
prohibition lands -- convert the B9 test into an error-case test
and drop the XXX note.

> > On sequencing, if we do take it up, I would suggest handling it
> > after the 31-patch set, alongside the README.rpr split as
> > follow-up work on top of 0031. Whether it ultimately lands
> > inside v47 or as a separate piece on top does not need to be
> > decided right now -- there is still room to discuss it as the
> > review progresses, and I am happy to adjust either way based on
> > your direction.
>
> I too prefer to have it after the 31-patch set.
>

Thank you, confirmed. Both items (volatile-DEFINE prohibition and
README.rpr split) will land as follow-up patches on top of 0031,
after the current series is committed.

One quick question: for the 0007 / 0009 / 0017 / 0024 review fixes
summarised below, would additional follow-up patches on top of 0031
be preferable to refreshing the already-reviewed series?

For ease of tracking, here is a one-line summary per patch of the
changes I plan to fold into the next revision:

0007: analyse A1 and, if the diagnosis holds, revise it so the
frame optimization bypass is observable in EXPLAIN
(adjusting the comment and expected output accordingly).
0009: switch WindowAggState.nfaVisitedElems from a bitmapword[]
to Bitmapset, replacing the manual WORDNUM/BITNUM sites
with bms_add_member() / bms_is_member() / bms_free().
0017: fix the "Checks for" -> "Check for" comment, rewrite the
called-out passive-voice errmsg() strings (and the similar
ones nearby) in the active voice, and use Max() at the two
offset-update sites.
0024: reword the navigation synopses (first/last/prev/next) to
"Returns the value of <value> evaluated at the row ...",
and replace the non-ASCII ≠ / → in rpr.out with != / ->
(sweeping the rest of rpr*.out for other non-ASCII).

In addition, I plan to follow the current series with two new
patches on top of 0031 (final numbering deferred until they are
posted):

new: README.rpr split -- extract the design notes currently
inlined in execRPR.c into src/backend/executor/README.rpr,
as previously discussed.
new: Prohibit volatile functions in DEFINE -- separate,
self-contained patch as described above; intended as a
lower-priority follow-up that can be picked up, held, or
revised independently of the main series depending on how
the community discussion settles.

Separately, one item is deferred for later discussion rather than
folded into a patch:

deferred: B7 Recursive CTE XXX in 0007 -- whether this case
falls under ISO/IEC 9075-2 4.18.5 / 6.17.5 prohibition
("Ok, let's discuss on this later on."). Noted, happy
to pick this up whenever convenient.

Once you let me know your preference on the sequencing question
above, I will send the corresponding patches as soon as they are
ready, and post the two follow-ups separately so that they can be
reviewed and committed on top of the main series.

Thank you again for the thorough review.

Regards,
Henson