From: Tom Lane <tgl@sss.pgh.pa.us>
To: "=?utf-8?B?5pu+5ruh?=" <zengman@halodbtech.com>
cc: pgsql-hackers@lists.postgresql.org
Subject: Re: [PATCH] Fixed assertion issues in "pg_get_viewdef"
In-reply-to: <tencent_7ABF9B1F23B0C77606FC5FE3@qq.com>
References: <tencent_7ABF9B1F23B0C77606FC5FE3@qq.com>
Comments: In-reply-to "=?utf-8?B?5pu+5ruh?=" <zengman@halodbtech.com>
	message dated "Fri, 15 Nov 2024 19:05:33 +0800"
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0"
Content-ID: <3774836.1732033209.0@sss.pgh.pa.us>
Date: Tue, 19 Nov 2024 11:21:33 -0500
Message-ID: <3774968.1732033293@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/3774968.1732033293%40sss.pgh.pa.us>
Precedence: bulk

------- =_aaaaaaaaaa0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <3774836.1732033209.1@sss.pgh.pa.us>
Content-Transfer-Encoding: quoted-printable

"=3D?utf-8?B?5pu+5ruh?=3D" <zengman@halodbtech.com> writes:
> We can comment out the assertion that raises the exception, because I lo=
oked up the code and found that the assertion doesn't seem to make a lot
> of sense here.

I looked at the git history and found that I added this assertion
in 07b4c48b6.  Your example shows that indeed it's a thinko, but
I'm not convinced that simply removing it is the best answer.

The real point here is that we'd want to parenthesize if a "leaf"
subquery contains set operations, to ensure that the setop nesting
is represented correctly.  Normally, a "leaf" query would not contain
any set operations, but that can happen if the leaf query also
contains WITH/ORDER BY/FOR UPDATE/LIMIT, because that stops
transformSetOperationTree from merging it into the upper
SetOperationStmt tree.  So the assertion should have been less
"can't have setOperations here" and more "can't have setOperations
here unless there's also one of sortClause etc".

But on reflection I don't see why it needs to be an assert at all.
Let's just make nonempty setOperations another reason to force
need_paren on, as attached.

			regards, tom lane


------- =_aaaaaaaaaa0
Content-Type: text/x-diff; name="v2-fix-incorrect-assertion.patch";
	charset="us-ascii"
Content-ID: <3774836.1732033209.2@sss.pgh.pa.us>
Content-Description: v2-fix-incorrect-assertion.patch
Content-Transfer-Encoding: quoted-printable

diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/rul=
eutils.c
index a39068d1bf..2194ab3dfa 100644
--- a/src/backend/utils/adt/ruleutils.c
+++ b/src/backend/utils/adt/ruleutils.c
@@ -6353,13 +6353,19 @@ get_setop_query(Node *setOp, Query *query, deparse=
_context *context)
 		Query	   *subquery =3D rte->subquery;
 =

 		Assert(subquery !=3D NULL);
-		Assert(subquery->setOperations =3D=3D NULL);
-		/* Need parens if WITH, ORDER BY, FOR UPDATE, or LIMIT; see gram.y */
+
+		/*
+		 * We need parens if WITH, ORDER BY, FOR UPDATE, or LIMIT; see gram.y.
+		 * Also add parens if the leaf query contains its own set operations.
+		 * (That shouldn't happen unless one of the other clauses is also
+		 * present, see transformSetOperationTree; but let's be safe.)
+		 */
 		need_paren =3D (subquery->cteList ||
 					  subquery->sortClause ||
 					  subquery->rowMarks ||
 					  subquery->limitOffset ||
-					  subquery->limitCount);
+					  subquery->limitCount ||
+					  subquery->setOperations);
 		if (need_paren)
 			appendStringInfoChar(buf, '(');
 		get_query_def(subquery, buf, context->namespaces,

------- =_aaaaaaaaaa0--