The following review has been posted through the commitfest application: make installcheck-world: tested, passed Implements feature: tested, passed Spec compliant: tested, passed Documentation: not tested
Hi Henson, I read your report and the patch, and I agree with the analysis. A cast of a ROW() to record (or to a named composite type) is a no-op that leaves no trace in the analyzed tree, so the deparser prints the operands bare, and the parser reads "ROW(..) op ROW(..)" back as a row comparison and "ROW(..) op (SELECT ..)" as a row comparison sublink. The results then differ silently (true becomes NULL, because a row comparison treats NULLs differently) or the definition does not read back at all. I also confirmed the two ways a bare ROW() arises without the user writing one: a whole-row reference to a JOIN USING alias, and a whole-row reference coerced to a named composite type. Two code-level checks I made: The condition in get_oper_expr() — left operand a bare ROW(), right operand a bare ROW() or an EXPR_SUBLINK sublink — mirrors the parser's special cases in transformAExprOp() exactly, so the fix fires in precisely the cases that would be misread, and nowhere else. One-sided ROW() comparisons and explicit casts keep their current output, as the new v3 and v8 tests show. For scope, I searched the tree for the other places that print or consume RowExprs. Genuine row comparisons are stored as RowCompareExpr with already-expanded operands, so they cannot be affected; get_oper_expr() is the only place an operator's operands are printed in this form. Tom — on the layering question, I read the code you pointed at and you are right about the mechanism: coerce_type() returns the node unchanged when the target type already matches, without fixing up the RowExpr's coercion format, and get_rule_expr() would print the cast itself if row_format were COERCE_EXPLICIT_CAST. I do not have a strong view on which layer should own the fix. My testing below covers v1 as submitted; it shows the deparse output is correct, but it does not settle where the fix belongs. For what it is worth, I looked for other consumers of row_format that the mislabeling could fool: the other readers I found (for example in parse_clause.c) work on raw parse trees, where an explicit cast is still a TypeCast node, so I could not find a second observable symptom — but I may have missed one. For the hands-on part I used AI assistance. The bug was reproduced on current master (e9d0600): the record-equality view deparses as SELECT (ROW(1, NULL::integer) = ROW(1, NULL::integer)) and reads back with r = NULL instead of true; the *=, subquery, JOIN USING and =#= cases fail to recreate with "operator does not exist: integer *= integer", "subquery has too few columns", "operator does not exist: integer = record" and "operator does not exist: integer =#= integer" respectively. A full pg_dump/restore of a database containing these objects showed 8 errors and, worse, the generated column and the BEGIN ATOMIC function body you mention in the report silently changed from true to NULL. With v1 applied, the same stored objects deparse with the casts (ROW(1, NULL::integer)::record, ROW(s1.a, s1.b)::rowtypes_cmp_t), every view recreates from its pg_get_viewdef() output with identical results, and the dump/restore is clean with no value changes. This was checked in both pretty mode (pg_get_viewdef) and the non-pretty form pg_dump emits. The one execution error I saw — selecting the different-width column of the *= view, "cannot compare record types with different numbers of columns" — occurs identically on the original view, so it is pre-existing executor behaviour, not a deparse effect. make installcheck-world was run against an installed build of master plus v1 (with plperl, uuid-ossp and injection points enabled): core regression 239/239, isolation 134/134, contrib 51/51 suites, test modules 40/40 suites, and the injection_points isolation specs 18/18. One coverage suggestion: the report names generated columns and BEGIN ATOMIC function bodies as silently corrupted stored expressions, but the tests exercise views only. I verified those two cases by hand, as described above, and they break and fix exactly as the report says; a round-trip test for at least the generated column would strengthen the suite, since it reaches the same code through pg_get_expr rather than pg_get_viewdef. On backpatching to 15, I have no objection: as with the IS DOCUMENT fix, the current output in stable branches is unrestorable or silently wrong, which seems to justify the deparse change there, and I note the updatable_views expected-output change is dropped on 15–18 as you describe. No documentation changes are needed for this fix. Thanks for the clear report and the round-trip tests. Regards, Prashant
