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

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, Tom,

Thanks for v3. I have read through the new series and tested it with AI assistance. I agree with the direction settled in the thread: record the casts in the parse tree and leave ruleutils.c alone.

On the code: in 0002, marking a copy of the RowExpr in coerce_type() only for a same-type explicit cast that is not already marked looks right to me - the copy avoids modifying a node that may be shared, and restricting it to COERCE_EXPLICIT_CAST leaves implicit coercions alone. In 0003, marking the JOIN USING whole-row RowExpr as explicit does mean j.* prints with ::record wherever it is deparsed, not just under an operator, but I could not find a case where the new output fails to read back, and the regression suite is clean. The make_op() change also makes sense to me: it only fires for a two-argument operator whose operands take the row-comparison form, and coercing there keeps make_fn_arguments() from redoing it as an implicit cast.

What I verified, on current master with the patches applied in order:

The bug still reproduces on unpatched master in all the forms from my earlier review: silent true-to-NULL changes when views are recreated from pg_get_viewdef() output, and restore errors for the subquery, JOIN USING, and coerced whole-row cases.

With only 0001+0002 applied, the user-written cast cases are fixed - the ::record equality and <= views, the scalar-subquery case, and the generated column print their casts, round-trip with the same values, and dump/restore cleanly. The JOIN USING and =#= cases still fail at that point, as expected.

With the full v3 series, the JOIN USING view prints ROW(...)::record and round-trips, and the =#= view prints ::rowtypes_cmp_t casts on both operands and round-trips. A dump/restore of objects created after the fix completes with no errors (the same dump/restore on unpatched master reports 8 errors; with 0002 only, 4).

Thank you for adding the generated column case I confirmed it now prints the cast through pg_get_expr() and round-trips.
The whole-row comparison in the ON CONFLICT updatable-view case (Henson's case 5, rewriter-built RowExprs) prints exactly as on master; I diffed the EXPLAIN output before and after and it is identical.

Objects created before the fix still deparse the old way when the database is run under the patched server, and a dump taken before the fix restores with the same errors as before. I understand this to be the accepted trade-off behind dropping the ruleutils.c change, not a problem with v3 - noting it only because I checked it explicitly.

installcheck-world is fully green for me on the patched build: core regression 239/239, isolation 134/134, contrib 205/205 tests plus contrib isolation 20/20, test modules 71/71 plus module isolation 24/24, including the injection_points isolation specs 18/18. I watched specifically for side effects of the coerce_type() change in the form of unexpected cast, EXPLAIN, or deparse diffs elsewhere; the only expected-output changes are the ones in rowtypes.out itself.

One question: with the ruleutils.c part gone, what is intended for the back branches? A parser-side fix changes nothing for objects already stored in a back-branch database, and I could not tell from the v3 email whether backpatching is planned.

- Prashant

The new status of this patch is: Ready for Committer