Re: Row pattern recognition

Hi Tatsuo, Jian,

Two problems, and they came up together: the second is what let me
write the second reproduction of the first.

1. A DEFINE clause can take the backend down. Patch attached.
2. A DEFINE can name what looks like a volatile function, when the
same expression is in GROUP BY. I read that as the existing
conventions working rather than as a hole, but I would like to
hear if anyone sees it differently.

Setup for everything below:

CREATE TABLE t (id int, val int);
INSERT INTO t VALUES (1, 10), (2, 20), (3, 15), (4, 30), (5, 5);

1. The crash

A navigation offset spelled the same as a window ORDER BY key:

SELECT id, val, count(*) OVER w AS cnt
FROM t
WINDOW w AS (
ORDER BY (extract(hour from localtimestamp)::int * 0 + 1), id
ROWS BETWEEN CURRENT ROW AND UNBOUNDED FOLLOWING
PATTERN (A B+)
DEFINE B AS val >
PREV(val, (extract(hour from localtimestamp)::int * 0 + 1)));
server closed the connection unexpectedly

And spelled the same as a GROUP BY expression:

SELECT id, val, count(*) OVER w AS cnt
FROM t
GROUP BY GROUPING SETS ((id, val, ((random() * 0)::bigint + 1)),
(id, ((random() * 0)::bigint + 1)))
WINDOW w AS (
ORDER BY id ROWS BETWEEN CURRENT ROW AND UNBOUNDED FOLLOWING
PATTERN (A B+)
DEFINE B AS val > PREV(val, (random() * 0)::bigint + 1))
ORDER BY id, val;
server closed the connection unexpectedly

On a cassert + ASan build both abort with signal 6, frame for frame
the same:

#0 CheckVarSlotCompatibility execExprInterp.c:2431
#1 CheckExprStillValid
#2 ExecInterpExprStillValid execExprInterp.c:2325
#3 ExecEvalExprSwitchContext executor.h:452
#4 eval_nav_offset nodeWindowAgg.c:4092
#5 resolve_one_nav nodeWindowAgg.c:4291
#6 resolve_nav_offsets nodeWindowAgg.c:4422
#7 ExecWindowAgg nodeWindowAgg.c:2473

That line reads slot->tts_tupleDescriptor. The slot is null: UBSan
calls it a member access within a null pointer, and ASan reports the
read at 0x10, which is that member's offset. Nothing is corrupted --
the slot simply is not set yet.

set_upper_references() hands the whole DEFINE expression to
fix_upper_expr(), which matches any subexpression against the
subplan's targetlist. An offset spelled the same as something already
in the window input is therefore replaced with a Var(OUTER_VAR)
referencing it, and the compiled expression gets an EEOP_OUTER_VAR
step. resolve_nav_offsets() then evaluates the offsets at the top of
ExecWindowAgg(), before the partition is opened and before
ecxt_outertuple has been pointed at anything.

The substitution does not get the value wrong -- the window input
holds the result of that same expression -- it gets the timing wrong.
An offset is resolved once per scan; a Var is a per-row read.

Two things have to line up, and it is worth saying which:

- the offset must be equal() to something in the window input, which
is why both reproductions spell it twice. Change one of the two and
the query runs;
- it must not be a Const, because
search_indexed_tlist_for_non_var() declines to match one
("replacing it with a Var is silly"). That is why the offset has to
survive constant folding.

The "* 0" is not part of either condition. It only pins the value so
the regression output does not move; without it the crash is the same.

The fix. Only the navigated argument is read from the input, one row
at a time; the offsets are run-time constants. set_plan_refs()
already draws that line for the frame offsets of the same node, a few
lines above the DEFINE block:

* Like Limit node limit/offset expressions, WindowAgg has
* frame offset expressions, which cannot contain subplan
* variable refs, so fix_scan_expr works for them.

So RPRNavExpr gets its own case in fix_upper_expr_mutator(): arg
recurses as before, the two offsets go to fix_scan_expr(). That is
the whole change, 22 lines in setrefs.c.

Both reproductions and a control are now in rpr_base. With the case
taken back out they abort again, so they do hold the fix in place.
All five RPR suites pass, with no sanitizer output.

2. Why the second reproduction is accepted at all

DEFINE rejects volatile functions:

SELECT id, count(*) OVER w AS cnt
FROM t
WINDOW w AS (
ORDER BY id ROWS BETWEEN CURRENT ROW AND UNBOUNDED FOLLOWING
PATTERN (A+) DEFINE A AS val > (random() * 0)::int);
ERROR: DEFINE clause cannot contain volatile functions

Name the same expression in GROUP BY and the same DEFINE is accepted:

SELECT id, count(*) OVER w AS cnt
FROM t
GROUP BY id, val, ((random() * 0)::int)
WINDOW w AS (
ORDER BY id ROWS BETWEEN CURRENT ROW AND UNBOUNDED FOLLOWING
PATTERN (A+) DEFINE A AS val > (random() * 0)::int);

The check is contain_volatile_functions() on wc->defineClause in
subquery_planner(), immediately after the clause is preprocessed. At
that point a DEFINE expression that matched a GROUP BY expression is a
GROUP RTE Var, so there is no volatile function to find.
flatten_group_exprs() expands those Vars back into the grouping
expressions further down, after the check has run.

I think that is the convention rather than a gap, on two counts. The
comment on the check states the first itself: volatility is examined
in the planner and not while parsing, and "a subquery the planner
discards before reaching this point is therefore not checked, which is
the same rule that lets a volatile fold away". The planner does not
go back and re-ask about what it has rearranged.

The second is what actually runs. In the accepted query above the
DEFINE condition reaches the executor as

OPEXPR(>) { Var(OUTER_VAR, 2), Var(OUTER_VAR, 3) }

That is val, and the grouped column. No volatile function is called
during matching: random() was evaluated once per row by the grouping,
and DEFINE reads the result the way it reads any other column. If the
grouping expression is genuinely random, DEFINE sees a random value,
but it sees the one the group already has.

So I have left it alone. If either of you reads the restriction as
covering this case, say so and I will write the extra check after the
flatten_group_exprs() loop.

Best regards,
Henson