Internal error while setting reloption on system catalogs. - Mailing list pgsql-hackers

Hello.

The following command complains with an internal
error. (allow_system_table_mods is on).

alter table pg_attribute set (fillfactor = 90);
> ERROR:  AccessExclusiveLock required to add toast table.

The same happens for pg_class. This is because ATRewriteCatalogs
tries to add a toast table to the relations with taking
ShareUpdateExclusiveLock. The decision whether to provide a toast
table for each system catalog is taken in the commit 96cdeae07f
that the two relations won't have a toast relation, so we should
avoid that for all mapped relations.

I didn't find a place where ATTACH PARTITION sends
RELKIND_PARTITIONED_TABLE to the function so the case is omitted
in the patch. Actually the regression test doesn't complain with
the following assertion there in the function.

> Assert(tab->relkind != RELKIND_PARTITIONED_TABLE ||
>        tab->partition_constraint == NULL ||
>        !tab->check_toast)

Many other commands seem not to need the toast check but the
patch doesn't care about them. It would be another issue.

According to alter_table.sql, it doesn't need a test.

-- XXX: It would be useful to add checks around trying to manipulate
-- catalog tables, but that might have ugly consequences when run
-- against an existing server with allow_system_table_mods = on.

The first attached is for master and 11.
The second is for 10.
The third is for 9.6.

9.4 and 9.5 doesn't suffer the problem but it is because they
create toast table for mapped relations at that time.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 35a9ade059..026ee027cd 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -159,6 +159,7 @@ typedef struct AlteredTableInfo
     List       *newvals;        /* List of NewColumnValue */
     bool        new_notnull;    /* T if we added new NOT NULL constraints */
     int            rewrite;        /* Reason for forced rewrite, if any */
+    bool        check_toast;    /* check for toast table */
     Oid            newTableSpace;    /* new tablespace; 0 means no change */
     bool        chgPersistence; /* T if SET LOGGED/UNLOGGED is used */
     char        newrelpersistence;    /* if above is true */
@@ -4051,15 +4052,7 @@ ATRewriteCatalogs(List **wqueue, LOCKMODE lockmode)
     {
         AlteredTableInfo *tab = (AlteredTableInfo *) lfirst(ltab);
 
-        /*
-         * If the table is source table of ATTACH PARTITION command, we did
-         * not modify anything about it that will change its toasting
-         * requirement, so no need to check.
-         */
-        if (((tab->relkind == RELKIND_RELATION ||
-              tab->relkind == RELKIND_PARTITIONED_TABLE) &&
-             tab->partition_constraint == NULL) ||
-            tab->relkind == RELKIND_MATVIEW)
+        if (tab->check_toast)
             AlterTableCreateToastTable(tab->relid, (Datum) 0, lockmode);
     }
 }
@@ -4917,6 +4910,15 @@ ATGetQueueEntry(List **wqueue, Relation rel)
     tab->newrelpersistence = RELPERSISTENCE_PERMANENT;
     tab->chgPersistence = false;
 
+    /* We don't check for toast table of mapped relations */
+    if ((tab->relkind == RELKIND_MATVIEW ||
+         tab->relkind == RELKIND_RELATION ||
+         tab->relkind == RELKIND_PARTITIONED_TABLE) &&
+        !RelationIsMapped(rel))
+        tab->check_toast = true;
+    else
+        tab->check_toast = false;
+
     *wqueue = lappend(*wqueue, tab);
 
     return tab;
