Обсуждение: Failed Assert in pgstat_assoc_relation

Hi,

While reviewing/testing one of the patches I found the following Assert:
#0  __pthread_kill_implementation (no_tid=0, signo=6,
threadid=139624429171648) at ./nptl/pthread_kill.c:44
#1  __pthread_kill_internal (signo=6, threadid=139624429171648) at
./nptl/pthread_kill.c:78
#2  __GI___pthread_kill (threadid=139624429171648,
signo=signo@entry=6) at ./nptl/pthread_kill.c:89
#3  0x00007efcda6e3476 in __GI_raise (sig=sig@entry=6) at
../sysdeps/posix/raise.c:26
#4  0x00007efcda6c97f3 in __GI_abort () at ./stdlib/abort.c:79
#5  0x00005590bf283139 in ExceptionalCondition
(conditionName=0x5590bf468170 "rel->pgstat_info->relation == NULL",
fileName=0x5590bf46812b "pgstat_relation.c", lineNumber=143) at
assert.c:66
#6  0x00005590bf0ce5f8 in pgstat_assoc_relation (rel=0x7efcce996a48)
at pgstat_relation.c:143
#7  0x00005590beb83046 in initscan (scan=0x5590bfbf4af8, key=0x0,
keep_startblock=false) at heapam.c:343
#8  0x00005590beb8466f in heap_beginscan (relation=0x7efcce996a48,
snapshot=0x5590bfb5a520, nkeys=0, key=0x0, parallel_scan=0x0,
flags=449) at heapam.c:1223
#9  0x00005590bf02af39 in table_beginscan (rel=0x7efcce996a48,
snapshot=0x5590bfb5a520, nkeys=0, key=0x0) at
../../../src/include/access/tableam.h:891
#10 0x00005590bf02bf8a in DefineQueryRewrite (rulename=0x5590bfb281d0
"_RETURN", event_relid=16387, event_qual=0x0, event_type=CMD_SELECT,
is_instead=true, replace=false, action=0x5590bfbf4aa8)
    at rewriteDefine.c:447
#11 0x00005590bf02b5ab in DefineRule (stmt=0x5590bfb285c0,
queryString=0x5590bfb277a8 "CREATE RULE \"_RETURN\" AS ON SELECT TO t
DO INSTEAD  SELECT * FROM t1;") at rewriteDefine.c:213

I could reproduce this issue with the following steps:
create table t1(c int);
BEGIN;
CREATE TABLE t (c int);
SAVEPOINT q;
CREATE RULE "_RETURN" AS ON SELECT TO t DO INSTEAD  SELECT * FROM t1;
select * from t;
ROLLBACK TO q;
CREATE RULE "_RETURN" AS ON SELECT TO t DO INSTEAD  SELECT * FROM t1;
ROLLBACK;

Regards,
Vignesh

On Mon, Nov 28, 2022 at 8:10 PM vignesh C <vignesh21@gmail.com> wrote:
>
> Hi,
>
> While reviewing/testing one of the patches I found the following Assert:
> #0  __pthread_kill_implementation (no_tid=0, signo=6,
> threadid=139624429171648) at ./nptl/pthread_kill.c:44
> #1  __pthread_kill_internal (signo=6, threadid=139624429171648) at
> ./nptl/pthread_kill.c:78
> #2  __GI___pthread_kill (threadid=139624429171648,
> signo=signo@entry=6) at ./nptl/pthread_kill.c:89
> #3  0x00007efcda6e3476 in __GI_raise (sig=sig@entry=6) at
> ../sysdeps/posix/raise.c:26
> #4  0x00007efcda6c97f3 in __GI_abort () at ./stdlib/abort.c:79
> #5  0x00005590bf283139 in ExceptionalCondition
> (conditionName=0x5590bf468170 "rel->pgstat_info->relation == NULL",
> fileName=0x5590bf46812b "pgstat_relation.c", lineNumber=143) at
> assert.c:66
> #6  0x00005590bf0ce5f8 in pgstat_assoc_relation (rel=0x7efcce996a48)
> at pgstat_relation.c:143
> #7  0x00005590beb83046 in initscan (scan=0x5590bfbf4af8, key=0x0,
> keep_startblock=false) at heapam.c:343
> #8  0x00005590beb8466f in heap_beginscan (relation=0x7efcce996a48,
> snapshot=0x5590bfb5a520, nkeys=0, key=0x0, parallel_scan=0x0,
> flags=449) at heapam.c:1223
> #9  0x00005590bf02af39 in table_beginscan (rel=0x7efcce996a48,
> snapshot=0x5590bfb5a520, nkeys=0, key=0x0) at
> ../../../src/include/access/tableam.h:891
> #10 0x00005590bf02bf8a in DefineQueryRewrite (rulename=0x5590bfb281d0
> "_RETURN", event_relid=16387, event_qual=0x0, event_type=CMD_SELECT,
> is_instead=true, replace=false, action=0x5590bfbf4aa8)
>     at rewriteDefine.c:447
> #11 0x00005590bf02b5ab in DefineRule (stmt=0x5590bfb285c0,
> queryString=0x5590bfb277a8 "CREATE RULE \"_RETURN\" AS ON SELECT TO t
> DO INSTEAD  SELECT * FROM t1;") at rewriteDefine.c:213
>
> I could reproduce this issue with the following steps:
> create table t1(c int);
> BEGIN;
> CREATE TABLE t (c int);
> SAVEPOINT q;
> CREATE RULE "_RETURN" AS ON SELECT TO t DO INSTEAD  SELECT * FROM t1;
> select * from t;
> ROLLBACK TO q;
> CREATE RULE "_RETURN" AS ON SELECT TO t DO INSTEAD  SELECT * FROM t1;
> ROLLBACK;
>
> Regards,
> Vignesh


