Re: [DEPARSE] Operators on ROW() constructors read back as row comparisons

Hi Tom,

> Indeed. I think this is a parser bug and you're trying to band-aid
> around it in the wrong place. get_rule_expr() would already do the
> right thing if the RowExpr had row_format == COERCE_EXPLICIT_CAST,
> but it does not, which seems like it could fool more things than
> only deparsing.

Recording the cast in the parser fixes the case you describe, and
v2-0002 does that. It cannot fix trees that are already stored, though.
I started a server with only that patch on a data directory made before
it. A view created there with

row(1, null::int)::record = row(1, null::int)::record

still printed without the casts, and after pg_dump and restore it
returned NULL instead of true. Its original text is gone, so it cannot
be fixed by creating it again. So the ruleutils.c change from v1 is
still needed, and it is the only part that helps the back branches.
With A, B and C as described below, I would propose:

Back branches (15 and later): C only, that is the tests and 0004, with
the expected output adjusted for each branch. The parser changes
would only change how new objects are printed there.

Master: A + C, that is 0001, 0002 and 0004. The parser keeps the cast
the user wrote, and C covers the rest. B adds nothing that C does not
already cover, and has to guess in make_op() what C sees directly, so
I would leave 0003 out.

I attached A + B + C so that each part can be seen on its own; dropping
0003 gives the master proposal.

There is a trade-off depending on where the fix goes. I tried these
ways:

A Parser, casts the user wrote: coerce_type() marks a copy of the
RowExpr as COERCE_EXPLICIT_CAST when asked for an explicit cast of
a RowExpr to its own type. (v2-0002)

B Parser, ROW()s the parser makes itself: mark the ROW() for a
whole-row reference to a JOIN USING alias as an explicit cast, and
have make_op() pass COERCE_EXPLICIT_CAST when it coerces a
whole-row reference of type record to an operator's composite
input type and the operands would take the form that
transformAExprOp reads as a row comparison. (v2-0003)

B' A shorter form of B: keep the JOIN USING part, but instead of
the make_op() change, have coerce_record_to_complex() in
src/backend/parser/parse_coerce.c always mark the ROW() it
builds as an explicit cast, ignoring the cformat it is given.
(not attached)

@@ -1133,2 +1133,2 @@ coerce_record_to_complex(ParseState *pstate,
rowexpr->row_typeid = baseTypeId;
- rowexpr->row_format = cformat;
+ rowexpr->row_format = COERCE_EXPLICIT_CAST;

C ruleutils.c: when the left operand of an operator is a ROW()
not marked as a cast and the right one is a ROW() or a
scalar subquery, print a cast after each such ROW().
(v1-0002, now v2-0004)

against these cases:

1 a ROW() cast to record by the user, as in the view above
2 a whole-row reference to a JOIN USING alias
3 a whole-row reference coerced to an operator's composite type
4 objects stored before the fix, as above
5 ROW()s made by the rewriter, as for whole-row references to an
updatable view in EXPLAIN; nothing stored changes

1 2 3 4 5 other output changed
A o - - - - none
B - o o - - j.* prints ::record wherever used
B' - o o - - every record-to-composite coercion,
e.g. function arguments and defaults
C o o o o o none

Whether a ROW() needs a cast depends on its parent and its sibling: it
does only as the left operand of an operator whose right operand is a
ROW() or a scalar subquery. For 1 the cast is the user's own, so A can
record it on the node itself. For 2 and 3 the parser builds the ROW()
without seeing either; it sees them only in make_op(), before the
operands are coerced, so B has to tell from their types what they will
become, and B' marks every such ROW() instead. ruleutils.c sees the
operator and both operands complete, so C needs no such guess.

In short, A keeps a cast that must not be dropped when the conversion
is simplified, while B and C add one the user did not write. B adds
it when parsing, to the ROW()s the transformation makes: in make_op()
only where it is needed, and always for a JOIN USING alias. C adds it
when printing, whatever made the tree, so it also covers trees parsed
before the code for A and B came in (4) and ROW()s the rewriter makes
after parsing (5).

Since only C reaches 4 and 5, the ways combine like this:

C alone (v1)
+ smallest change; the only one that helps the back branches
- the tree still loses the cast the user wrote
A alone (what you suggested)
+ fixes the parser bug
- 2 to 5 stay broken
A + C
+ parser records what the user wrote; C covers the rest
- C decides from the shape of the output
A + B + C (attached v2)
+ the tree also records 2 and 3 when they are parsed
- make_op() has to guess the operands before coercing them;
C is still needed
A + B' + C
+ B' is a single line
- ignores the cformat it is given, and changes the output of
every record-to-composite coercion

Once the back branches are counted, none of these is clean. What
matters there is the objects that already exist, so only C helps them,
while in master the parser fix still leaves C needed for 4 and 5. C
ends up in every choice, and master fixes the same bug in more places
than the back branches, which is why I split the proposal above.

> So roughly speaking, I think
> we want something that's like hide_coercion_node() but can install
> a cformat other than COERCE_IMPLICIT_CAST, and then invoke that
> in the coerce_type() paths that don't build a new node.

A function like that sees only the node it is given, but whether a ROW()
needs a cast depends on its parent and its sibling, as above, and
coerce_type() is told neither. Putting a function that looks at them
into the existing logic would mean passing them down through the
callers, which makes the change much larger. So A only records the cast
the user wrote, in the same-type path, and leaves the rest to C.

0001 now has fewer tests than in v1: one view per case, recreated from
its pg_get_viewdef() output and compared with the original.

Best regards,
Henson