Re: Row pattern recognition

Hi Tatsuo, Jian,

> The fix is to not do that split for a DEFINE clause. I added
> EXPRKIND_RPR_DEFINE for the defineClause preprocessing pass, and had that
> path call eval_const_expressions_keep_row_nulltest() in place of
> eval_const_expressions().

I am withdrawing that. EXPRKIND_RPR_DEFINE and
eval_const_expressions_keep_row_nulltest() are both gone, and
clauses.c and optimizer.h are files this series does not touch again.

> My reading is that a DEFINE clause has no reason to be split in the first
> place: it never feeds an index lookup, and is evaluated per row inside
> the pattern matcher. I'd appreciate a check on whether that holds.

That was the wrong question. It is not whether a DEFINE clause may be
split, but who reads the result after it has been.

I had been looking at the target list from the DEFINE side. The
picture was that DEFINE puts what it reads into the target list, so I
wanted the shape settled by the time it went in, and turning off
constant folding was how I settled it. Turn it around and look from
the target list side, and DEFINE is one of several things that live
outside the target list and are evaluated by an upper planner node.
The core already has one of those. It is HAVING.

havingQual is boolean too, lives outside the target list too, and is
evaluated by an upper planner node, the Agg. The core solves it with
two sites and no more:

build_base_rel_tlists() -- mark the columns needed so they reach
the top of the join tree (initsplan.c)
make_group_input_target() -- ask for them again in that node's own
input target (planner.c)

And havingQual is never put in the target list at all.

DEFINE now goes through the same two sites. The DEFINE block sits
right below the HAVING block in build_base_rel_tlists(), and
make_window_input_target() asks for what the clause reads. The latter
runs after every rewrite, so what it reads is the shape the clause
will be executed with: split if it was split, whole if it stayed
whole. The reason to prevent the split is gone.

There is one rule HAVING does not need. The walk stops at an
expression the input target already computes whole; asking for the
Vars underneath would ask the grouping step for columns it cannot
produce. make_group_input_target() builds the input of the grouping
step and has no such problem, while the window input target sits above
it.

That left the parse-time planting of the Vars a DEFINE reads with
nothing to do, so it is gone. Planting was there to keep
remove_unused_subquery_outputs() and remove_useless_outer_joins() from
dropping the column, and build_base_rel_tlists() does the same thing
and runs before both. Parse analysis no longer rewrites the user's
target list at all.

The visible difference is in the rpr_integration expected output:

- Output: id, count(*) OVER w, val
+ Output: id, count(*) OVER w

A column only a DEFINE clause reads used to ride on the WindowAgg's
own output and be carried to the top of the plan. It now stays on the
input. Five places lost it, and no result row changed.

Two tests are added. One is a DEFINE clause reading a compound GROUP
BY expression, which is the only place the stop rule above does any
work and was not covered. The other is a window function nested inside
a subquery output expression the upper query drops: the pass that
settles which windows are still live only looks at top-level nodes, so
it misses that one and a dead window's column is retained. That is an
over-retention rather than a wrong answer, and making the pass exact
is left to its own commit; the test records what happens today.

ExecInitWindowAgg() gets one Assert. The varId-to-list-position
invariant that buildRPRPattern() establishes and the executor consumes
had nothing checking it, and a reorder would evaluate one variable's
search condition for another -- a wrong answer with no crash and no
assert to show for it.

Patch attached.

Best regards,
Henson