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
