From: zengman <zengman@halodbtech.com>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: pgsql-hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: [PATCH] Fixed assertion issues in "pg_get_viewdef"
Date: Wed, 20 Nov 2024 11:11:26 +0800
Message-ID: <tencent_7A5BA3B734BA3F616F6C2732@qq.com> (raw)
In-Reply-To: <3774968.1732033293@sss.pgh.pa.us>
References: <tencent_7ABF9B1F23B0C77606FC5FE3@qq.com>
<3774968.1732033293@sss.pgh.pa.us>
Thanks for your guidance, you are right, I looked at your patch
and combined it with the example to generate a new patch,
which is really better.
Thanks,
Man Zeng
------------------ Original ------------------
From: "Tom Lane"<tgl@sss.pgh.pa.us>;
Date: Wed, Nov 20, 2024 00:21 AM
To: "曾满"<zengman@halodbtech.com>;
Cc: "pgsql-hackers"<pgsql-hackers@lists.postgresql.org>;
Subject: Re: [PATCH] Fixed assertion issues in "pg_get_viewdef"
"=?utf-8?B?5pu+5ruh?=" <zengman@halodbtech.com> writes:
> We can comment out the assertion that raises the exception, because I looked 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
Attachments:
[application/octet-stream] v3-fix-incorrect-assertion.patch (4.3K, ../tencent_7A5BA3B734BA3F616F6C2732@qq.com/3-v3-fix-incorrect-assertion.patch)
download | inline diff:
From 74d44e46df86576f7b80bf98f027363dae3a44ef Mon Sep 17 00:00:00 2001
From: zengman <zengman@halodbtech.com>
Date: Wed, 20 Nov 2024 10:38:57 +0800
Subject: [PATCH] fix incorrect assertion
---
src/backend/utils/adt/ruleutils.c | 11 +++++---
src/test/regress/expected/create_view.out | 31 ++++++++++++++++++++++-
src/test/regress/sql/create_view.sql | 11 ++++++++
3 files changed, 49 insertions(+), 4 deletions(-)
diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.cindex 99d9bb5d6f..ac0075ad92 100644--- a/src/backend/utils/adt/ruleutils.c+++ b/src/backend/utils/adt/ruleutils.c@@ -6091,13 +6091,18 @@ get_setop_query(Node *setOp, Query *query, deparse_context *context,
Query *subquery = rte->subquery;
Assert(subquery != NULL);
- Assert(subquery->setOperations == 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 = (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, resultDesc,
diff --git a/src/test/regress/expected/create_view.out b/src/test/regress/expected/create_view.outindex b1b96a8874..ce80d7cc89 100644--- a/src/test/regress/expected/create_view.out+++ b/src/test/regress/expected/create_view.out@@ -2028,6 +2028,34 @@ select viewname from pg_views where viewname = 'tt27v'; -- Ok to access a system
(1 row)
reset restrict_nonsystem_relation_kind;
+CREATE VIEW tt28v AS+WITH RECURSIVE outermost(x) AS (+ SELECT 1+ UNION (WITH innermost1 AS (+ SELECT 2)+ SELECT * FROM outermost+ UNION SELECT * FROM innermost1))+ SELECT * FROM outermost ORDER BY 1;+SELECT pg_get_viewdef('tt28v', true);+ pg_get_viewdef +-----------------------------------------+ WITH RECURSIVE outermost(x) AS ( ++ SELECT 1 AS "?column?" ++ UNION ++ ( WITH innermost1 AS ( ++ SELECT 2 AS "?column?"++ ) ++ SELECT outermost_1.x ++ FROM outermost outermost_1 ++ UNION ++ SELECT innermost1."?column?" ++ FROM innermost1) ++ ) ++ SELECT outermost.x ++ FROM outermost ++ ORDER BY outermost.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
@@ -2059,7 +2087,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 77 other objects+NOTICE: drop cascades to 78 other objects
DETAIL: drop cascades to table t1
drop cascades to view temporal1
drop cascades to view temporal2
@@ -2137,3 +2165,4 @@ drop cascades to view tt26v
drop cascades to table tt26
drop cascades to table tt27v_tbl
drop cascades to view tt27v
+drop cascades to view tt28vdiff --git a/src/test/regress/sql/create_view.sql b/src/test/regress/sql/create_view.sqlindex 662bf57c86..b42c0b26fd 100644--- a/src/test/regress/sql/create_view.sql+++ b/src/test/regress/sql/create_view.sql@@ -717,6 +717,17 @@ 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;
+CREATE VIEW tt28v AS+WITH RECURSIVE outermost(x) AS (+ SELECT 1+ UNION (WITH innermost1 AS (+ SELECT 2)+ SELECT * FROM outermost+ UNION SELECT * FROM innermost1))+ SELECT * FROM outermost ORDER BY 1;++SELECT pg_get_viewdef('tt28v', true);+
-- clean up all the random objects we made above
DROP SCHEMA temp_view_test CASCADE;
DROP SCHEMA testviewschm2 CASCADE;
--
2.27.0
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: zengman@halodbtech.com, tgl@sss.pgh.pa.us, pgsql-hackers@lists.postgresql.org
Subject: Re: [PATCH] Fixed assertion issues in "pg_get_viewdef"
In-Reply-To: <tencent_7A5BA3B734BA3F616F6C2732@qq.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox