[DEPARSE] Pretty-printed sublink test expressions lose needed parentheses

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]. 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

Best regards,
Henson