Re: Row pattern recognition

Hi Jian,

Taking these in order.

> The above change looks good to me.
> No comments should be necessary for ResetExprContext or
> ExecEvalExprSwitchContext.
> This is because they are common routines.

What is common here is the two routines, not the context or the point
it is reset at. rprContext is a third ExprContext that
ExecInitWindowAgg() creates only for a window carrying a DEFINE clause,
and that line is the only reset of it in the tree.

Nor is it the usual once-per-tuple reset. For a single output row the
NFA scans forward over the rows of the frame, evaluates a DEFINE
predicate per row and per pattern variable, and inside that predicate
the navigation operators move to further rows and evaluate there again.
The reset runs at the innermost of those, many times over for one row.
That is unusual enough that the invariant it rests on is worth writing
down: a predicate leaves nothing behind but the RPRVarMatch, and the
pass-by-ref results the navigation steps stabilize in that context are
consumed before it returns.

And the case for marking it is that patch itself. One consumer, and it
still went wrong once -- that patch is the fix. That makes the comment
more warranted, not less.

So I would keep it as it stands.

> 2002 can be merged into 2019.

I would rather leave these apart, mostly because 2019 is yours. You
are the Author on it and I am only the committer; folding it into 2002
would put your work under my name. And it buys nothing: the increment
is folded into per-subsystem patches when a version is assembled, so in
v53 the two land in the same patch either way.

> For wholerow rejection, one or two test queries should be enough,
otherwise
> people may find it overwhelming and hard to review.

Let me put the denominator straight first. That patch carries two
rules, not one -- whole-row references and qualified names -- and the
tests in rpr.sql stand behind eight distinct diagnostics over 28
rejections. The whole-row message is 10 of them. Five more are
controls: an ordinary typo like stock.pric has to keep coming back as
"column does not exist" and not as one of the new rejections.

The 10 are not one case written ten times. They are the cells of a
matrix over two axes -- how the reference is spelled, that is which
form a user can actually write, and what the qualifier names, which
decides which rule has to catch it.

RPR rel outer join alias pattern var
t x
t.* x x x
schema.t.* x
ROW(t.*) x *
ROW(schema.t.*) x
(t.*, 1) x
ROW((t.*)) x

Every x is a different spelling or a different referent, so no two of
them are the same query. The * is ROW(A.*), and it is a separate
criterion rather than a repeat: a whole-row reference to a pattern
variable is caught by the pattern-variable rule first, and that cell is
the only place the precedence between the two rules is pinned. One
more, the ROW(t.*) inside a CREATE VIEW, is there so the deparser
section can say that no view ever carries such a reference that far.

That is black-box coverage over the two axes. What fixes the coverage
is the spelling, not whichever code happens to serve it today. That
ROW(t.*), (t.*, 1) and ROW((t.*)) converge on one arm right now is a
fact about the current implementation, not a guarantee about the next
one, and catching the day it stops holding is what a regression test is
for.

White-box measurement of this area has not been done. If gcov later
shows that one of these spellings leaves a coverage gap under some
condition the others do not reach, the tests get added to, not taken
away.

> transformSubLink. too much comments for (case EXPR_KIND_RPR_DEFINE).
> The following should suffice:

I read it again with your comment in hand and I cannot find a sentence
to drop. The three paragraphs each guard a different misreading: what
the standard actually allows, that rejecting all of them is deliberate
rather than blocked on something, and that it is the SubLink being
turned away and not the subquery. Take one out and that misreading is
available again.

(a) and (b) I would keep above all. With only "certain subqueries",
the only way to learn which ones is to obtain ISO/IEC 19075-5 and read
it. Someone who reads this comment and feels like having a go is not
going to buy the standard and track down the subclause; the odds of
that are close to zero. Spelling (a) and (b) out lets them start
without it, and that is the whole of what an XXX like this can do.

Whether XXX or FIXME is the convention in this spot I am not sure of.
I would leave that to Tatsuo to apply the right way when he merges.

> Actually, we don't need to change transformWholeRowRef at all.
> See the atached no_need_change_transformWholeRowRef.txt

I agree with the direction. You are right that for_func_call is not
needed: the speculative call has the field shape [tbl, funcname], which is
neither an A_Star nor a single field, so it misses both conditions. That
reads back from cref what the call site already knew, and it holds, and the
gain of keeping the RPR rule out of a shared function signature remains. I
applied it and checked: 27 whole-row spellings run through the same probe on
both builds produce output that does not differ by a line.

What I do not agree with is placing the check after the name resolves. When
resolution fails, node != NULL keeps the check from running at all.

DEFINE A AS (stok.*) IS NOT NULL
ERROR: missing FROM-clause entry for table "stok"

That diagnostic points at a repair that does not exist. Follow it, put
stok in the FROM clause, and the next error is a range variable qualified
expression. The qualifier slot in a DEFINE clause is the row pattern
variable's slot (ISO/IEC 19075-5 6.5), and whether that name exists as a
table has no bearing on whether it may stand there. Nor does any qualifier
make a whole-row reference legal, so resolving one first only chooses which
rejection it gets.

So I think what the form settles belongs before resolution, and only what
resolution settles belongs after it. "t.*" is finished by its spelling, so
it sits right after the pattern variable check; a lone name is
indistinguishable from a column, so it sits where the name is settled as a
range variable -- before transformWholeRowRef() marks the range table. For
the same reason a qualifier that names nothing at all is reported in the
DEFINE clause's own terms.

DEFINE A AS (stok.*) IS NOT NULL
ERROR: whole-row reference is not allowed in DEFINE clause
HINT: A DEFINE condition may reference individual columns only.

DEFINE A AS (nosuch.val > 0)
ERROR: qualified expression "nosuch.val" is not allowed in DEFINE clause

That is attached as wip-rpr-define-whole-row-check-placement.txt. The
ISO/IEC 19075-5 6.5 citation the removed block carried has moved to the new
site, and the two diagnostics above are pinned by queries in rpr.sql.

Best regards,
Henson