Re: "tuple concurrently updated" in pg_restore --jobs

Justin Pryzby <pryzby@telsasoft.com> writes:
> On Fri, Jul 10, 2020 at 04:54:40PM -0400, Tom Lane wrote:
>> This works about 99% of the time, in fact.  It falls down in the --clean

> Note that this fails for me (sometimes) even without --clean.

Oh, I was thinking that REVOKE would only be issued in the --clean
case, but apparently that's not so.  Doesn't really affect the fix
proposal though.  I just finished a patch for HEAD, as attached.

(I flushed the "CatalogId objCatId" argument of dumpACL, which was
not used.)

I'm not sure how far to back-patch it -- I think the parallel restore
of ACLs behavior is not very old, but we might want to teach older
pg_dump versions to insert the extra dependency anyway, for safety.

            regards, tom lane

diff --git a/src/bin/pg_dump/common.c b/src/bin/pg_dump/common.c
index 4ea59f5494..08239dde4f 100644
--- a/src/bin/pg_dump/common.c
+++ b/src/bin/pg_dump/common.c
@@ -28,7 +28,7 @@
  */
 static DumpableObject **dumpIdMap = NULL;
 static int    allocedDumpIds = 0;
-static DumpId lastDumpId = 0;
+static DumpId lastDumpId = 0;    /* Note: 0 is InvalidDumpId */

 /*
  * Variables for mapping CatalogId to DumpableObject
diff --git a/src/bin/pg_dump/pg_backup.h b/src/bin/pg_dump/pg_backup.h
index b17c9dbb8b..1017abbbe5 100644
--- a/src/bin/pg_dump/pg_backup.h
+++ b/src/bin/pg_dump/pg_backup.h
@@ -233,6 +233,12 @@ typedef struct

 typedef int DumpId;

+#define InvalidDumpId 0
+
+/*
+ * Function pointer prototypes for assorted callback methods.
+ */
+
 typedef int (*DataDumperPtr) (Archive *AH, void *userArg);

 typedef void (*SetupWorkerPtrType) (Archive *AH);
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 8bca147a33..e758b5c50a 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -220,11 +220,11 @@ static void dumpUserMappings(Archive *fout,
                              const char *owner, CatalogId catalogId, DumpId dumpId);
 static void dumpDefaultACL(Archive *fout, DefaultACLInfo *daclinfo);

-static void dumpACL(Archive *fout, CatalogId objCatId, DumpId objDumpId,
-                    const char *type, const char *name, const char *subname,
-                    const char *nspname, const char *owner,
-                    const char *acls, const char *racls,
-                    const char *initacls, const char *initracls);
+static DumpId dumpACL(Archive *fout, DumpId objDumpId, DumpId altDumpId,
+                      const char *type, const char *name, const char *subname,
+                      const char *nspname, const char *owner,
+                      const char *acls, const char *racls,
+                      const char *initacls, const char *initracls);

 static void getDependencies(Archive *fout);
 static void BuildArchiveDependencies(Archive *fout);
@@ -2992,7 +2992,7 @@ dumpDatabase(Archive *fout)
      * Dump ACL if any.  Note that we do not support initial privileges
      * (pg_init_privs) on databases.
      */
