[DEPARSE] Pretty-printed sublink test expressions lose needed parentheses
Hackorum builds and tests every patch posted to the lists, not only commitfest submissions. This is Hackorum's own CI rather than the PostgreSQL project's, and it is still under testing - please report anything that looks wrong.
This thread has been committed, so CI has stopped here. Anything below is the last result it produced.
You can run a PostgreSQL built from this patch straight from Docker, with no checkout and no build:
docker run --rm -p 5432:5432 ghcr.io/hackorum-dev/postgres-patch:t254175psql -h localhost -U postgresBuilt from patchset v4 (message #4), October 19, 2026 at 02:19 PM.
Every patchset is also pushed to a branch of our PostgreSQL fork, so you can check out the same tree CI built. Without a PostgreSQL checkout:
git clone --branch t254175_4 https://github.com/hackorum-dev/postgres.gitIn a checkout you already have, add the fork once:
git remote add hackorum https://github.com/hackorum-dev/postgres.gitthen, for this patchset and every later one:
git fetch hackorum t254175_4 && git checkout t254175_4Patchset v4 (message #4) is on t254175_4
Hi hackers,
pg_get_viewdef(..., true) and the other pretty-printing paths drop the
parentheses around the test expression of a sublink, the left operand
of IN, of op ANY/ALL or of a single-column row comparison, even where
they are needed. The text then reads back as another expression, or
is rejected. The two attached patches add tests and fix it.
Background: precedence of comparison predicates
-----------------------------------------------
Even for someone used to SQL, it is hard to tell intuitively
how IN, EXISTS and the comparison operators bind relative to
each other, or to see that they are all comparisons of the
same kind. In the standard they are all <predicate>s, and one
cannot be the operand of another without parentheses [1]https://jakewheat.github.io/sql-overview/sql-2016-foundation-grammar.html. Our
gram.y allows it, though, so c < d IN (...) can be written.
None of the statements below is allowed by the standard.
Giving these predicates equal precedence is not that simple
in a parser, so implementations handle them differently; our
gram.y puts IN one level above the comparison operators. I
ran the same statements on a few other systems:
Q1: select true < true in (select false)
(true < true) IN ... -> true
true < (true IN ...) -> false
Q2: select true < true <> all (select true)
Q3: select 1 < 2 = true
system Q1 Q2 Q3
------------------- ------------- ------------- -------------
PostgreSQL master false syntax error syntax error
MariaDB 11.8 0 (false) 1 1
Oracle 23ai Free TRUE ORA-03048 ORA-03048
SQLite 3.54 1 (true) no ANY/ALL 1
Trino 471 syntax error syntax error syntax error
Only Trino follows the standard and rejects all three; the
others all differ.
Whatever one thinks of that, deparse has to print text that
our own grammar reads back as the same expression, so it has
to follow the precedence in gram.y, even if gram.y were wrong.
The problem
-----------
get_sublink_expr() prints the test expression with get_rule_expr(),
so in pretty mode it never gets parentheses. The examples below
are from the new regression tests and show the output before and
after the patch (only the relevant columns are shown):
create view tt28v as
select a, b, ((a or b) in (select false)) as r1, ...
from (values (true, false, 1, 2), ...) t(a, b, c, d);
before: (a OR b IN ( SELECT false)) AS r1,
after: ((a OR b) IN ( SELECT false)) AS r1,
r1 is false for the first row, but true in a view created
from the text printed before the patch, since it reads as
a OR (b IN (SELECT false)). Other test expressions make the
text rejected instead:
((c < d) <> all (select false))
before: (c < d <> ALL ( SELECT false)) AS r2,
ERROR: syntax error at or near "<>"
after: ((c < d) <> ALL ( SELECT false)) AS r2,
create operator * (leftarg = int, rightarg = text,
function = tt28f); -- returns bool
((c + 1) * any (select '2'::text))
before: (c + 1 * ANY ( SELECT '2'::text AS text)) AS r1,
ERROR: operator does not exist: integer + boolean
after: ((c + 1) * ANY ( SELECT '2'::text AS text)) AS r1,
Note that = ANY is printed as IN, so a comparison as its operand
gives no syntax error, unlike under <> ALL, but regroups as
c < (d IN (...)); that is a type error, or with booleans a
different expression. Test expressions that need no parentheses
print as before, for example (c + d IN ( SELECT 3)).
Only pretty mode is affected. Without it every operator
prints parentheses of its own, so pg_dump, which uses
non-pretty pg_get_viewdef(), is fine.
The fix
-------
0002 prints the test expression with get_rule_expr_paren(),
as for the operands of any other operator. For a simple
fix, that alone would do.
Even with the simple fix, the output is correct; isSimpleNode()
knows nothing about a sublink as the parent, so it only adds
parentheses that are not needed. Every view of the new tests
still reads back the same, but 27 of their test expressions gain
a redundant pair, as in ((c + d) IN ...), ((c IS NULL) IN ...)
and even ((( SELECT 1)) IN ...) around a subquery's own
parentheses. Pretty mode has such cases already:
x || y || z -> (x || y) || z
i << 1 << 2 -> (i << 1) << 2
Still, 0002 also teaches isSimpleNode() the precedence that
matters there, so that most output that was right does not change:
get_sublink_expr()
-> get_rule_expr_paren(testexpr, parentNode = sublink)
-> isSimpleNode()
-> sublink_testexpr_is_simple()
-> sublink_binop_prec()
- sublink_binop_prec(): finds the gram.y precedence level
(SUBLINK_PREC_*) of an operator by its printed name.
- sublink_testexpr_is_simple(): compares the levels of the combining
operator and the test expression to decide on parentheses, and
leaves nodes it cannot judge to the existing rules.
- isSimpleNode(): asks the above first when the parent is a sublink.
The combining operator has to be compared too, not just checked
against a fixed level: a combining operator named like an arithmetic
operator binds tighter than IN. (c + 1) needs no parentheses before
IN, but needs them before * ANY (a user-defined operator), as above.
Whether IN is taken to be at the level of the comparisons or
one above, the output is the same, as a comparison as the
test expression gets parentheses either way. Parentheses
are omitted only for operators at the Op level and above.
Tests
-----
0001 adds the tests with the output before the fix, and 0002
updates it, so that the diff shows what changes. Every view
is printed and created again from the printed text.
view combining test expression
----- ---------- ------------------------------------------
tt28v IN OR, AND, NOT (parenthesized); +, a sublink
tt28w IN < <> <= comparisons, IS DISTINCT FROM NULL on a row
(parenthesized); +, <<
= (row) a comparison (parenthesized)
tt28x IN * ^ << (binary), unary - + ~, IS NULL,
IS TRUE, IS JSON, COLLATE, = ANY (array),
an implicit cast, a row comparison
~~ ANY || (equal level, left-associative)
<< ANY +
tt28y * (user) +, << (parenthesized); *, unary -,
= ANY (array)
tt28z + (user) *, -; |/ (parenthesized)
* (user) ^; |/ (parenthesized)
^ (user) * (parenthesized); ^, unary -
I checked with gcov that all the added code is run.
Back branches
-------------
I think this should be back-patched to 15. I applied both
patches to REL_15_STABLE through REL_19_STABLE.
- REL_16 to REL_19: git am applies them cleanly and
make check passes. Committed on its own, the test patch
needs the older wording of the "operator does not exist"
errors in its expected output on REL_16 to REL_18.
- REL_15: the test patch does not apply cleanly. As IS JSON came
in 16, the T_JsonIsPredicate case in sublink_testexpr_is_simple()
and the IS JSON column of tt28x have to be removed, and the
expected output regenerated. With that, make check passes.
Longer term, it might be better to rework the parenthesization in
ruleutils.c around one table of the gram.y precedence levels for all
operators and expressions, instead of the special cases in
isSimpleNode(). That would also drop parentheses pretty mode adds
today, as in (x || y) || z, but it would change a lot of pretty output
and is for master only, so I kept this patch to the case at hand.
This came out of an AI-assisted audit of ruleutils.c. I
will post the other problems it found as separate,
independent reports as I work through them.
[1]: https://jakewheat.github.io/sql-overview/sql-2016-foundation-grammar.html
https://jakewheat.github.io/sql-overview/sql-2016-foundation-grammar.html
Best regards,
Henson
Attachments:
t254175_1v1-0002-Keep-the-parentheses-of-sublink-test-expressions-.patchapplication/octet-stream; name=v1-0002-Keep-the-parentheses-of-sublink-test-expressions-.patchDownload+221-55
v1-0001-Add-tests-for-pretty-printed-sublink-test-express.patchapplication/octet-stream; name=v1-0001-Add-tests-for-pretty-printed-sublink-test-express.patchDownload+367-2
On Tue, Oct 13, 2026 at 2:04 PM Henson Choi <assam258@gmail.com> wrote:
Whatever one thinks of that, deparse has to print text that
our own grammar reads back as the same expression, so it has
to follow the precedence in gram.y, even if gram.y were wrong.
Hi Henson,
I reviewed both v1 patches on master with assertions enabled. The core
regression results match the expected output with 0001 alone and with both
patches applied. The view recreation tests reproduce the changed results
and errors before the fix; after the fix, the recreated views return the
same results as the originals.
I found no blocking issue in the changes.
Thanks,
Sho Ito
--
＝＝＝＝＝＝＝＝＝＝＝＝＝＝＝＝＝＝＝＝＝＝＝＝＝＝＝＝＝＝＝＝＝＝＝＝＝＝＝
*Sho Ito (Sean)*
E-mail: i.sho.0628@gmail.com
LinkedIn: *https://www.linkedin.com/in/sho-ito-0628/
<https://www.linkedin.com/in/sho-ito-0628/>*
GitHub: https://github.com/sean0628/
＝＝＝＝＝＝＝＝＝＝＝＝＝＝＝＝＝＝＝＝＝＝＝＝＝＝＝＝＝＝＝＝＝＝＝＝＝＝＝
Henson Choi <assam258@gmail.com> writes:
pg_get_viewdef(..., true) and the other pretty-printing paths drop the
parentheses around the test expression of a sublink, the left operand
of IN, of op ANY/ALL or of a single-column row comparison, even where
they are needed. The text then reads back as another expression, or
is rejected. The two attached patches add tests and fix it.
Good catch. I do not like your proposed fix though: it adds yet more
complexity that has to be kept in sync with gram.y and yet doesn't
line up nicely with the grammar's rules. I think what we ought to
do for now is your "simple fix"
0002 prints the test expression with get_rule_expr_paren(),
as for the operands of any other operator. For a simple
fix, that alone would do.
without the other stuff. In the longer run, I'm interested in
investigating a more invasive improvement such as you mentioned:
Longer term, it might be better to rework the parenthesization in
ruleutils.c around one table of the gram.y precedence levels for all
operators and expressions, instead of the special cases in
isSimpleNode().
because otherwise we'll just keep having bugs of this sort.
(I'm already wondering whether we correctly deparse all the random
SQL/JSON syntax, for example.) I wonder if there is any way to
get the necessary data built automatically from gram.y ...
regards, tom lane
Hi Tom, Sho,
Thank you, Sho, for testing v1.
Good catch. I do not like your proposed fix though: it adds yet more
complexity that has to be kept in sync with gram.y and yet doesn't
line up nicely with the grammar's rules. I think what we ought to
do for now is your "simple fix"0002 prints the test expression with get_rule_expr_paren(),
as for the operands of any other operator. For a simple
fix, that alone would do.without the other stuff.
Agreed. The attached v2 keeps only the get_rule_expr_paren()
change. The tests are cut down to one case for each form that needs
parentheses, plus test expressions that need none and keep none.
Some test expressions that need no parentheses do get them now, as
in ((c + d) IN (SELECT 3)), which reads back as the same expression.
The full regression suite passes on master.
I also checked that the simple fix does not miss a place that needs
parentheses. Under a sublink, isSimpleNode() leaves them out only
for nodes printed as:
- a single word: c, $1, 42
- a function-like call: lower(t), COALESCE(c, 0)
- CASE ... END
- a field selection: (r).f
- a cast: c::text
- a postfix IS DOCUMENT: x IS DOCUMENT
All of these are closed on the right, so the IN or op ANY that follows
cannot bind into them; everything else gets parentheses. IS DOCUMENT
is closed on the right but not on the left, so it loses its own
parentheses when it is the right operand of another operator; that
is handled in CF 7451.
The code change applies cleanly to REL_15 through REL_19, and I built
each of them and ran the regression tests. Only the parentheses in
pretty output change, so I see no problem with back-patching.
In the longer run, I'm interested in investigating a more invasive
improvement such as you mentioned:Longer term, it might be better to rework the parenthesization in
ruleutils.c around one table of the gram.y precedence levels for all
operators and expressions, instead of the special cases in
isSimpleNode().because otherwise we'll just keep having bugs of this sort.
(I'm already wondering whether we correctly deparse all the random
SQL/JSON syntax, for example.) I wonder if there is any way to
get the necessary data built automatically from gram.y ...
I think so too. I became interested in deparse while resolving a
conflict between the existing deparse logic and the rule that a row
pattern recognition DEFINE clause cannot use qualifiers.
The ones of the same kind already posted are:
- IS DOCUMENT as an operand (CF 7451)
- operators on ROW() constructors (CF 7452)
On the SQL/JSON side you mention, and in XML, there are these:
- IS JSON in a JSON_OBJECT() value takes over WITH UNIQUE KEYS
- COLLATE on an XMLNAMESPACES URI loses its parentheses
- JSON_TABLE and XMLTABLE lose their alias and column aliases
Counting these, I have found about 30 candidates in deparse and am
going through them; some look as if they have no solution at all. I
will work through them one by one as time allows and post each once
I find a fix. Any that I still cannot find a fix for after a while, I
will report as they are, so that the community can help.
Looking at the candidates together, ruleutils.c is well divided along
node types: each node's printing lives in one place. What is spread
out is the cross-cutting part -- name selection, parentheses, lexical
details such as quoting, type labels and the resolution of operator
and function names -- which each printer and helper decides on its
own. One place that does not know a new node or syntax is then enough
to produce wrong text even when the others are right. Most of the
candidates fall on that side.
Names are the largest group, and the one most often giving silently
wrong results. ruleutils.c already has infrastructure for choosing
them, but names added later, such as RETURNING's OLD/NEW aliases,
EXCLUDED or a JOIN USING alias, are not all registered with it; there
the work is to make every kind of name go through it.
Parentheses, on the other hand, have no shared infrastructure at all.
What gram.y can give them directly is the precedence table. I have
a vague idea that, if that table were generated from gram.y the way
the node support functions are generated from the node definitions,
and the printed forms of analyzed nodes were tied to the gram.y
productions they print as, parentheses and keyword quoting could be
derived mechanically.
As I work through the cases and clues I have collected, a better
structure may become clearer; if a good idea comes to me, I will
make a proposal.
Best regards,
Henson
Attachments:
t254175_4v2-0001-Add-tests-for-pretty-printed-sublink-test-express.patchapplication/x-patch; name=v2-0001-Add-tests-for-pretty-printed-sublink-test-express.patchDownload+106-2
v2-0002-Keep-the-parentheses-of-sublink-test-expressions-.patchapplication/x-patch; name=v2-0002-Keep-the-parentheses-of-sublink-test-expressions-.patchDownload+23-17
Henson Choi <assam258@gmail.com> writes:
Agreed. The attached v2 keeps only the get_rule_expr_paren()
change. The tests are cut down to one case for each form that needs
parentheses, plus test expressions that need none and keep none.
Some test expressions that need no parentheses do get them now, as
in ((c + d) IN (SELECT 3)), which reads back as the same expression.
Okay. I merged this with the other thread about AS in
UPDATE/DELETE/MERGE, reduced the tests a bit (in particular the
behavior for UPDATE/DELETE/MERGE seems to me already fully covered
by existing tests), and pushed it.
All of these are closed on the right, so the IN or op ANY that follows
cannot bind into them; everything else gets parentheses. IS DOCUMENT
is closed on the right but not on the left, so it loses its own
parentheses when it is the right operand of another operator; that
is handled in CF 7451.
OK, I'll look at that.
Parentheses, on the other hand, have no shared infrastructure at all.
What gram.y can give them directly is the precedence table. I have
a vague idea that, if that table were generated from gram.y the way
the node support functions are generated from the node definitions,
and the printed forms of analyzed nodes were tied to the gram.y
productions they print as, parentheses and keyword quoting could be
derived mechanically.
In theory that should be do-able, but I'm worried that we might have
to more or less replicate bison to extract the data we need :-(.
Another thing to think about here is that sometimes, "extra" parens
are useful for readability even when not formally necessary,
because not everybody has all the precedence rules fully memorized.
So to some extent, ruleutil's habit of giving up and parenthesizing
for anything beyond basic arithmetic operators isn't such a bad thing.
I'm not sure what that observation leads to in terms of devising a
more-maintainable solution, but it's worth thinking about.
regards, tom lane