From: Tom Lane Date: Sat, 11 Jul 2020 17:36:51 +0000 (-0400) Subject: Avoid trying to restore table ACLs and per-column ACLs in parallel. X-Git-Tag: REL9_5_23~25 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=db820b9a9d004213b7a372dc0ba2e014ce1d2a73;p=thirdparty%2Fpostgresql.git 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 ad8b2fd4826..ac9315cf967 100644 --- 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 9f6a8b869f0..3bc32df258f 100644 --- 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 e7a12794b60..9dadf5f690a 100644 --- 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);