Re: Failed Assert in pgstat_assoc_relation - Mailing list pgsql-hackers

Andres Freund <andres@anarazel.de> writes:
> On 2022-11-28 13:37:16 -0500, Tom Lane wrote:
>> As far as HEAD is concerned, maybe it's time to nuke the whole
>> convert-table-to-view kluge entirely?  Only pg_dump older than
>> 9.4 will emit such code, so we're really about out of reasons
>> to keep on maintaining it.

> Sounds good to me.

Here's a draft patch for that.  If we apply this to HEAD then
we only need that klugery in relcache for v15.

            regards, tom lane

diff --git a/doc/src/sgml/rules.sgml b/doc/src/sgml/rules.sgml
index be3d17c852..3c9459b648 100644
--- a/doc/src/sgml/rules.sgml
+++ b/doc/src/sgml/rules.sgml
@@ -280,14 +280,16 @@

 <para>
     Views in <productname>PostgreSQL</productname> are implemented
-    using the rule system. In fact, there is essentially no difference
-    between:
+    using the rule system.  A view is basically an empty table (having no
+    actual storage) with an <literal>ON SELECT DO INSTEAD</literal> rule.
+    Conventionally, that rule is named <literal>_RETURN</literal>.
+    So a view like

 <programlisting>
 CREATE VIEW myview AS SELECT * FROM mytab;
 </programlisting>

-    compared against the two commands:
+    is very nearly the same thing as

 <programlisting>
 CREATE TABLE myview (<replaceable>same column list as mytab</replaceable>);
@@ -295,13 +297,17 @@ CREATE RULE "_RETURN" AS ON SELECT TO myview DO INSTEAD
     SELECT * FROM mytab;
 </programlisting>

-    because this is exactly what the <command>CREATE VIEW</command>
-    command does internally.  This has some side effects. One of them
-    is that the information about a view in the
-    <productname>PostgreSQL</productname> system catalogs is exactly
-    the same as it is for a table. So for the parser, there is
-    absolutely no difference between a table and a view. They are the
-    same thing: relations.
+    although you can't actually write that, because tables are not
+    allowed to have <literal>ON SELECT</literal> rules.
+</para>
+
+<para>
+    A view can also have other kinds of <literal>DO INSTEAD</literal>
+    rules, allowing <command>INSERT</command>, <command>UPDATE</command>,
+    or <command>DELETE</command> commands to be performed on the view
+    despite its lack of underlying storage.
+    This is discussed further below, in
+    <xref linkend="rules-views-update"/>.
 </para>

 <sect2 id="rules-select">
@@ -1111,7 +1117,7 @@ SELECT word FROM words ORDER BY word <-> 'caterpiler' LIMIT 10;
 <para>
     Rules that are defined on <command>INSERT</command>, <command>UPDATE</command>,
     and <command>DELETE</command> are significantly different from the view rules
-    described in the previous section. First, their <command>CREATE
+    described in the previous sections. First, their <command>CREATE
     RULE</command> command allows more:

     <itemizedlist>
diff --git a/src/backend/rewrite/rewriteDefine.c b/src/backend/rewrite/rewriteDefine.c
index db45d8a08b..8ac2c81b06 100644
--- a/src/backend/rewrite/rewriteDefine.c
+++ b/src/backend/rewrite/rewriteDefine.c
@@ -239,7 +239,6 @@ DefineQueryRewrite(const char *rulename,
     Relation    event_relation;
     ListCell   *l;
     Query       *query;
-    bool        RelisBecomingView = false;
     Oid            ruleId = InvalidOid;
     ObjectAddress address;

@@ -311,7 +310,18 @@ DefineQueryRewrite(const char *rulename,
         /*
          * Rules ON SELECT are restricted to view definitions
          *
-         * So there cannot be INSTEAD NOTHING, ...
+         * So this had better be a view, ...
+         */
+        if (event_relation->rd_rel->relkind != RELKIND_VIEW &&
+            event_relation->rd_rel->relkind != RELKIND_MATVIEW)
+            ereport(ERROR,
+                    (errcode(ERRCODE_WRONG_OBJECT_TYPE),
+                     errmsg("relation \"%s\" cannot have ON SELECT rules",
+                            RelationGetRelationName(event_relation)),
+                     errdetail_relkind_not_supported(event_relation->rd_rel->relkind)));
+
+        /*
+         * ... there cannot be INSTEAD NOTHING, ...
          */
         if (action == NIL)
             ereport(ERROR,
@@ -407,93 +417,6 @@ DefineQueryRewrite(const char *rulename,
                                 ViewSelectRuleName)));
             rulename = pstrdup(ViewSelectRuleName);
         }
-
-        /*
-         * Are we converting a relation to a view?
-         *
-         * If so, check that the relation is empty because the storage for the
-         * relation is going to be deleted.  Also insist that the rel not be
-         * involved in partitioning, nor have any triggers, indexes, child or
-         * parent tables, RLS policies, or RLS enabled.  (Note: some of these
-         * tests are too strict, because they will reject relations that once
-         * had such but don't anymore.  But we don't really care, because this
-         * whole business of converting relations to views is just an obsolete
-         * kluge to allow dump/reload of views that participate in circular
-         * dependencies.)
-         */
-        if (event_relation->rd_rel->relkind != RELKIND_VIEW &&
-            event_relation->rd_rel->relkind != RELKIND_MATVIEW)
-        {
-            TableScanDesc scanDesc;
-            Snapshot    snapshot;
-            TupleTableSlot *slot;
-
-            if (event_relation->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
-                ereport(ERROR,
-                        (errcode(ERRCODE_WRONG_OBJECT_TYPE),
-                         errmsg("cannot convert partitioned table \"%s\" to a view",
-                                RelationGetRelationName(event_relation))));
-
-            /* only case left: */
-            Assert(event_relation->rd_rel->relkind == RELKIND_RELATION);
-
-            if (event_relation->rd_rel->relispartition)
-                ereport(ERROR,
-                        (errcode(ERRCODE_WRONG_OBJECT_TYPE),
-                         errmsg("cannot convert partition \"%s\" to a view",
-                                RelationGetRelationName(event_relation))));
-
-            snapshot = RegisterSnapshot(GetLatestSnapshot());
-            scanDesc = table_beginscan(event_relation, snapshot, 0, NULL);
-            slot = table_slot_create(event_relation, NULL);
-            if (table_scan_getnextslot(scanDesc, ForwardScanDirection, slot))
-                ereport(ERROR,
-                        (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
-                         errmsg("could not convert table \"%s\" to a view because it is not empty",
-                                RelationGetRelationName(event_relation))));
-            ExecDropSingleTupleTableSlot(slot);
-            table_endscan(scanDesc);
-            UnregisterSnapshot(snapshot);
-
-            if (event_relation->rd_rel->relhastriggers)
-                ereport(ERROR,
-                        (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
-                         errmsg("could not convert table \"%s\" to a view because it has triggers",
-                                RelationGetRelationName(event_relation)),
-                         errhint("In particular, the table cannot be involved in any foreign key relationships.")));
-
-            if (event_relation->rd_rel->relhasindex)
-                ereport(ERROR,
-                        (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
-                         errmsg("could not convert table \"%s\" to a view because it has indexes",
-                                RelationGetRelationName(event_relation))));
-
-            if (event_relation->rd_rel->relhassubclass)
-                ereport(ERROR,
-                        (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
-                         errmsg("could not convert table \"%s\" to a view because it has child tables",
-                                RelationGetRelationName(event_relation))));
-
-            if (has_superclass(RelationGetRelid(event_relation)))
-                ereport(ERROR,
-                        (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
-                         errmsg("could not convert table \"%s\" to a view because it has parent tables",
-                                RelationGetRelationName(event_relation))));
-
-            if (event_relation->rd_rel->relrowsecurity)
-                ereport(ERROR,
-                        (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
-                         errmsg("could not convert table \"%s\" to a view because it has row security enabled",
-                                RelationGetRelationName(event_relation))));
-
-            if (relation_has_policies(event_relation))
-                ereport(ERROR,
-                        (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
-                         errmsg("could not convert table \"%s\" to a view because it has row security policies",
-                                RelationGetRelationName(event_relation))));
-
-            RelisBecomingView = true;
-        }
     }
     else
     {
@@ -569,94 +492,6 @@ DefineQueryRewrite(const char *rulename,
         SetRelationRuleStatus(event_relid, true);
     }

-    /* ---------------------------------------------------------------------
-     * If the relation is becoming a view:
-     * - delete the associated storage files
-     * - get rid of any system attributes in pg_attribute; a view shouldn't
-     *     have any of those
-     * - remove the toast table; there is no need for it anymore, and its
-     *     presence would make vacuum slightly more complicated
-     * - set relkind to RELKIND_VIEW, and adjust other pg_class fields
-     *     to be appropriate for a view
-     *
-     * NB: we had better have AccessExclusiveLock to do this ...
-     * ---------------------------------------------------------------------
-     */
-    if (RelisBecomingView)
-    {
-        Relation    relationRelation;
-        Oid            toastrelid;
-        HeapTuple    classTup;
-        Form_pg_class classForm;
-
-        relationRelation = table_open(RelationRelationId, RowExclusiveLock);
-        toastrelid = event_relation->rd_rel->reltoastrelid;
-
-        /* drop storage while table still looks like a table  */
-        RelationDropStorage(event_relation);
-        DeleteSystemAttributeTuples(event_relid);
-
-        /*
-         * Drop the toast table if any.  (This won't take care of updating the
-         * toast fields in the relation's own pg_class entry; we handle that
-         * below.)
-         */
-        if (OidIsValid(toastrelid))
-        {
-            ObjectAddress toastobject;
-
-            /*
-             * Delete the dependency of the toast relation on the main
-             * relation so we can drop the former without dropping the latter.
-             */
-            deleteDependencyRecordsFor(RelationRelationId, toastrelid,
-                                       false);
-
-            /* Make deletion of dependency record visible */
-            CommandCounterIncrement();
-
-            /* Now drop toast table, including its index */
-            toastobject.classId = RelationRelationId;
-            toastobject.objectId = toastrelid;
-            toastobject.objectSubId = 0;
-            performDeletion(&toastobject, DROP_RESTRICT,
-                            PERFORM_DELETION_INTERNAL);
-        }
-
-        /*
-         * SetRelationRuleStatus may have updated the pg_class row, so we must
-         * advance the command counter before trying to update it again.
-         */
-        CommandCounterIncrement();
-
-        /*
-         * Fix pg_class entry to look like a normal view's, including setting
-         * the correct relkind and removal of reltoastrelid of the toast table
-         * we potentially removed above.
-         */
-        classTup = SearchSysCacheCopy1(RELOID, ObjectIdGetDatum(event_relid));
-        if (!HeapTupleIsValid(classTup))
-            elog(ERROR, "cache lookup failed for relation %u", event_relid);
-        classForm = (Form_pg_class) GETSTRUCT(classTup);
-
-        classForm->relam = InvalidOid;
-        classForm->reltablespace = InvalidOid;
-        classForm->relpages = 0;
-        classForm->reltuples = -1;
-        classForm->relallvisible = 0;
-        classForm->reltoastrelid = InvalidOid;
-        classForm->relhasindex = false;
-        classForm->relkind = RELKIND_VIEW;
-        classForm->relfrozenxid = InvalidTransactionId;
-        classForm->relminmxid = InvalidMultiXactId;
-        classForm->relreplident = REPLICA_IDENTITY_NOTHING;
-
-        CatalogTupleUpdate(relationRelation, &classTup->t_self, classTup);
-
-        heap_freetuple(classTup);
-        table_close(relationRelation, RowExclusiveLock);
-    }
-
     ObjectAddressSet(address, RewriteRelationId, ruleId);

     /* Close rel, but keep lock till commit... */
diff --git a/src/test/modules/test_ddl_deparse/expected/create_rule.out
b/src/test/modules/test_ddl_deparse/expected/create_rule.out
index fe3d047a41..78724df818 100644
--- a/src/test/modules/test_ddl_deparse/expected/create_rule.out
+++ b/src/test/modules/test_ddl_deparse/expected/create_rule.out
@@ -1,6 +1,8 @@
 ---
 --- CREATE_RULE
 ---
+--- Note: views' ON SELECT rules are tested elsewhere.
+---
 CREATE RULE rule_1 AS
   ON INSERT
   TO datatype_table
@@ -16,12 +18,6 @@ CREATE RULE rule_3 AS
   TO datatype_table
   DO ALSO NOTHING;
 NOTICE:  DDL test: type simple, tag CREATE RULE
-CREATE RULE "_RETURN" AS
-  ON SELECT
-  TO like_datatype_table
-  DO INSTEAD
-    SELECT * FROM datatype_view;
-NOTICE:  DDL test: type simple, tag CREATE RULE
 CREATE RULE rule_3 AS
   ON DELETE
   TO like_datatype_table
diff --git a/src/test/modules/test_ddl_deparse/sql/create_rule.sql
b/src/test/modules/test_ddl_deparse/sql/create_rule.sql
index 60ac151a26..7d03859a17 100644
--- a/src/test/modules/test_ddl_deparse/sql/create_rule.sql
+++ b/src/test/modules/test_ddl_deparse/sql/create_rule.sql
@@ -1,6 +1,8 @@
 ---
 --- CREATE_RULE
 ---
+--- Note: views' ON SELECT rules are tested elsewhere.
+---


 CREATE RULE rule_1 AS
@@ -18,12 +20,6 @@ CREATE RULE rule_3 AS
   TO datatype_table
   DO ALSO NOTHING;

-CREATE RULE "_RETURN" AS
-  ON SELECT
-  TO like_datatype_table
-  DO INSTEAD
-    SELECT * FROM datatype_view;
-
 CREATE RULE rule_3 AS
   ON DELETE
   TO like_datatype_table
diff --git a/src/test/regress/expected/rowsecurity.out b/src/test/regress/expected/rowsecurity.out
index b5f6eecba1..c6ab6f4afb 100644
--- a/src/test/regress/expected/rowsecurity.out
+++ b/src/test/regress/expected/rowsecurity.out
@@ -3942,28 +3942,6 @@ DROP ROLE regress_rls_frank; -- succeeds
 ROLLBACK TO q;
 ROLLBACK; -- cleanup
 --
--- Converting table to view
---
-BEGIN;
-CREATE TABLE t (c int);
-CREATE POLICY p ON t USING (c % 2 = 1);
-ALTER TABLE t ENABLE ROW LEVEL SECURITY;
-SAVEPOINT q;
-CREATE RULE "_RETURN" AS ON SELECT TO t DO INSTEAD
-  SELECT * FROM generate_series(1,5) t0(c); -- fails due to row-level security enabled
-ERROR:  could not convert table "t" to a view because it has row security enabled
-ROLLBACK TO q;
-ALTER TABLE t DISABLE ROW LEVEL SECURITY;
-SAVEPOINT q;
-CREATE RULE "_RETURN" AS ON SELECT TO t DO INSTEAD
-  SELECT * FROM generate_series(1,5) t0(c); -- fails due to policy p on t
-ERROR:  could not convert table "t" to a view because it has row security policies
-ROLLBACK TO q;
-DROP POLICY p ON t;
-CREATE RULE "_RETURN" AS ON SELECT TO t DO INSTEAD
-  SELECT * FROM generate_series(1,5) t0(c); -- succeeds
-ROLLBACK;
---
 -- Policy expression handling
 --
 BEGIN;
diff --git a/src/test/regress/expected/rules.out b/src/test/regress/expected/rules.out
index 37c1c86473..8d85ffc760 100644
--- a/src/test/regress/expected/rules.out
+++ b/src/test/regress/expected/rules.out
@@ -2707,54 +2707,26 @@ ERROR:  cannot drop rule _RETURN on view rules_fooview because view rules_foovie
 HINT:  You can drop view rules_fooview instead.
 drop view rules_fooview;
 --
--- test conversion of table to view (needed to load some pg_dump files)
+-- We used to allow converting a table to a view by creating a "_RETURN"
+-- rule for it, but no more.
 --
 create table rules_fooview (x int, y text);
-select xmin, * from rules_fooview;
- xmin | x | y
-------+---+---
-(0 rows)
-
-create rule "_RETURN" as on select to rules_fooview do instead
-  select 1 as x, 'aaa'::text as y;
-select * from rules_fooview;
- x |  y
----+-----
- 1 | aaa
-(1 row)
-
-select xmin, * from rules_fooview;  -- fail, views don't have such a column
-ERROR:  column "xmin" does not exist
-LINE 1: select xmin, * from rules_fooview;
-               ^
-select reltoastrelid, relkind, relfrozenxid
-  from pg_class where oid = 'rules_fooview'::regclass;
- reltoastrelid | relkind | relfrozenxid
----------------+---------+--------------
-             0 | v       |            0
-(1 row)
-
-drop view rules_fooview;
--- cannot convert an inheritance parent or child to a view, though
-create table rules_fooview (x int, y text);
-create table rules_fooview_child () inherits (rules_fooview);
 create rule "_RETURN" as on select to rules_fooview do instead
   select 1 as x, 'aaa'::text as y;
-ERROR:  could not convert table "rules_fooview" to a view because it has child tables
-create rule "_RETURN" as on select to rules_fooview_child do instead
-  select 1 as x, 'aaa'::text as y;
-ERROR:  could not convert table "rules_fooview_child" to a view because it has parent tables
-drop table rules_fooview cascade;
-NOTICE:  drop cascades to table rules_fooview_child
+ERROR:  relation "rules_fooview" cannot have ON SELECT rules
+DETAIL:  This operation is not supported for tables.
+drop table rules_fooview;
 -- likewise, converting a partitioned table or partition to view is not allowed
 create table rules_fooview (x int, y text) partition by list (x);
 create rule "_RETURN" as on select to rules_fooview do instead
   select 1 as x, 'aaa'::text as y;
-ERROR:  cannot convert partitioned table "rules_fooview" to a view
+ERROR:  relation "rules_fooview" cannot have ON SELECT rules
+DETAIL:  This operation is not supported for partitioned tables.
 create table rules_fooview_part partition of rules_fooview for values in (1);
 create rule "_RETURN" as on select to rules_fooview_part do instead
   select 1 as x, 'aaa'::text as y;
-ERROR:  cannot convert partition "rules_fooview_part" to a view
+ERROR:  relation "rules_fooview_part" cannot have ON SELECT rules
+DETAIL:  This operation is not supported for tables.
 drop table rules_fooview;
 --
 -- check for planner problems with complex inherited UPDATES
diff --git a/src/test/regress/sql/rowsecurity.sql b/src/test/regress/sql/rowsecurity.sql
index febf3cc4cf..b38fa8ed8f 100644
--- a/src/test/regress/sql/rowsecurity.sql
+++ b/src/test/regress/sql/rowsecurity.sql
@@ -1713,30 +1713,6 @@ ROLLBACK TO q;

 ROLLBACK; -- cleanup

---
--- Converting table to view
---
-BEGIN;
-CREATE TABLE t (c int);
-CREATE POLICY p ON t USING (c % 2 = 1);
-ALTER TABLE t ENABLE ROW LEVEL SECURITY;
-
-SAVEPOINT q;
-CREATE RULE "_RETURN" AS ON SELECT TO t DO INSTEAD
-  SELECT * FROM generate_series(1,5) t0(c); -- fails due to row-level security enabled
-ROLLBACK TO q;
-
-ALTER TABLE t DISABLE ROW LEVEL SECURITY;
-SAVEPOINT q;
-CREATE RULE "_RETURN" AS ON SELECT TO t DO INSTEAD
-  SELECT * FROM generate_series(1,5) t0(c); -- fails due to policy p on t
-ROLLBACK TO q;
-
-DROP POLICY p ON t;
-CREATE RULE "_RETURN" AS ON SELECT TO t DO INSTEAD
-  SELECT * FROM generate_series(1,5) t0(c); -- succeeds
-ROLLBACK;
-
 --
 -- Policy expression handling
 --
diff --git a/src/test/regress/sql/rules.sql b/src/test/regress/sql/rules.sql
index bfb5f3b0bb..aee5381665 100644
--- a/src/test/regress/sql/rules.sql
+++ b/src/test/regress/sql/rules.sql
@@ -884,33 +884,14 @@ drop rule "_RETURN" on rules_fooview;
 drop view rules_fooview;

 --
--- test conversion of table to view (needed to load some pg_dump files)
+-- We used to allow converting a table to a view by creating a "_RETURN"
+-- rule for it, but no more.
 --

 create table rules_fooview (x int, y text);
-select xmin, * from rules_fooview;
-
 create rule "_RETURN" as on select to rules_fooview do instead
   select 1 as x, 'aaa'::text as y;
-
-select * from rules_fooview;
-select xmin, * from rules_fooview;  -- fail, views don't have such a column
-
-select reltoastrelid, relkind, relfrozenxid
-  from pg_class where oid = 'rules_fooview'::regclass;
-
-drop view rules_fooview;
-
--- cannot convert an inheritance parent or child to a view, though
-create table rules_fooview (x int, y text);
-create table rules_fooview_child () inherits (rules_fooview);
-
-create rule "_RETURN" as on select to rules_fooview do instead
-  select 1 as x, 'aaa'::text as y;
-create rule "_RETURN" as on select to rules_fooview_child do instead
-  select 1 as x, 'aaa'::text as y;
-
-drop table rules_fooview cascade;
+drop table rules_fooview;

 -- likewise, converting a partitioned table or partition to view is not allowed
 create table rules_fooview (x int, y text) partition by list (x);