I think what is happening here is that the previous relation is not
unlinked when pgstat_init_relation() is called
because the relation is now a view and for relations without storage
the relation is not unlinked in pgstat_init_relation()

void
pgstat_init_relation(Relation rel)
{
    char        relkind = rel->rd_rel->relkind;

    /*
     * We only count stats for relations with storage and partitioned tables
     */
    if (!RELKIND_HAS_STORAGE(relkind) && relkind != RELKIND_PARTITIONED_TABLE)
    {
        rel->pgstat_enabled = false;
        rel->pgstat_info = NULL;
        return;
    }

There is a logic in DefineQueryRewrite() which converts a relation to
a view when you create such a rule like the test case does.
So initially the relation had storage,  the pgstat_info is linked,
then table is converted to a view, but in init, the previous
relation is not unlinked but when it tries to link a new relation, the
assert fails saying a previous relation is already linked to
pgstat_info

I have made a small patch with a fix, but I am not sure if this is the
right way to fix this.

regards,
Ajin Cherian
Fujitsu Australia

vignesh C <vignesh21@gmail.com> writes:
> I could reproduce this issue with the following steps:
> create table t1(c int);
> BEGIN;
> CREATE TABLE t (c int);
> SAVEPOINT q;
> CREATE RULE "_RETURN" AS ON SELECT TO t DO INSTEAD  SELECT * FROM t1;
> select * from t;
> ROLLBACK TO q;
> CREATE RULE "_RETURN" AS ON SELECT TO t DO INSTEAD  SELECT * FROM t1;
> ROLLBACK;

Uh-huh.  I've not bothered to trace this in detail, but presumably
what is happening is that the first CREATE RULE converts the table
to a view, and then the ROLLBACK undoes that so far as the catalogs
are concerned, but probably doesn't undo related pg_stats state
changes fully.  Then we're in a bad state that will cause problems.
(It still crashes if you replace the second CREATE RULE with
"select * from t".)

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.

However, I'm not sure that removing that code in v15 will fly,
so maybe we need to make the new pg_stats code a little more
robust against the possibility of a relkind change.

            regards, tom lane

Hi,

On 2022-11-28 13:37:16 -0500, Tom Lane wrote:
> vignesh C <vignesh21@gmail.com> writes:
> > I could reproduce this issue with the following steps:
> > create table t1(c int);
> > BEGIN;
> > CREATE TABLE t (c int);
> > SAVEPOINT q;
> > CREATE RULE "_RETURN" AS ON SELECT TO t DO INSTEAD  SELECT * FROM t1;
> > select * from t;
> > ROLLBACK TO q;
> > CREATE RULE "_RETURN" AS ON SELECT TO t DO INSTEAD  SELECT * FROM t1;
> > ROLLBACK;
> 
> Uh-huh.  I've not bothered to trace this in detail, but presumably
> what is happening is that the first CREATE RULE converts the table
> to a view, and then the ROLLBACK undoes that so far as the catalogs
> are concerned, but probably doesn't undo related pg_stats state
> changes fully.  Then we're in a bad state that will cause problems.
> (It still crashes if you replace the second CREATE RULE with
> "select * from t".)

Yea. I haven't yet fully traced through this, but presumably relcache inval
doesn't fix this because we don't want to loose pending stats after DDL.

Perhaps we need to add a rule about not swapping pgstat* in
RelationClearRelation() when relkind changes?


> 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.


> However, I'm not sure that removing that code in v15 will fly,

Agreed, at the very least that'd increase memory usage.


> so maybe we need to make the new pg_stats code a little more
> robust against the possibility of a relkind change.

Possibly via the relcache code.

Greetings,

Andres Freund

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);

