Re: pg_restore causing deadlocks on partitioned tables - Mailing list pgsql-hackers

I wrote:
>> (2) ALTER TABLE ONLY ... ADD CONSTRAINT on a partition root tries to get
>> AccessExclusiveLock on all child partitions, despite the ONLY.

> The cause of this seems to be that ATPrepSetNotNull is too dumb to
> avoid recursing to all the child tables when the parent is already
> attnotnull.  Or is there a reason we have to recurse anyway?

I wrote a quick patch for this part.  It seems pretty safe and probably
could be back-patched without fear.  (I also noticed that
ATSimpleRecursion is being unnecessarily stupid: instead of the
demonstrably not-future-proof relkind check, it could test relhassubclass,
which is not only simpler and less likely to need future changes, but
is able to save a scan of pg_inherits in a lot more cases.)

As far as I can tell in some quick testing, this fix is sufficient to
resolve the complained-of deadlock.  It'd still be a good idea to fix the
TRUNCATE side of things as well.  But that would be hard to back-patch
because removing ri_PartitionCheck, or even just failing to fill it,
seems like a potential ABI break for extensions.  So my proposal is
to back-patch this, but address the ResultRelInfo change only in HEAD.

            regards, tom lane

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index c21a309f04..4a51d79d09 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -5681,14 +5681,10 @@ ATSimpleRecursion(List **wqueue, Relation rel,
                   AlterTableUtilityContext *context)
 {
     /*
-     * Propagate to children if desired.  Only plain tables, foreign tables
-     * and partitioned tables have children, so no need to search for other
-     * relkinds.
+     * Propagate to children, if desired and if there are (or might be) any
+     * children.
      */
-    if (recurse &&
-        (rel->rd_rel->relkind == RELKIND_RELATION ||
-         rel->rd_rel->relkind == RELKIND_FOREIGN_TABLE ||
-         rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE))
+    if (recurse && rel->rd_rel->relhassubclass)
     {
         Oid            relid = RelationGetRelid(rel);
         ListCell   *child;
@@ -6698,6 +6694,35 @@ ATPrepSetNotNull(List **wqueue, Relation rel,
     if (recursing)
         return;

+    /*
+     * If the target column is already marked NOT NULL, we can skip recursing
+     * to children, because their columns should already be marked NOT NULL as
+     * well.  But there's no point in checking here unless the relation has
+     * some children; else we can just wait till execution to check.  (If it
+     * does have children, however, this can save taking per-child locks
+     * unnecessarily.  This greatly improves concurrency in some parallel
+     * restore scenarios.)
+     */
+    if (rel->rd_rel->relhassubclass)
+    {
+        HeapTuple    tuple;
+        bool        attnotnull;
+
+        tuple = SearchSysCacheAttName(RelationGetRelid(rel), cmd->name);
+
+        /* Might as well throw the error now, if name is bad */
+        if (!HeapTupleIsValid(tuple))
+            ereport(ERROR,
+                    (errcode(ERRCODE_UNDEFINED_COLUMN),
+                     errmsg("column \"%s\" of relation \"%s\" does not exist",
+                            cmd->name, RelationGetRelationName(rel))));
+
+        attnotnull = ((Form_pg_attribute) GETSTRUCT(tuple))->attnotnull;
+        ReleaseSysCache(tuple);
+        if (attnotnull)
+            return;
+    }
+
     /*
      * If we have ALTER TABLE ONLY ... SET NOT NULL on a partitioned table,
      * apply ALTER TABLE ... CHECK NOT NULL to every child.  Otherwise, use