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
diff --git a/src/backend/parser/parse_expr.c b/src/backend/parser/parse_expr.c
index 24099beb0d0..4f931ae1f55 100644
--- a/src/backend/parser/parse_expr.c
+++ b/src/backend/parser/parse_expr.c
@@ -72,7 +72,7 @@ static Node *transformXmlSerialize(ParseState *pstate,
XmlSerialize *xs);
static Node *transformBooleanTest(ParseState *pstate, BooleanTest *b);
static Node *transformCurrentOfExpr(ParseState *pstate, CurrentOfExpr *cexpr);
static Node *transformColumnRef(ParseState *pstate, ColumnRef *cref);
-static Node *transformWholeRowRef(ParseState *pstate, bool for_func_call,
+static Node *transformWholeRowRef(ParseState *pstate,
ParseNamespaceItem *nsitem,
int
sublevels_up, int location);
static Node *transformIndirection(ParseState *pstate, A_Indirection *ind);
@@ -656,6 +656,18 @@ transformColumnRef(ParseState *pstate, ColumnRef *cref)
NameListToString(cref->fields)),
parser_errposition(pstate,
cref->location));
}
+
+ /*
+ * A whole-row reference is barred by its form alone: no
qualifier
+ * makes one legal here, so resolving it first would only
choose which
+ * rejection it gets.
+ */
+ if (IsA(llast(cref->fields), A_Star))
+ ereport(ERROR,
+ errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("whole-row reference is not
allowed in DEFINE clause"),
+ errhint("A DEFINE condition may
reference individual columns only."),
+ parser_errposition(pstate,
cref->location));
}
/*----------
@@ -709,8 +721,22 @@ transformColumnRef(ParseState *pstate, ColumnRef *cref)
cref->location,
&levels_up);
if (nsitem)
- node =
transformWholeRowRef(pstate, false, nsitem,
-
levels_up, cref->location);
+ {
+ /*
+ * A lone name reaches here
only as a whole-row
+ * reference, so the rule above
catches up with it,
+ * before
transformWholeRowRef() marks anything.
+ */
+ if (pstate->p_expr_kind ==
EXPR_KIND_RPR_DEFINE)
+ ereport(ERROR,
+
errcode(ERRCODE_SYNTAX_ERROR),
+
errmsg("whole-row reference is not allowed in DEFINE clause"),
+
errhint("A DEFINE condition may reference individual columns only."),
+
parser_errposition(pstate, cref->location));
+
+ node =
transformWholeRowRef(pstate, nsitem, levels_up,
+
cref->location);
+ }
}
break;
}
@@ -734,8 +760,8 @@ transformColumnRef(ParseState *pstate, ColumnRef *cref)
/* Whole-row reference? */
if (IsA(field2, A_Star))
{
- node = transformWholeRowRef(pstate,
false, nsitem,
-
levels_up, cref->location);
+ node = transformWholeRowRef(pstate,
nsitem, levels_up,
+
cref->location);
break;
}
@@ -747,8 +773,8 @@ transformColumnRef(ParseState *pstate, ColumnRef *cref)
if (node == NULL)
{
/* Try it as a function call on the
whole row */
- node = transformWholeRowRef(pstate,
true, nsitem,
-
levels_up, cref->location);
+ node = transformWholeRowRef(pstate,
nsitem, levels_up,
+
cref->location);
node = ParseFuncOrColumn(pstate,
list_make1(makeString(colname)),
list_make1(node),
@@ -781,8 +807,8 @@ transformColumnRef(ParseState *pstate, ColumnRef *cref)
/* Whole-row reference? */
if (IsA(field3, A_Star))
{
- node = transformWholeRowRef(pstate,
false, nsitem,
-
levels_up, cref->location);
+ node = transformWholeRowRef(pstate,
nsitem, levels_up,
+
cref->location);
break;
}
@@ -794,8 +820,8 @@ transformColumnRef(ParseState *pstate, ColumnRef *cref)
if (node == NULL)
{
/* Try it as a function call on the
whole row */
- node = transformWholeRowRef(pstate,
true, nsitem,
-
levels_up, cref->location);
+ node = transformWholeRowRef(pstate,
nsitem, levels_up,
+
cref->location);
node = ParseFuncOrColumn(pstate,
list_make1(makeString(colname)),
list_make1(node),
@@ -840,8 +866,8 @@ transformColumnRef(ParseState *pstate, ColumnRef *cref)
/* Whole-row reference? */
if (IsA(field4, A_Star))
{
- node = transformWholeRowRef(pstate,
false, nsitem,
-
levels_up, cref->location);
+ node = transformWholeRowRef(pstate,
nsitem, levels_up,
+
cref->location);
break;
}
@@ -853,8 +879,8 @@ transformColumnRef(ParseState *pstate, ColumnRef *cref)
if (node == NULL)
{
/* Try it as a function call on the
whole row */
- node = transformWholeRowRef(pstate,
true, nsitem,
-
levels_up, cref->location);
+ node = transformWholeRowRef(pstate,
nsitem, levels_up,
+
cref->location);
node = ParseFuncOrColumn(pstate,
list_make1(makeString(colname)),
list_make1(node),
@@ -905,6 +931,23 @@ transformColumnRef(ParseState *pstate, ColumnRef *cref)
errorMissingColumn(pstate, relname, colname,
cref->location);
break;
case CRERR_NO_RTE:
+
+ /*
+ * ISO/IEC 19075-5 6.5 reserves the qualifier
slot in a DEFINE
+ * clause for a row pattern variable, so a
qualifier naming
+ * nothing is rejected for occupying it, the
same as one that
+ * names something. Reporting a missing
FROM-clause entry
+ * would point at a repair that does not exist:
adding the
+ * relation only moves the reference to the
range variable
+ * rejection below.
+ */
+ if (pstate->p_expr_kind == EXPR_KIND_RPR_DEFINE)
+ ereport(ERROR,
+
errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("qualified
expression \"%s\" is not allowed in DEFINE clause",
+
NameListToString(cref->fields)),
+
parser_errposition(pstate, cref->location));
+
errorMissingRTE(pstate, makeRangeVar(nspname,
relname,
cref->location));
break;
@@ -2792,29 +2835,11 @@ transformCurrentOfExpr(ParseState *pstate,
CurrentOfExpr *cexpr)
/*
* Construct a whole-row reference to represent the notation "relation.*".
- *
- * for_func_call is true when transformColumnRef is building the reference
- * speculatively, to retry a name that did not resolve as a column as a
- * function call on the composite value. The query does not contain a
- * whole-row reference in that case, so restrictions on writing one must not
- * fire; whatever the retry resolves to is diagnosed by the caller.
*/
static Node *
-transformWholeRowRef(ParseState *pstate, bool for_func_call,
- ParseNamespaceItem *nsitem, int
sublevels_up,
- int location)
+transformWholeRowRef(ParseState *pstate, ParseNamespaceItem *nsitem,
+ int sublevels_up, int location)
{
- /*
- * A DEFINE clause cannot use a whole-row reference: ISO/IEC 19075-5 6.5
- * limits the range variables in scope to the row pattern variables.
- */
- if (pstate->p_expr_kind == EXPR_KIND_RPR_DEFINE && !for_func_call)
- ereport(ERROR,
- errcode(ERRCODE_SYNTAX_ERROR),
- errmsg("whole-row reference is not allowed in
DEFINE clause"),
- errhint("A DEFINE condition may reference
individual columns only."),
- parser_errposition(pstate, location));
-
/*
* Build the appropriate referencing node. Normally this can be a
* whole-row Var, but if the nsitem is a JOIN USING alias then it
contains
diff --git a/src/test/regress/expected/rpr.out
b/src/test/regress/expected/rpr.out
index 59b6677b0cf..a5a573400ad 100644
--- a/src/test/regress/expected/rpr.out
+++ b/src/test/regress/expected/rpr.out
@@ -1658,6 +1658,34 @@ ERROR: whole-row reference is not allowed in DEFINE
clause
LINE 7: DEFINE A AS (stock.*) IS NOT NULL
^
HINT: A DEFINE condition may reference individual columns only.
+-- The rule applies to a whole-row reference the query actually has, so it
+-- waits for the qualifier to resolve. A misspelled table name is still
+-- reported as one, with the hint normal resolution offers:
+SELECT price FROM stock
+WINDOW w AS (
+ PARTITION BY company
+ ROWS BETWEEN CURRENT ROW AND UNBOUNDED FOLLOWING
+ INITIAL
+ PATTERN (A)
+ DEFINE A AS (stok.*) IS NOT NULL
+);
+ERROR: whole-row reference is not allowed in DEFINE clause
+LINE 7: DEFINE A AS (stok.*) IS NOT NULL
+ ^
+HINT: A DEFINE condition may reference individual columns only.
+-- and the same through a row constructor:
+SELECT price FROM stock
+WINDOW w AS (
+ PARTITION BY company
+ ROWS BETWEEN CURRENT ROW AND UNBOUNDED FOLLOWING
+ INITIAL
+ PATTERN (A)
+ DEFINE A AS ROW(stok.*) IS NOT NULL
+);
+ERROR: whole-row reference is not allowed in DEFINE clause
+LINE 7: DEFINE A AS ROW(stok.*) IS NOT NULL
+ ^
+HINT: A DEFINE condition may reference individual columns only.
-- A row constructor reaches the same references through
-- transformExpressionList(), which expanded the star by RTE before either
-- check could see it. The first four below were accepted and returned rows;
diff --git a/src/test/regress/expected/rpr_base.out
b/src/test/regress/expected/rpr_base.out
index 37835eb7924..e6ac2ec3348 100644
--- a/src/test/regress/expected/rpr_base.out
+++ b/src/test/regress/expected/rpr_base.out
@@ -5113,7 +5113,7 @@ WINDOW w AS (
PATTERN (A+)
DEFINE A AS nosuch.val > 0
);
-ERROR: missing FROM-clause entry for table "nosuch"
+ERROR: qualified expression "nosuch.val" is not allowed in DEFINE clause
LINE 7: DEFINE A AS nosuch.val > 0
^
-- Unqualified composite field access in DEFINE works: no qualifier means no
diff --git a/src/test/regress/sql/rpr.sql b/src/test/regress/sql/rpr.sql
index 5b48d1e7dbc..6b6350c762d 100644
--- a/src/test/regress/sql/rpr.sql
+++ b/src/test/regress/sql/rpr.sql
@@ -960,6 +960,26 @@ WINDOW w AS (
PATTERN (A)
DEFINE A AS (stock.*) IS NOT NULL
);
+-- The rule applies to a whole-row reference the query actually has, so it
+-- waits for the qualifier to resolve. A misspelled table name is still
+-- reported as one, with the hint normal resolution offers:
+SELECT price FROM stock
+WINDOW w AS (
+ PARTITION BY company
+ ROWS BETWEEN CURRENT ROW AND UNBOUNDED FOLLOWING
+ INITIAL
+ PATTERN (A)
+ DEFINE A AS (stok.*) IS NOT NULL
+);
+-- and the same through a row constructor:
+SELECT price FROM stock
+WINDOW w AS (
+ PARTITION BY company
+ ROWS BETWEEN CURRENT ROW AND UNBOUNDED FOLLOWING
+ INITIAL
+ PATTERN (A)
+ DEFINE A AS ROW(stok.*) IS NOT NULL
+);
-- A row constructor reaches the same references through
-- transformExpressionList(), which expanded the star by RTE before either