Hi,

On 2022-11-28 10:50:13 -0800, Andres Freund wrote:
> On 2022-11-28 13:37:16 -0500, Tom Lane wrote:
> > Uh-huh.  I've not bothered to trace this in detail, but presumably
> > what is happening is that the first CREATE RULE converts the table
> > to a view, and then the ROLLBACK undoes that so far as the catalogs
> > are concerned, but probably doesn't undo related pg_stats state
> > changes fully.  Then we're in a bad state that will cause problems.
> > (It still crashes if you replace the second CREATE RULE with
> > "select * from t".)
> 
> Yea. I haven't yet fully traced through this, but presumably relcache inval
> doesn't fix this because we don't want to loose pending stats after DDL.
> 
> Perhaps we need to add a rule about not swapping pgstat* in
> RelationClearRelation() when relkind changes?

Something like the attached. Still needs a bit of polish, e.g. adding the test
case from above.

I'm a bit uncomfortable adding a function call below
         * Perform swapping of the relcache entry contents.  Within this
         * process the old entry is momentarily invalid, so there *must* be no
         * possibility of CHECK_FOR_INTERRUPTS within this sequence. Do it in
         * all-in-line code for safety.
but it's not the first, see MemoryContextSetParent().


Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:
> Something like the attached. Still needs a bit of polish, e.g. adding the test
> case from above.

> I'm a bit uncomfortable adding a function call below
>          * Perform swapping of the relcache entry contents.  Within this
>          * process the old entry is momentarily invalid, so there *must* be no
>          * possibility of CHECK_FOR_INTERRUPTS within this sequence. Do it in
>          * all-in-line code for safety.

Ugh.  I don't know what pgstat_unlink_relation does, but assuming
that it can never throw an error seems like a pretty bad idea,
especially when you aren't adding that to its API spec (contrast
the comments for MemoryContextSetParent).

Can't that part be done outside the critical section?

            regards, tom lane

Hi,

On 2022-11-28 16:33:20 -0500, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > Something like the attached. Still needs a bit of polish, e.g. adding the test
> > case from above.
>
> > I'm a bit uncomfortable adding a function call below
> >          * Perform swapping of the relcache entry contents.  Within this
> >          * process the old entry is momentarily invalid, so there *must* be no
> >          * possibility of CHECK_FOR_INTERRUPTS within this sequence. Do it in
> >          * all-in-line code for safety.
>
> Ugh.  I don't know what pgstat_unlink_relation does, but assuming
> that it can never throw an error seems like a pretty bad idea,

I don't think it'd be an issue - it just resets the pointer from a pgstat
entry to the relcache entry.

But you're right:

> Can't that part be done outside the critical section?

we can do that. See the attached.


Do we have any cases of relcache entries changing their relkind?

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:
> Do we have any cases of relcache entries changing their relkind?

Just the table-to-view hack.  I'm not aware that there are any other
cases, and it seems hard to credit that there ever will be any.
I think we could get rid of table-to-view in HEAD, and use your patch
only in v15.

            regards, tom lane

Hi,

On 2022-12-02 00:08:20 -0500, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > Do we have any cases of relcache entries changing their relkind?
>
> Just the table-to-view hack.  I'm not aware that there are any other
> cases, and it seems hard to credit that there ever will be any.

I can see some halfway credible scenarios. E.g. converting a view to a
matview, or a table into a partition. I kind of wonder if it's worth keeping
the change, just in case we do - it's not that easy to hit...


