Avoid trying to restore table ACLs and per-column ACLs in parallel.

Parallel pg_restore has always supposed that ACL items for different
objects are independent and can be restored in parallel without
conflicts.  However, there is one case where this fails: because
REVOKE on a table is defined to also revoke the privilege(s) at
column level, we can't restore per-column ACLs till after we restore
any table-level privileges on their table.  Failure to honor this
restriction can lead to "tuple concurrently updated" errors during
parallel restore, or even to the per-column ACLs silently disappearing
because the table-level REVOKE is executed afterwards.

To fix, add a dependency from each column-level ACL item to its table's
ACL item, if there is one.  Note that this doesn't fix the hazard
for pre-existing archive files, only for ones made with a corrected
pg_dump.  Given that the bug's been there quite awhile without
field reports, I think this is acceptable.

This requires changing the API of pg_dump's dumpACL() function.
To keep its argument list from getting even longer, I removed the
"CatalogId objCatId" argument, which has been unused for ages.

Per report from Justin Pryzby.  Back-patch to all supported branches.

Discussion: https://postgr.es/m/20200706050129[email protected]

diff --git a/src/bin/pg_dump/common.c b/src/bin/pg_dump/common.c
index ad8b2fd482683ab37df2083e1354d15f6efcd763..ac9315cf9673033bee6623e4b5c9dc5e2b83d83f 100644 (file)
--- a/src/bin/pg_dump/common.c
+++ b/src/bin/pg_dump/common.c
@@ -29,7 +29,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 9f6a8b869f09cd620fcdc5798374cce32f020f75..3bc32df258fa28468739e6e852665b55e04edb5b 100644 (file)
--- a/src/bin/pg_dump/pg_backup.h
+++ b/src/bin/pg_dump/pg_backup.h
@@ -222,6 +222,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 (*SetupWorkerPtr) (Archive *AH);

diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index e7a12794b608da68941994c34d175de40fee2fae..9dadf5f690a0d65c5195fc52e4188ff0ad97648d 100644 (file)
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -208,7 +208,7 @@ 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,
+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);
@@ -2915,7 +2915,7 @@ dumpBlob(Archive *fout, BlobInfo *binfo)
     * table.
     */
    if (binfo->blobacl && !fout->dopt->binary_upgrade)
-       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);
 
@@ -8609,7 +8609,7 @@ dumpNamespace(Archive *fout, NamespaceInfo *nspinfo)
                 NULL, nspinfo->rolname,
                 nspinfo->dobj.catId, 0, nspinfo->dobj.dumpId);
 
-   dumpACL(fout, nspinfo->dobj.catId, nspinfo->dobj.dumpId, "SCHEMA",
+   dumpACL(fout, nspinfo->dobj.dumpId, InvalidDumpId, "SCHEMA",
            qnspname, NULL, NULL,
            nspinfo->rolname, nspinfo->nspacl);
 
@@ -8879,7 +8879,7 @@ dumpEnumType(Archive *fout, TypeInfo *tyinfo)
                 tyinfo->dobj.namespace->dobj.name, tyinfo->rolname,
                 tyinfo->dobj.catId, 0, tyinfo->dobj.dumpId);
 
-   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);
@@ -8999,7 +8999,7 @@ dumpRangeType(Archive *fout, TypeInfo *tyinfo)
                 tyinfo->dobj.namespace->dobj.name, tyinfo->rolname,
                 tyinfo->dobj.catId, 0, tyinfo->dobj.dumpId);
 
-   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);
@@ -9065,7 +9065,7 @@ dumpUndefinedType(Archive *fout, TypeInfo *tyinfo)
                 tyinfo->dobj.namespace->dobj.name, tyinfo->rolname,
                 tyinfo->dobj.catId, 0, tyinfo->dobj.dumpId);
 
-   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);
@@ -9447,7 +9447,7 @@ dumpBaseType(Archive *fout, TypeInfo *tyinfo)
                 tyinfo->dobj.namespace->dobj.name, tyinfo->rolname,
                 tyinfo->dobj.catId, 0, tyinfo->dobj.dumpId);
 
-   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);
@@ -9598,7 +9598,7 @@ dumpDomain(Archive *fout, TypeInfo *tyinfo)
                 tyinfo->dobj.namespace->dobj.name, tyinfo->rolname,
                 tyinfo->dobj.catId, 0, tyinfo->dobj.dumpId);
 