@@ -14425,6 +14427,14 @@ QueuePartitionConstraintValidation(List **wqueue, Relation scanrel,
         Assert(tab->partition_constraint == NULL);
         tab->partition_constraint = (Expr *) linitial(partConstraint);
         tab->validate_default = validate_default;
+
+        /*
+         * If the table is source table of ATTACH PARTITION command, we did
+         * not modify anything about it that will change its toasting
+         * requirement, so no need to check.
+         */
+        Assert(tab->partition_constraint != NULL);
+        tab->check_toast = false;
     }
     else if (scanrel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
     {
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 6f1a429c7e..d238dd78da 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -164,6 +164,7 @@ typedef struct AlteredTableInfo
     List       *newvals;        /* List of NewColumnValue */
     bool        new_notnull;    /* T if we added new NOT NULL constraints */
     int            rewrite;        /* Reason for forced rewrite, if any */
+    bool        check_toast;    /* check for toast table */
     Oid            newTableSpace;    /* new tablespace; 0 means no change */
     bool        chgPersistence; /* T if SET LOGGED/UNLOGGED is used */
     char        newrelpersistence;    /* if above is true */
@@ -3793,15 +3794,7 @@ ATRewriteCatalogs(List **wqueue, LOCKMODE lockmode)
     {
         AlteredTableInfo *tab = (AlteredTableInfo *) lfirst(ltab);
 
-        /*
-         * If the table is source table of ATTACH PARTITION command, we did
-         * not modify anything about it that will change its toasting
-         * requirement, so no need to check.
-         */
-        if (((tab->relkind == RELKIND_RELATION ||
-              tab->relkind == RELKIND_PARTITIONED_TABLE) &&
-             tab->partition_constraint == NULL) ||
-            tab->relkind == RELKIND_MATVIEW)
+        if (tab->check_toast)
             AlterTableCreateToastTable(tab->relid, (Datum) 0, lockmode);
     }
 }
@@ -4653,6 +4646,15 @@ ATGetQueueEntry(List **wqueue, Relation rel)
     tab->newrelpersistence = RELPERSISTENCE_PERMANENT;
     tab->chgPersistence = false;
 
+    /* We don't check for toast table of mapped relations */
+    if ((tab->relkind == RELKIND_MATVIEW ||
+         tab->relkind == RELKIND_RELATION ||
+         tab->relkind == RELKIND_PARTITIONED_TABLE) &&
+        !RelationIsMapped(rel))
+        tab->check_toast = true;
+    else
+        tab->check_toast = false;
+
     *wqueue = lappend(*wqueue, tab);
 
     return tab;
@@ -13794,6 +13796,13 @@ ATExecAttachPartition(List **wqueue, Relation rel, PartitionCmd *cmd)
             tab = ATGetQueueEntry(wqueue, part_rel);
             tab->partition_constraint = (Expr *) linitial(my_partconstr);
 
+            /*
+             * If the table is source table of ATTACH PARTITION command, we did
+             * not modify anything about it that will change its toasting
+             * requirement, so no need to check.
+             */
+            tab->check_toast = false;
+
             /* keep our lock until commit */
             if (part_rel != attachrel)
                 heap_close(part_rel, NoLock);
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 1f7c732c02..0026442610 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -160,6 +160,7 @@ typedef struct AlteredTableInfo
     List       *newvals;        /* List of NewColumnValue */
     bool        new_notnull;    /* T if we added new NOT NULL constraints */
     int            rewrite;        /* Reason for forced rewrite, if any */
+    bool        check_toast;    /* check for toast table */
     Oid            newTableSpace;    /* new tablespace; 0 means no change */
     bool        chgPersistence; /* T if SET LOGGED/UNLOGGED is used */
     char        newrelpersistence;        /* if above is true */
@@ -3445,8 +3446,7 @@ ATRewriteCatalogs(List **wqueue, LOCKMODE lockmode)
     {
         AlteredTableInfo *tab = (AlteredTableInfo *) lfirst(ltab);
 
-        if (tab->relkind == RELKIND_RELATION ||
-            tab->relkind == RELKIND_MATVIEW)
+        if (tab->check_toast)
             AlterTableCreateToastTable(tab->relid, (Datum) 0, lockmode);
     }
 }
@@ -4272,6 +4272,14 @@ ATGetQueueEntry(List **wqueue, Relation rel)
     tab->newrelpersistence = RELPERSISTENCE_PERMANENT;
     tab->chgPersistence = false;
 
+    /* We don't check for toast table of mapped relations */
+    if ((tab->relkind == RELKIND_MATVIEW ||
+         tab->relkind == RELKIND_RELATION) &&
+        !RelationIsMapped(rel))
+        tab->check_toast = true;
+    else
+        tab->check_toast = false;
+
     *wqueue = lappend(*wqueue, tab);
 
     return tab;