> I think we could get rid of table-to-view in HEAD, and use your patch
> only in v15.

WFM. I'll push it to 15 tomorrow.

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:
> On 2022-12-02 00:08:20 -0500, Tom Lane wrote:
>> Just the table-to-view hack.  I'm not aware that there are any other
>> cases, and it seems hard to credit that there ever will be any.

> I can see some halfway credible scenarios. E.g. converting a view to a
> matview, or a table into a partition. I kind of wonder if it's worth keeping
> the change, just in case we do - it's not that easy to hit...

I'd suggest putting in an assertion that the relkind isn't changing,
instead.  When and if somebody makes a credible feature patch that'd
require relaxing that, we can see what to do.

(There's a couple of places in rewriteHandler.c that could
perhaps be simplified, too.)

            regards, tom lane

Hi,

On December 1, 2022 9:48:48 PM PST, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>Andres Freund <andres@anarazel.de> writes:
>> On 2022-12-02 00:08:20 -0500, Tom Lane wrote:
>>> Just the table-to-view hack.  I'm not aware that there are any other
>>> cases, and it seems hard to credit that there ever will be any.
>
>> I can see some halfway credible scenarios. E.g. converting a view to a
>> matview, or a table into a partition. I kind of wonder if it's worth keeping
>> the change, just in case we do - it's not that easy to hit...
>
>I'd suggest putting in an assertion that the relkind isn't changing,
>instead.

Sounds like a plan. Will you do that when you remove the table-to-view hack?

Andres
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Andres Freund <andres@anarazel.de> writes:
> On December 1, 2022 9:48:48 PM PST, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I'd suggest putting in an assertion that the relkind isn't changing,
>> instead.

> Sounds like a plan. Will you do that when you remove the table-to-view hack?

I'd suggest committing it concurrently with the v15 fix, instead,
so that there's a cross-reference to what some future hacker might
need to install if they remove the assertion.

I guess that means that the table-to-view removal has to go in
first.  I should be able to take care of that tomorrow, or if
you're in a hurry I don't mind if you commit it for me.

            regards, tom lane

Hi,

On 2022-12-02 01:03:35 -0500, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > On December 1, 2022 9:48:48 PM PST, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >> I'd suggest putting in an assertion that the relkind isn't changing,
> >> instead.
> 
> > Sounds like a plan. Will you do that when you remove the table-to-view hack? 
> 
> I'd suggest committing it concurrently with the v15 fix, instead,
> so that there's a cross-reference to what some future hacker might
> need to install if they remove the assertion.

Good idea.


> I guess that means that the table-to-view removal has to go in
> first.  I should be able to take care of that tomorrow, or if
> you're in a hurry I don't mind if you commit it for me.

No particular hurry from my end.

Greetings,

Andres Freund

I wrote:
> I guess that means that the table-to-view removal has to go in
> first.  I should be able to take care of that tomorrow, or if
> you're in a hurry I don't mind if you commit it for me.

Done now, feel free to deal with the pgstat problem.

            regards, tom lane

Hi,

On 2022-12-02 12:15:37 -0500, Tom Lane wrote:
> I wrote:
> > I guess that means that the table-to-view removal has to go in
> > first.  I should be able to take care of that tomorrow, or if
> > you're in a hurry I don't mind if you commit it for me.
> 
> Done now, feel free to deal with the pgstat problem.

Thanks.  I'm out for a few hours without proper computer access, couldn't
quite get it finished inbetween your push and now. Will deal with it once I
get back.

Greetings,

Andres Freund

Hi,

On 2022-12-02 09:51:39 -0800, Andres Freund wrote:
> On 2022-12-02 12:15:37 -0500, Tom Lane wrote:
> > I wrote:
> > > I guess that means that the table-to-view removal has to go in
> > > first.  I should be able to take care of that tomorrow, or if
> > > you're in a hurry I don't mind if you commit it for me.
> > 
> > Done now, feel free to deal with the pgstat problem.
> 
> Thanks.  I'm out for a few hours without proper computer access, couldn't
> quite get it finished inbetween your push and now. Will deal with it once I
> get back.

Pushed that now. I debated for a bit whether to backpatch the test all the
way, but after it took me a while to convince myself that there's no active
problem in the older branches, I decided it's a good idea.

Thanks Vignesh for the bugreports!

Greetings,

Andres Freund