Re: [DEPARSE] AT TIME ZONE and similar as a function in FROM do not read back

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,

Thanks for the patch. I have read through the report and the code, and tested it hands-on (the build and test runs were done with AI assistance). I agree with the analysis: the four forms are not function calls in the grammar, and FROM is the only place that insists on function-call syntax, so teaching looks_like_function() about the context rather than changing what get_func_sql_syntax() prints everywhere - is the right layer.

On the code: I checked the OID list in looks_like_function() against get_func_sql_syntax() case by case. The 23 funcids are exactly the ones printed there as a parenthesized expression; everything else (EXTRACT, TRIM, SUBSTRING, POSITION, OVERLAY, COLLATION FOR, and so on) prints in KEYWORD(...) form and is correctly left returning true. The cross-pointing comments on both sides should help keep the two lists in sync. I also confirmed the only other callers are the index, statistics and partition key deparsers, which pass false, and that index/partition/statistics key output is byte-for-byte unchanged with the patch - no extra parentheses appear there.

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

The bug reproduces on unpatched master in all the reported forms: views using AT TIME ZONE, AT LOCAL, OVERLAPS or IS NORMALIZED in FROM print back in parentheses and fail to recreate with a syntax error (the report's example fails at "TIME"; the ROWS FROM form fails at "("), and a pg_dump containing them cannot be restored.
With the patch, every one of these views recreates from its pg_get_viewdef() output -- pretty and non-pretty -- with an identical definition, and a dump/restore of the same database completes with no errors.
Objects created before the fix are repaired by the patched deparser alone: reading a database created on unpatched master under the patched server prints the CAST form, and a fresh dump of it restores cleanly.
I also tried a few forms the new tests do not cover. A timetz AT TIME ZONE whose result is used as timetz (identity cast) is broken on master and fixed by the patch, so the F_TIMEZONE_*_TIMETZ entries in the list are load-bearing; time-pair and interval-endpoint OVERLAPS behave the same way, as does a nested AT TIME ZONE. Two related forms were never broken, for the record: IS NOT NORMALIZED (the NOT sits on top, so it already got the dummy CAST) and AT TIME ZONE under a real, type-changing cast.
A BEGIN ATOMIC SQL function whose body uses AT TIME ZONE in FROM also fails to read back on master and is fixed; its definition now prints the CAST form.
installcheck-world is fully green for me on the patched build: core regression 239/239, isolation 134/134, contrib 206/206 tests plus contrib isolation 20/20, test modules 71/71 plus module isolation 24/24, including the injection_points isolation specs 18/18. (The only failures I saw along the way were in my own installcheck setup - test_decoding needed max_prepared_transactions=10 on the server; with that set it passes 20/20 and 16/16.)

One suggestion: the tests added to tt20v cover the timestamp and timestamptz forms, but not the timetz form of AT TIME ZONE, which is broken in the same way and fixed by the same list. Adding that one case would pin the timetz entries of the list against regressions.

I also agree with backpatching to 14, with the AT LOCAL cases dropped for 14–16 as you describe, since the only output that changes is the FROM case that was broken.

--Prashant

The new status of this patch is: Waiting on Author