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.GW4107@telsasoft.com

diff --git a/src/bin/pg_dump/common.c b/src/bin/pg_dump/common.c
index 54eaa5ef539e77575095113c6e75a94d713d7a54..9304b9b2517534664c404f6a529cdec16c8d8956 100644 (file)
--- a/src/bin/pg_dump/common.c
+++ b/src/bin/pg_dump/common.c
@@ -30,7 +30,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 560de018844f9f4e030303053fcc39a7b82403b2..1b7d943bbb5471696494715e88dfbce2766bd2af 100644 (file)
--- a/src/bin/pg_dump/pg_backup.h
+++ b/src/bin/pg_dump/pg_backup.h
@@ -231,6 +231,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 bd14d387403ea05f94106a70dae0f1330f70923f..dd98b1776197ef807dc5d3643215c403ed3980ba 100644 (file)
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -216,7 +216,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, const char *racls,
@@ -3080,7 +3080,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);
@@ -9571,7 +9571,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);
@@ -9851,7 +9851,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,
@@ -9977,7 +9977,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,
@@ -10049,7 +10049,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,
@@ -10327,7 +10327,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,
@@ -10483,7 +10483,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,
@@ -10705,7 +10705,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,
@@ -11000,7 +11000,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);
@@ -11645,7 +11645,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, "FUNCTION",
+               dumpACL(fout, finfo->dobj.dumpId, InvalidDumpId, "FUNCTION",
                                funcsig, NULL,
                                finfo->dobj.namespace->dobj.name,
                                finfo->rolname, finfo->proacl, finfo->rproacl,
@@ -13580,7 +13580,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,
@@ -13984,7 +13984,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,
@@ -14075,7 +14075,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,
@@ -14277,8 +14277,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.
@@ -14301,25 +14302,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();
 
@@ -14353,24 +14358,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;
 }
 
 /*
@@ -14692,6 +14708,7 @@ static void
 dumpTable(Archive *fout, TableInfo *tbinfo)
 {
        DumpOptions *dopt = fout->dopt;
+       DumpId          tableAclDumpId = InvalidDumpId;
        char       *namecopy;
 
        /*
@@ -14713,11 +14730,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);
        }
 
        /*
@@ -14801,8 +14819,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);