-    dumpACL(fout, dbCatId, dbDumpId, "DATABASE",
+    dumpACL(fout, dbDumpId, InvalidDumpId, "DATABASE",
             qdatname, NULL, NULL,
             dba, datacl, rdatacl, "", "");

@@ -3477,7 +3477,7 @@ dumpBlob(Archive *fout, BlobInfo *binfo)

     /* Dump ACL if any */
     if (binfo->blobacl && (binfo->dobj.dump & DUMP_COMPONENT_ACL))
-        dumpACL(fout, binfo->dobj.catId, binfo->dobj.dumpId, "LARGE OBJECT",
+        dumpACL(fout, binfo->dobj.dumpId, InvalidDumpId, "LARGE OBJECT",
                 binfo->dobj.name, NULL,
                 NULL, binfo->rolname, binfo->blobacl, binfo->rblobacl,
                 binfo->initblobacl, binfo->initrblobacl);
@@ -10155,7 +10155,7 @@ dumpNamespace(Archive *fout, NamespaceInfo *nspinfo)
                      nspinfo->dobj.catId, 0, nspinfo->dobj.dumpId);

     if (nspinfo->dobj.dump & DUMP_COMPONENT_ACL)
-        dumpACL(fout, nspinfo->dobj.catId, nspinfo->dobj.dumpId, "SCHEMA",
+        dumpACL(fout, nspinfo->dobj.dumpId, InvalidDumpId, "SCHEMA",
                 qnspname, NULL, NULL,
                 nspinfo->rolname, nspinfo->nspacl, nspinfo->rnspacl,
                 nspinfo->initnspacl, nspinfo->initrnspacl);
@@ -10439,7 +10439,7 @@ dumpEnumType(Archive *fout, TypeInfo *tyinfo)
                      tyinfo->dobj.catId, 0, tyinfo->dobj.dumpId);

     if (tyinfo->dobj.dump & DUMP_COMPONENT_ACL)
-        dumpACL(fout, tyinfo->dobj.catId, tyinfo->dobj.dumpId, "TYPE",
+        dumpACL(fout, tyinfo->dobj.dumpId, InvalidDumpId, "TYPE",
                 qtypname, NULL,
                 tyinfo->dobj.namespace->dobj.name,
                 tyinfo->rolname, tyinfo->typacl, tyinfo->rtypacl,
@@ -10565,7 +10565,7 @@ dumpRangeType(Archive *fout, TypeInfo *tyinfo)
                      tyinfo->dobj.catId, 0, tyinfo->dobj.dumpId);

     if (tyinfo->dobj.dump & DUMP_COMPONENT_ACL)
-        dumpACL(fout, tyinfo->dobj.catId, tyinfo->dobj.dumpId, "TYPE",
+        dumpACL(fout, tyinfo->dobj.dumpId, InvalidDumpId, "TYPE",
                 qtypname, NULL,
                 tyinfo->dobj.namespace->dobj.name,
                 tyinfo->rolname, tyinfo->typacl, tyinfo->rtypacl,
@@ -10637,7 +10637,7 @@ dumpUndefinedType(Archive *fout, TypeInfo *tyinfo)
                      tyinfo->dobj.catId, 0, tyinfo->dobj.dumpId);

     if (tyinfo->dobj.dump & DUMP_COMPONENT_ACL)
-        dumpACL(fout, tyinfo->dobj.catId, tyinfo->dobj.dumpId, "TYPE",
+        dumpACL(fout, tyinfo->dobj.dumpId, InvalidDumpId, "TYPE",
                 qtypname, NULL,
                 tyinfo->dobj.namespace->dobj.name,
                 tyinfo->rolname, tyinfo->typacl, tyinfo->rtypacl,
@@ -10918,7 +10918,7 @@ dumpBaseType(Archive *fout, TypeInfo *tyinfo)
                      tyinfo->dobj.catId, 0, tyinfo->dobj.dumpId);

     if (tyinfo->dobj.dump & DUMP_COMPONENT_ACL)
-        dumpACL(fout, tyinfo->dobj.catId, tyinfo->dobj.dumpId, "TYPE",
+        dumpACL(fout, tyinfo->dobj.dumpId, InvalidDumpId, "TYPE",
                 qtypname, NULL,
                 tyinfo->dobj.namespace->dobj.name,
                 tyinfo->rolname, tyinfo->typacl, tyinfo->rtypacl,
@@ -11074,7 +11074,7 @@ dumpDomain(Archive *fout, TypeInfo *tyinfo)
                      tyinfo->dobj.catId, 0, tyinfo->dobj.dumpId);

     if (tyinfo->dobj.dump & DUMP_COMPONENT_ACL)
-        dumpACL(fout, tyinfo->dobj.catId, tyinfo->dobj.dumpId, "TYPE",
+        dumpACL(fout, tyinfo->dobj.dumpId, InvalidDumpId, "TYPE",
                 qtypname, NULL,
                 tyinfo->dobj.namespace->dobj.name,
                 tyinfo->rolname, tyinfo->typacl, tyinfo->rtypacl,
@@ -11296,7 +11296,7 @@ dumpCompositeType(Archive *fout, TypeInfo *tyinfo)
                      tyinfo->dobj.catId, 0, tyinfo->dobj.dumpId);

     if (tyinfo->dobj.dump & DUMP_COMPONENT_ACL)
-        dumpACL(fout, tyinfo->dobj.catId, tyinfo->dobj.dumpId, "TYPE",
+        dumpACL(fout, tyinfo->dobj.dumpId, InvalidDumpId, "TYPE",
                 qtypname, NULL,
                 tyinfo->dobj.namespace->dobj.name,
                 tyinfo->rolname, tyinfo->typacl, tyinfo->rtypacl,
@@ -11596,7 +11596,7 @@ dumpProcLang(Archive *fout, ProcLangInfo *plang)
                      plang->dobj.catId, 0, plang->dobj.dumpId);

     if (plang->lanpltrusted && plang->dobj.dump & DUMP_COMPONENT_ACL)
-        dumpACL(fout, plang->dobj.catId, plang->dobj.dumpId, "LANGUAGE",
+        dumpACL(fout, plang->dobj.dumpId, InvalidDumpId, "LANGUAGE",
                 qlanname, NULL, NULL,
                 plang->lanowner, plang->lanacl, plang->rlanacl,
                 plang->initlanacl, plang->initrlanacl);
@@ -12306,7 +12306,7 @@ dumpFunc(Archive *fout, FuncInfo *finfo)
                      finfo->dobj.catId, 0, finfo->dobj.dumpId);

     if (finfo->dobj.dump & DUMP_COMPONENT_ACL)
-        dumpACL(fout, finfo->dobj.catId, finfo->dobj.dumpId, keyword,
+        dumpACL(fout, finfo->dobj.dumpId, InvalidDumpId, keyword,
                 funcsig, NULL,
                 finfo->dobj.namespace->dobj.name,
                 finfo->rolname, finfo->proacl, finfo->rproacl,
@@ -14304,7 +14304,7 @@ dumpAgg(Archive *fout, AggInfo *agginfo)
     aggsig = format_function_signature(fout, &agginfo->aggfn, true);

     if (agginfo->aggfn.dobj.dump & DUMP_COMPONENT_ACL)
-        dumpACL(fout, agginfo->aggfn.dobj.catId, agginfo->aggfn.dobj.dumpId,
+        dumpACL(fout, agginfo->aggfn.dobj.dumpId, InvalidDumpId,
                 "FUNCTION", aggsig, NULL,
                 agginfo->aggfn.dobj.namespace->dobj.name,
                 agginfo->aggfn.rolname, agginfo->aggfn.proacl,
@@ -14706,7 +14706,7 @@ dumpForeignDataWrapper(Archive *fout, FdwInfo *fdwinfo)

     /* Handle the ACL */
     if (fdwinfo->dobj.dump & DUMP_COMPONENT_ACL)
-        dumpACL(fout, fdwinfo->dobj.catId, fdwinfo->dobj.dumpId,
+        dumpACL(fout, fdwinfo->dobj.dumpId, InvalidDumpId,
                 "FOREIGN DATA WRAPPER", qfdwname, NULL,
                 NULL, fdwinfo->rolname,
                 fdwinfo->fdwacl, fdwinfo->rfdwacl,
@@ -14795,7 +14795,7 @@ dumpForeignServer(Archive *fout, ForeignServerInfo *srvinfo)

     /* Handle the ACL */
     if (srvinfo->dobj.dump & DUMP_COMPONENT_ACL)
-        dumpACL(fout, srvinfo->dobj.catId, srvinfo->dobj.dumpId,
+        dumpACL(fout, srvinfo->dobj.dumpId, InvalidDumpId,
                 "FOREIGN SERVER", qsrvname, NULL,
                 NULL, srvinfo->rolname,
                 srvinfo->srvacl, srvinfo->rsrvacl,
@@ -14988,8 +14988,9 @@ dumpDefaultACL(Archive *fout, DefaultACLInfo *daclinfo)
 /*----------
  * Write out grant/revoke information
  *
- * 'objCatId' is the catalog ID of the underlying object.
  * 'objDumpId' is the dump ID of the underlying object.
+ * 'altDumpId' can be a second dumpId that the ACL entry must also depend on,
+ *        or InvalidDumpId if there is no need for a second dependency.
  * 'type' must be one of
  *        TABLE, SEQUENCE, FUNCTION, LANGUAGE, SCHEMA, DATABASE, TABLESPACE,
  *        FOREIGN DATA WRAPPER, SERVER, or LARGE OBJECT.
@@ -15012,25 +15013,29 @@ dumpDefaultACL(Archive *fout, DefaultACLInfo *daclinfo)
  * NB: initacls/initracls are needed because extensions can set privileges on
  * an object during the extension's script file and we record those into
  * pg_init_privs as that object's initial privileges.
+ *
+ * Returns the dump ID assigned to the ACL TocEntry, or InvalidDumpId if
+ * no ACL entry was created.
  *----------
  */
-static void
-dumpACL(Archive *fout, CatalogId objCatId, DumpId objDumpId,
+static DumpId
+dumpACL(Archive *fout, DumpId objDumpId, DumpId altDumpId,
         const char *type, const char *name, const char *subname,
         const char *nspname, const char *owner,
         const char *acls, const char *racls,
         const char *initacls, const char *initracls)
 {
+    DumpId        aclDumpId = InvalidDumpId;
     DumpOptions *dopt = fout->dopt;
     PQExpBuffer sql;

     /* Do nothing if ACL dump is not enabled */
     if (dopt->aclsSkip)
-        return;
+        return InvalidDumpId;

     /* --data-only skips ACLs *except* BLOB ACLs */
     if (dopt->dataOnly && strcmp(type, "LARGE OBJECT") != 0)
-        return;
+        return InvalidDumpId;

     sql = createPQExpBuffer();

@@ -15062,25 +15067,36 @@ dumpACL(Archive *fout, CatalogId objCatId, DumpId objDumpId,
     if (sql->len > 0)
     {
         PQExpBuffer tag = createPQExpBuffer();
+        DumpId        aclDeps[2];
+        int            nDeps = 0;

         if (subname)
             appendPQExpBuffer(tag, "COLUMN %s.%s", name, subname);
         else
             appendPQExpBuffer(tag, "%s %s", type, name);

-        ArchiveEntry(fout, nilCatalogId, createDumpId(),
+        aclDeps[nDeps++] = objDumpId;
+        if (altDumpId != InvalidDumpId)
+            aclDeps[nDeps++] = altDumpId;
+
+        aclDumpId = createDumpId();
+
+        ArchiveEntry(fout, nilCatalogId, aclDumpId,
                      ARCHIVE_OPTS(.tag = tag->data,
                                   .namespace = nspname,
                                   .owner = owner,
                                   .description = "ACL",
                                   .section = SECTION_NONE,
                                   .createStmt = sql->data,
-                                  .deps = &objDumpId,
-                                  .nDeps = 1));
+                                  .deps = aclDeps,
+                                  .nDeps = nDeps));
+
         destroyPQExpBuffer(tag);
     }

     destroyPQExpBuffer(sql);
+
+    return aclDumpId;
 }

 /*
@@ -15406,6 +15422,7 @@ static void
 dumpTable(Archive *fout, TableInfo *tbinfo)
 {
     DumpOptions *dopt = fout->dopt;
+    DumpId        tableAclDumpId = InvalidDumpId;
     char       *namecopy;

     /*
@@ -15427,11 +15444,12 @@ dumpTable(Archive *fout, TableInfo *tbinfo)
         const char *objtype =
         (tbinfo->relkind == RELKIND_SEQUENCE) ? "SEQUENCE" : "TABLE";

-        dumpACL(fout, tbinfo->dobj.catId, tbinfo->dobj.dumpId,
-                objtype, namecopy, NULL,
-                tbinfo->dobj.namespace->dobj.name, tbinfo->rolname,
-                tbinfo->relacl, tbinfo->rrelacl,
-                tbinfo->initrelacl, tbinfo->initrrelacl);
+        tableAclDumpId =
+            dumpACL(fout, tbinfo->dobj.dumpId, InvalidDumpId,
+                    objtype, namecopy, NULL,
+                    tbinfo->dobj.namespace->dobj.name, tbinfo->rolname,
+                    tbinfo->relacl, tbinfo->rrelacl,
+                    tbinfo->initrelacl, tbinfo->initrrelacl);
     }

     /*
@@ -15515,8 +15533,13 @@ dumpTable(Archive *fout, TableInfo *tbinfo)
             char       *attnamecopy;

             attnamecopy = pg_strdup(fmtId(attname));
-            /* Column's GRANT type is always TABLE */
-            dumpACL(fout, tbinfo->dobj.catId, tbinfo->dobj.dumpId,
+
+            /*
+             * Column's GRANT type is always TABLE.  Each column ACL depends
+             * on the table-level ACL, since we can restore column ACLs in
+             * parallel but the table-level ACL has to be done first.
+             */
+            dumpACL(fout, tbinfo->dobj.dumpId, tableAclDumpId,
                     "TABLE", namecopy, attnamecopy,
                     tbinfo->dobj.namespace->dobj.name, tbinfo->rolname,
                     attacl, rattacl, initattacl, initrattacl);