[PATCH] Fixed assertion issues in "pg_get_viewdef"

Hi everyone,

The following bug has been logged on the website:

Bug reference: 18710
Logged by: Man Zeng
Email address: zengman(at)halodbtech(dot)com
PostgreSQL version: 14.14
Operating system: centos-8
Description:

A prototype of the problem from
https://github.com/duckdb/pg_duckdb/issues/435
This exception can be reliably triggered by calling "pg_get_viewdef"

Step 1 ：
CREATE VIEW view_a 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;

Step 2 ：
SELECT oid FROM pg_class where relname = 'view_a';

Step 3：
SELECT pg_get_viewdef( this oid ); -- error

The abnormalities appear as follows
[postgres(at)iZuf6hwo0wgeev4dvua4csZ postgres]$ psql
psql (14.14)
Type "help" for help.

postgres=# CREATE VIEW view_a AS
postgres-# WITH RECURSIVE outermost(x) AS (
postgres(# SELECT 1
postgres(# UNION (WITH innermost1 AS (
postgres(# SELECT 2)
postgres(# SELECT * FROM outermost
postgres(# UNION SELECT * FROM innermost1)
postgres(# )
postgres-# SELECT * FROM outermost ORDER BY 1;
CREATE VIEW
postgres=# SELECT * FROM pg_class where relname = 'view_a';
oid | relname | relnamespace | reltype | reloftype | relowner | relam |
relfilenode | reltablespace | relpages | reltuples | relallvisible |
reltoastrelid | relhasindex | relisshared | relpersistence | relkind |
relnatt
s | relchecks | relhasrules | relhastriggers | relhassubclass |
relrowsecurity | relforcerowsecurity | relispopulated | relreplident |
relispartition | relrewrite | relfrozenxid | relminmxid | relacl |
reloptions | relpart
bound
-------+---------+--------------+---------+-----------+----------+-------+-------------+---------------+----------+-----------+---------------+---------------+-------------+-------------+----------------+---------+--------
--+-----------+-------------+----------------+----------------+----------------+---------------------+----------------+--------------+----------------+------------+--------------+------------+--------+------------+--------
------
32768 | view_a | 2200 | 32770 | 0 | 10 | 0 |
0 | 0 | 0 | -1 | 0 |
0 | f | f | p | v |
1 | 0 | t | f | f | f
| f | t | n | f |
0 | 0 | 0 | | |
(1 row)

postgres=# select pg_get_viewdef(32768);
TRAP: FailedAssertion("subquery->setOperations == NULL", File:
"ruleutils.c", Line: 6094, PID: 325948)
postgres: postgres postgres [local]
SELECT(ExceptionalCondition+0xb9)[0xb1a6c1]
postgres: postgres postgres [local] SELECT[0xa95d6b]
postgres: postgres postgres [local] SELECT[0xa960b5]
......

Fix:

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.

Please find the patch attached.

"=?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

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

------------------&nbsp;Original&nbsp;------------------
From: &nbsp;"Tom&nbsp;Lane"<tgl@sss.pgh.pa.us&gt;;
Date: &nbsp;Wed, Nov 20, 2024 00:21 AM
To: &nbsp;"曾满"<zengman@halodbtech.com&gt;;
Cc: &nbsp;"pgsql-hackers"<pgsql-hackers@lists.postgresql.org&gt;;
Subject: &nbsp;Re: [PATCH] Fixed assertion issues in "pg_get_viewdef"

&nbsp;
"=?utf-8?B?5pu+5ruh?=" <zengman@halodbtech.com&gt; writes:
&gt; 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
&gt; of sense here.

I looked at the git history and found that I added this assertion
in 07b4c48b6.&nbsp; 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.&nbsp; 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.&nbsp; 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

"=?gb18030?B?emVuZ21hbg==?=" <zengman@halodbtech.com> writes:

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.

I pushed the code fix, but I can't really convince myself that the
test case is worth the cycles it'd eat forevermore. If we had
a way to reach the situation where there's setops but not any of
the other clauses in a leaf query, perhaps that would be worth
checking ... but we don't. It's just belt-and-suspenders-too
programming.

regards, tom lane