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
From 8a2edccff9be39c505de636d26c309711778f007 Mon Sep 17 00:00:00 2001 From: Henson Choi <[email protected]> Date: Mon, 19 Oct 2026 21:00:00 +0900 Subject: [PATCH v2 1/2] Add tests for pretty-printed sublink test expressions A pretty-printed sublink whose test expression needs parentheses as an operand loses them, so the text is parsed back as another expression, or is rejected. Cover a test expression that needs parentheses before IN, before an operator ANY/ALL and before a single-column row comparison, and recreate each view from its pretty-printed text. Also cover test expressions that need none, which the next commit leaves as they are. The expected output shows the behavior before the fix. The next commit fixes it and updates the output. Discussion: https://postgr.es/m/CAAAe_zCy+D45Qd_br484F+V34y0BVA=hp_7cb=u6HysyMkE3UQ@mail.gmail.com --- src/test/regress/expected/create_view.out | 77 ++++++++++++++++++++++- src/test/regress/sql/create_view.sql | 30 +++++++++ 2 files changed, 106 insertions(+), 1 deletion(-) diff --git a/src/test/regress/expected/create_view.out b/src/test/regress/expected/create_view.out index 053fa56573f..2a1d9fdde9e 100644 --- a/src/test/regress/expected/create_view.out +++ b/src/test/regress/expected/create_view.out @@ -2285,6 +2285,77 @@ select viewname from pg_views where viewname = 'tt27v'; -- Ok to access a system (1 row) reset restrict_nonsystem_relation_kind; +-- pretty-printed sublink test expressions must keep their parentheses +create view tt28v as + select a, b, ((a or b) in (select false)) as r + from (values (true, false), (false, true)) t(a, b); +select pg_get_viewdef('tt28v'::regclass, true); + pg_get_viewdef +------------------------------------------------------- + SELECT a, + + b, + + (a OR b IN ( SELECT false)) AS r + + FROM ( VALUES (true,false), (false,true)) t(a, b); +(1 row) + +select pg_get_viewdef('tt28v'::regclass, true) as def \gset +create view tt28v_re as :def +select * from tt28v except select * from tt28v_re; + a | b | r +---+---+--- + t | f | f +(1 row) + +create view tt28w as + select c, d, + ((c < d) <> all (select false)) as r1, + (row(c < d) = (select true)) as r2 + from (values (1, 2)) t(c, d); +select pg_get_viewdef('tt28w'::regclass, true); + pg_get_viewdef +------------------------------------------- + SELECT c, + + d, + + (c < d <> ALL ( SELECT false)) AS r1,+ + (c < d = ( SELECT true)) AS r2 + + FROM ( VALUES (1,2)) t(c, d); +(1 row) + +select pg_get_viewdef('tt28w'::regclass, true) as def \gset +create view tt28w_re as :def +ERROR: syntax error at or near "<>" +LINE 3: (c < d <> ALL ( SELECT false)) AS r1, + ^ +select * from tt28w_re; +ERROR: relation "tt28w_re" does not exist +LINE 1: select * from tt28w_re; + ^ +-- test expressions that need no parentheses keep none +create view tt28x as + select c, x, + (c in (select 1)) as r1, + (lower(x) in (select 'a')) as r2, + (case when c > 0 then 1 end in (select 1)) as r3, + ((row(c, x)).f1 in (select 1)) as r4, + (c::text in (select '1')) as r5 + from (values (1, 'A')) t(c, x); +select pg_get_viewdef('tt28x'::regclass, true); + pg_get_viewdef +---------------------------------------------- + SELECT c, + + x, + + (c IN ( SELECT 1)) AS r1, + + (lower(x) IN ( SELECT 'a'::text)) AS r2,+ + ( + + CASE + + WHEN c > 0 THEN 1 + + ELSE NULL::integer + + END IN ( SELECT 1)) AS r3, + + ((ROW(c, x)).f1 IN ( SELECT 1)) AS r4, + + (c::text IN ( SELECT '1'::text)) AS r5 + + FROM ( VALUES (1,'A'::text)) t(c, x); +(1 row) + -- clean up all the random objects we made above DROP SCHEMA temp_view_test CASCADE; NOTICE: drop cascades to 27 other objects @@ -2316,7 +2387,7 @@ drop cascades to view aliased_view_2 drop cascades to view aliased_view_3 drop cascades to view aliased_view_4 DROP SCHEMA testviewschm2 CASCADE; -NOTICE: drop cascades to 80 other objects +NOTICE: drop cascades to 84 other objects DETAIL: drop cascades to table t1 drop cascades to view temporal1 drop cascades to view temporal2 @@ -2397,3 +2468,7 @@ drop cascades to view tt25v drop cascades to view tt26v drop cascades to table tt27v_tbl drop cascades to view tt27v +drop cascades to view tt28v +drop cascades to view tt28v_re +drop cascades to view tt28w +drop cascades to view tt28x diff --git a/src/test/regress/sql/create_view.sql b/src/test/regress/sql/create_view.sql index 75b8cd5d0fb..53d23701ee1 100644 --- a/src/test/regress/sql/create_view.sql +++ b/src/test/regress/sql/create_view.sql @@ -838,6 +838,36 @@ insert into tt27v values (1); -- Error select viewname from pg_views where viewname = 'tt27v'; -- Ok to access a system view. reset restrict_nonsystem_relation_kind; +-- pretty-printed sublink test expressions must keep their parentheses +create view tt28v as + select a, b, ((a or b) in (select false)) as r + from (values (true, false), (false, true)) t(a, b); +select pg_get_viewdef('tt28v'::regclass, true); +select pg_get_viewdef('tt28v'::regclass, true) as def \gset +create view tt28v_re as :def +select * from tt28v except select * from tt28v_re; + +create view tt28w as + select c, d, + ((c < d) <> all (select false)) as r1, + (row(c < d) = (select true)) as r2 + from (values (1, 2)) t(c, d); +select pg_get_viewdef('tt28w'::regclass, true); +select pg_get_viewdef('tt28w'::regclass, true) as def \gset +create view tt28w_re as :def +select * from tt28w_re; + +-- test expressions that need no parentheses keep none +create view tt28x as + select c, x, + (c in (select 1)) as r1, + (lower(x) in (select 'a')) as r2, + (case when c > 0 then 1 end in (select 1)) as r3, + ((row(c, x)).f1 in (select 1)) as r4, + (c::text in (select '1')) as r5 + from (values (1, 'A')) t(c, x); +select pg_get_viewdef('tt28x'::regclass, true); + -- clean up all the random objects we made above DROP SCHEMA temp_view_test CASCADE; DROP SCHEMA testviewschm2 CASCADE; -- 2.54.0 (Apple Git-157)
From a0f3390ee39b5527ca57b5d60c9fdf868e6dcd5a Mon Sep 17 00:00:00 2001 From: Henson Choi <[email protected]> Date: Mon, 19 Oct 2026 21:00:00 +0900 Subject: [PATCH v2 2/2] Keep the parentheses of sublink test expressions in pretty mode get_sublink_expr() printed the test expression of a sublink, the left operand of IN, of an operator ANY/ALL or of a single-column row comparison, with get_rule_expr(), so an expression that needs parentheses there lost them in pretty mode. Use get_rule_expr_paren(), as for the operands of an ordinary operator. isSimpleNode() has no rule for a sublink as the parent, so test expressions that need no parentheses get them too, as in (c + d) IN (SELECT ...). That is redundant but harmless. Teaching isSimpleNode() the grammar's precedence for this one case would add yet more rules that have to be kept in sync with gram.y; a more general rework of the parenthesization is left for later. Only pretty mode is affected; without it, every operator prints parentheses of its own. Suggested-by: Tom Lane <[email protected]> Reviewed-by: Sho Ito <[email protected]> Discussion: https://postgr.es/m/CAAAe_zCy+D45Qd_br484F+V34y0BVA=hp_7cb=u6HysyMkE3UQ@mail.gmail.com Backpatch-through: 15 --- src/backend/utils/adt/ruleutils.c | 9 ++++++- src/test/regress/expected/create_view.out | 31 +++++++++++------------ 2 files changed, 23 insertions(+), 17 deletions(-) diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c index 5e8e1683db0..626966621a1 100644 --- a/src/backend/utils/adt/ruleutils.c +++ b/src/backend/utils/adt/ruleutils.c @@ -11942,7 +11942,14 @@ get_sublink_expr(SubLink *sublink, deparse_context *context) /* single combining operator */ OpExpr *opexpr = (OpExpr *) sublink->testexpr; - get_rule_expr(linitial(opexpr->args), context, true); + /* + * The test expression is the left operand of the combining + * operator, so it needs the same parenthesization as any other + * operator operand (we print IN, op ANY/ALL or the row comparison + * operator after it). + */ + get_rule_expr_paren(linitial(opexpr->args), context, true, + (Node *) sublink); opname = generate_operator_name(opexpr->opno, exprType(linitial(opexpr->args)), exprType(lsecond(opexpr->args))); diff --git a/src/test/regress/expected/create_view.out b/src/test/regress/expected/create_view.out index 2a1d9fdde9e..829b5ef9b78 100644 --- a/src/test/regress/expected/create_view.out +++ b/src/test/regress/expected/create_view.out @@ -2294,7 +2294,7 @@ select pg_get_viewdef('tt28v'::regclass, true); ------------------------------------------------------- SELECT a, + b, + - (a OR b IN ( SELECT false)) AS r + + ((a OR b) IN ( SELECT false)) AS r + FROM ( VALUES (true,false), (false,true)) t(a, b); (1 row) @@ -2303,8 +2303,7 @@ create view tt28v_re as :def select * from tt28v except select * from tt28v_re; a | b | r ---+---+--- - t | f | f -(1 row) +(0 rows) create view tt28w as select c, d, @@ -2312,24 +2311,23 @@ create view tt28w as (row(c < d) = (select true)) as r2 from (values (1, 2)) t(c, d); select pg_get_viewdef('tt28w'::regclass, true); - pg_get_viewdef -------------------------------------------- - SELECT c, + - d, + - (c < d <> ALL ( SELECT false)) AS r1,+ - (c < d = ( SELECT true)) AS r2 + + pg_get_viewdef +--------------------------------------------- + SELECT c, + + d, + + ((c < d) <> ALL ( SELECT false)) AS r1,+ + ((c < d) = ( SELECT true)) AS r2 + FROM ( VALUES (1,2)) t(c, d); (1 row) select pg_get_viewdef('tt28w'::regclass, true) as def \gset create view tt28w_re as :def -ERROR: syntax error at or near "<>" -LINE 3: (c < d <> ALL ( SELECT false)) AS r1, - ^ select * from tt28w_re; -ERROR: relation "tt28w_re" does not exist -LINE 1: select * from tt28w_re; - ^ + c | d | r1 | r2 +---+---+----+---- + 1 | 2 | t | t +(1 row) + -- test expressions that need no parentheses keep none create view tt28x as select c, x, @@ -2387,7 +2385,7 @@ drop cascades to view aliased_view_2 drop cascades to view aliased_view_3 drop cascades to view aliased_view_4 DROP SCHEMA testviewschm2 CASCADE; -NOTICE: drop cascades to 84 other objects +NOTICE: drop cascades to 85 other objects DETAIL: drop cascades to table t1 drop cascades to view temporal1 drop cascades to view temporal2 @@ -2471,4 +2469,5 @@ drop cascades to view tt27v drop cascades to view tt28v drop cascades to view tt28v_re drop cascades to view tt28w +drop cascades to view tt28w_re drop cascades to view tt28x -- 2.54.0 (Apple Git-157)
