Re: Row pattern recognition

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.

> 3. Teach eval_const_expressions about RPRNavExpr.

This one is taken, without the four ereports it raises over a folded
Const. Those two messages are already in eval_nav_offset(), and that
site has to stay: an offset written as a bind parameter, or the
PARAM_EXEC a correlated SRF leaves behind, has no value while
planning, so the executor is the only place that sees every offset.
Both offsets in core this one resembles -- a window frame's, resolved
in calculate_frame_offsets(), and LIMIT's, in recompute_limits() --
are checked at execution as well, even where the value is written as
a literal; EXPLAIN SELECT 1 LIMIT -1 prints a plan.

There is a defect in that case, though.

Leaving the argument out stops more than folding. That pass is not
only an optimization: it is also where nodes the executor cannot
handle get rewritten away -- named arguments become positional,
omitted defaults are filled in, and COLLATE is turned into a form the
executor knows. The navigation argument used to get all of that for
free by going through the generic path. Giving the node a case of its
own and leaving the argument out takes it away.

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

DEFINE A AS PREV(f(b => 7, a => v)) > 0
ERROR: unrecognized node type: 17
-- f(b => 7, a => v) > 0 returns rows

DEFINE A AS PREV(g(v)) = 100 -- g(a int, b int DEFAULT 100)
ExecInitFunc sizes the FunctionCallInfo from the shortened
argument list and the callee reads past it. In plpgsql I got a
different b on different runs; as a SQL function it raises
"no value found for parameter 2".
-- g(v) = 100 returns rows

I would guess the way out is to let the argument through that pass
with the evaluation turned off for the navigated argument alone,
rather than leaving it out altogether. That would still rewrite away
the nodes the executor cannot handle, while leaving uncomputed the
part that could raise when there is no target row. There seems to be
no reason to turn it off for the offsets: they are resolved once per
execution whether or not a target row is ever reached, so folding
them raises nothing execution would not.

That is a guess, though, and you may well see a better way.

Could you fix this? What is attached is that state: the three cases
go into the regression with their current failures as the expected
output and an XXX saying what each should return. When you fix it,
could you move the expected output to the correct form and take the
XXX out with it?

Best regards,