Re: Row pattern recognition

On Wed, Aug 26, 2026 at 11:12 PM Henson Choi <assam258(at)gmail(dot)com> wrote:
>
> Hi jian,
>
> I applied the "resolve offset evaluation once for all again." patch
> you sent off-list. Below is what I took and what I left, keyed to the
> numbered items in your commit message, and one defect that came out of
> it. The patch attached is that state.
>
> > 1. Remove some unnecessary structs.
> > 2. resolve_one_nav() resolves an entry's offsets ... uses each
> > offset's isnull marker to skip entries already resolved
>
> Neither is taken. The "unnecessary structs" is the validate field of
> EvalDefineOffsetsContext, and validate is what lets the resolution
> build_define_offsets() does at init, for EXPLAIN's sake, meet a null
> or negative constant offset without raising -- EXPLAIN does not
> execute. Retiring that path means rejecting such a constant while
> planning, which is the part I did not take, for the reason below.
>
> The isnull marker skip goes stale across a rescan as worded.
> build_nav_offsets() sets both markers true once at init, and
> ExecReScanWindowAgg re-arms only navResolvePending, so a
> PARAM_EXEC/PARAM_EXTERN offset resolved on the first scan is skipped
> on every later one and the stale value is reused -- along with the
> per-scan validation.
>

In rpr_explain.out, we have:
``````
-- A navigation with a negative offset cannot run, so it contributes no reach
-- and its dimension reports nothing at all.
EXPLAIN (COSTS OFF) SELECT count(*) OVER w
FROM generate_series(1,10) s(v)
WINDOW w AS (
ROWS BETWEEN CURRENT ROW AND UNBOUNDED FOLLOWING
PATTERN (A+)
DEFINE A AS PREV(FIRST(v, -3), 2) IS NOT NULL
);
QUERY PLAN
-------------------------------------------------------------------
WindowAgg
Window: w AS (ROWS BETWEEN CURRENT ROW AND UNBOUNDED FOLLOWING)
Pattern: a+
-> Function Scan on generate_series s
(4 rows)
``````

I don't think keeping the above EXPLAIN from failing justifies adding
a new field
(validate) to EvalDefineOffsetsContext. The EXPLAIN above just failing
should be fine.
Rationale:
1. This patchset is already quite complex. Adding an extra struct field and
the code paths that go with it solely for EXPLAIN can display a query that can
never execute doesn't seem worth the complication.

2. The SQL standard already says a negative navigation offset is not allowed, so
there is no reason to be lenient about it at any stage, EXPLAIN included.

There is precedent for EXPLAIN succeeding where execution fails:
EXPLAIN SELECT 1 LIMIT -1; -- ok
SELECT 1 LIMIT -1; -- ERROR
but LIMIT is a very old feature. If it were being added today, rejecting the
a negative value at an early stage should be fine, IMHO.

> > 3. Teach eval_const_expressions about RPRNavExpr.
>
> There is a defect in that case, though.
>
> So an expression carrying any of those three dies under a navigation
> where it runs one level out:
>
> DEFINE A AS PREV(s COLLATE "C") > 'a'
> ERROR: unrecognized node type: 32
> -- (s COLLATE "C") > 'a' returns rows
>

WITH t(id, v) AS (VALUES (1, 10))
SELECT count(*) OVER w AS cnt
FROM t
WINDOW w AS (ROWS BETWEEN CURRENT ROW AND UNBOUNDED FOLLOWING PATTERN
(A) DEFINE A AS PREV(v / 0) > 0);

Maybe we can just let such cases error out. Parser guarantees that PREV() and
friends contain at least one column reference, here ``WITH t(id, v) AS (VALUES
(1, 10))`` is pulled up, turning PREV(v / 0) into PREV(10 / 0), which fails in
eval_const_expressions_mutator. Hitting this requires deliberately building a
one row table that also inlines to a constant, which is a corner case.

The comments in preprocess_expression says we shouldn't selectively skip
constant folding for particular node fields, and the CASE documentation [1]
explicitly notes that a constant 1/0 can fail at planning time even in an arm
that would never be entered at run time.

[1] https://www.postgresql.org/docs/current/functions-conditional.html#FUNCTIONS-CASE

--
jian
https://www.enterprisedb.com/