-   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);
@@ -9812,7 +9812,7 @@ dumpCompositeType(Archive *fout, TypeInfo *tyinfo)
                 tyinfo->dobj.namespace->dobj.name, tyinfo->rolname,
                 tyinfo->dobj.catId, 0, tyinfo->dobj.dumpId);
 
-   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);
@@ -10101,7 +10101,7 @@ dumpProcLang(Archive *fout, ProcLangInfo *plang)
                 plang->dobj.catId, 0, plang->dobj.dumpId);
 
    if (plang->lanpltrusted)
-       dumpACL(fout, plang->dobj.catId, plang->dobj.dumpId, "LANGUAGE",
+       dumpACL(fout, plang->dobj.dumpId, InvalidDumpId, "LANGUAGE",
                qlanname, NULL, NULL,
                plang->lanowner, plang->lanacl);
 
@@ -10758,7 +10758,7 @@ dumpFunc(Archive *fout, FuncInfo *finfo)
                 finfo->dobj.namespace->dobj.name, finfo->rolname,
                 finfo->dobj.catId, 0, finfo->dobj.dumpId);
 
-   dumpACL(fout, finfo->dobj.catId, finfo->dobj.dumpId, "FUNCTION",
+   dumpACL(fout, finfo->dobj.dumpId, InvalidDumpId, "FUNCTION",
            funcsig, NULL,
            finfo->dobj.namespace->dobj.name,
            finfo->rolname, finfo->proacl);
@@ -12582,7 +12582,7 @@ dumpAgg(Archive *fout, AggInfo *agginfo)
 
    aggsig = format_function_signature(fout, &agginfo->aggfn, true);
 
-   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);
@@ -12974,7 +12974,7 @@ dumpForeignDataWrapper(Archive *fout, FdwInfo *fdwinfo)
                 NULL, NULL);
 
    /* Handle the 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);
@@ -13061,7 +13061,7 @@ dumpForeignServer(Archive *fout, ForeignServerInfo *srvinfo)
                 NULL, NULL);
 
    /* Handle the ACL */
-   dumpACL(fout, srvinfo->dobj.catId, srvinfo->dobj.dumpId,
+   dumpACL(fout, srvinfo->dobj.dumpId, InvalidDumpId,
            "FOREIGN SERVER", qsrvname, NULL,
            NULL, srvinfo->rolname,
            srvinfo->srvacl);
@@ -13253,8 +13253,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.
@@ -13265,24 +13266,28 @@ dumpDefaultACL(Archive *fout, DefaultACLInfo *daclinfo)
  * 'owner' is the owner, NULL if there is no owner (for languages).
  * 'acls' is the string read out of the fooacl system catalog field;
  *     it will be parsed here.
+ *
+ * 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)
 {
+   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();
 
@@ -13295,24 +13300,35 @@ 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,
                     tag->data, nspname,
                     NULL,
                     owner ? owner : "",
                     false, "ACL", SECTION_NONE,
                     sql->data, "", NULL,
-                    &(objDumpId), 1,
+                    aclDeps, nDeps,
                     NULL, NULL);
+
        destroyPQExpBuffer(tag);
    }
 
    destroyPQExpBuffer(sql);
+
+   return aclDumpId;
 }
 
 /*
@@ -13634,6 +13650,7 @@ static void
 dumpTable(Archive *fout, TableInfo *tbinfo)
 {
    DumpOptions *dopt = fout->dopt;
+   DumpId      tableAclDumpId = InvalidDumpId;
 
    if (tbinfo->dobj.dump && !dopt->dataOnly)
    {
@@ -13648,10 +13665,11 @@ dumpTable(Archive *fout, TableInfo *tbinfo)
        /* Handle the ACL here */
        namecopy = pg_strdup(fmtId(tbinfo->dobj.name));
        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);
+       tableAclDumpId =
+           dumpACL(fout, tbinfo->dobj.dumpId, InvalidDumpId,
+                   objtype, namecopy, NULL,
+                   tbinfo->dobj.namespace->dobj.name, tbinfo->rolname,
+                   tbinfo->relacl);
 
        /*
         * Handle column ACLs, if any.  Note: we pull these with a separate
@@ -13678,8 +13696,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);