| From: | Prashant Chinnam <crprashant(at)hotmail(dot)com> |
|---|---|
| To: | pgsql-hackers(at)lists(dot)postgresql(dot)org |
| Cc: | Henson Choi <assam258(at)gmail(dot)com> |
| Subject: | Re: [DEPARSE] IS DOCUMENT as an operand loses needed parentheses |
| Date: | 2026-10-20 16:52:59 |
| Message-ID: | 179251517918.1157.3687095928544604332.pgcf@coridan.postgresql.org |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
The following review has been posted through the commitfest application:
make installcheck-world: not tested
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.
IS DOCUMENT is a postfix test like IS NULL, and treating it as
self-delimiting in isSimpleNode() and looks_like_function() is
what loses the parentheses. The three changes look right to me,
including passing the single argument to get_rule_expr_paren()
instead of the argument list.
For the hands-on part I used AI assistance. The bug was
reproduced on master (the view reads back as (f = x IS DOCUMENT)
and fails with "operator does not exist: boolean = xml"; the
index definition prints without parentheses and fails with a
syntax error), and with v1 applied the view, index and
statistics definitions all read back and recreate successfully.
make installcheck-world was also run against an installed
server with the patch applied: the main regression suite passes
(239/239), as does the isolation suite (134/134), along with 47
of 51 contrib suites and 35 of 40 test-module suites. The
suites that did not run need features that were not built in
that environment (plperl, uuid-ossp, and the injection-points
option), so they are unrelated to this change.
One thing that was checked with that assistance: the other
IS_DOCUMENT special cases in the tree (parser, executor,
nodeFuncs, clauses) are not deparse paths, so the scope of the
patch looks complete to me. The double parentheses in the cast
case also match how IS NULL already prints, as you noted. No
documentation changes are needed for this fix.
Thanks for the clear report and the round-trip tests.
Regards,
Prashant
The new status of this patch is: Ready for Committer
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Greg Burd | 2026-10-20 16:54:13 | Re: [PATCH] pageinspect: validate line pointers before using them |
| Previous Message | Palak Chaturvedi | 2026-10-20 16:43:58 | Re: Changing shared_buffers without restart |