Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1ju0h9-0005kO-7m for pgsql-hackers@arkaria.postgresql.org; Fri, 10 Jul 2020 21:36:35 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.92) (envelope-from ) id 1ju0h8-0004fh-6B for pgsql-hackers@arkaria.postgresql.org; Fri, 10 Jul 2020 21:36:34 +0000 Received: from magus.postgresql.org ([2a02:c0:301:0:ffff::29]) by malur.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1ju0h7-0004fZ-Tt for pgsql-hackers@lists.postgresql.org; Fri, 10 Jul 2020 21:36:33 +0000 Received: from sss.pgh.pa.us ([66.207.139.130]) by magus.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1ju0h5-0007OY-1c for pgsql-hackers@lists.postgresql.org; Fri, 10 Jul 2020 21:36:33 +0000 Received: from sss1.sss.pgh.pa.us (localhost [127.0.0.1]) by sss.pgh.pa.us (8.15.2/8.15.2) with ESMTP id 06ALaSdR2164431; Fri, 10 Jul 2020 17:36:28 -0400 From: Tom Lane To: Justin Pryzby cc: David Gilman , pgsql-hackers@lists.postgresql.org Subject: Re: "tuple concurrently updated" in pg_restore --jobs In-reply-to: <20200710210607.GR26220@telsasoft.com> References: <20200706050129.GW4107@telsasoft.com> <2116463.1594414480@sss.pgh.pa.us> <20200710210607.GR26220@telsasoft.com> Comments: In-reply-to Justin Pryzby message dated "Fri, 10 Jul 2020 16:06:07 -0500" MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0" Content-ID: <2164346.1594416888.0@sss.pgh.pa.us> Date: Fri, 10 Jul 2020 17:36:28 -0400 Message-ID: <2164430.1594416988@sss.pgh.pa.us> List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Precedence: bulk ------- =_aaaaaaaaaa0 Content-Type: text/plain; charset="us-ascii" Content-ID: <2164346.1594416888.1@sss.pgh.pa.us> Justin Pryzby 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 ------- =_aaaaaaaaaa0 Content-Type: text/x-diff; name="make-column-ACLs-depend-on-table-ACLs-1.patch"; charset="us-ascii" Content-ID: <2164346.1594416888.2@sss.pgh.pa.us> Content-Description: make-column-ACLs-depend-on-table-ACLs-1.patch Content-Transfer-Encoding: quoted-printable 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 =3D NULL; static int allocedDumpIds =3D 0; -static DumpId lastDumpId =3D 0; +static DumpId lastDumpId =3D 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 *nspinf= o) 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 =3D 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 *fdw= info) = /* 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 *dacl= info) /*---------- * 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 *da= clinfo) * NB: initacls/initracls are needed because extensions can set privilege= s 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 =3D InvalidDumpId; DumpOptions *dopt =3D 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") !=3D 0) - return; + return InvalidDumpId; = sql =3D createPQExpBuffer(); = @@ -15062,25 +15067,36 @@ dumpACL(Archive *fout, CatalogId objCatId, DumpI= d objDumpId, if (sql->len > 0) { PQExpBuffer tag =3D createPQExpBuffer(); + DumpId aclDeps[2]; + int nDeps =3D 0; = if (subname) appendPQExpBuffer(tag, "COLUMN %s.%s", name, subname); else appendPQExpBuffer(tag, "%s %s", type, name); = - ArchiveEntry(fout, nilCatalogId, createDumpId(), + aclDeps[nDeps++] =3D objDumpId; + if (altDumpId !=3D InvalidDumpId) + aclDeps[nDeps++] =3D altDumpId; + + aclDumpId =3D createDumpId(); + + ArchiveEntry(fout, nilCatalogId, aclDumpId, ARCHIVE_OPTS(.tag =3D tag->data, .namespace =3D nspname, .owner =3D owner, .description =3D "ACL", .section =3D SECTION_NONE, .createStmt =3D sql->data, - .deps =3D &objDumpId, - .nDeps =3D 1)); + .deps =3D aclDeps, + .nDeps =3D nDeps)); + destroyPQExpBuffer(tag); } = destroyPQExpBuffer(sql); + + return aclDumpId; } = /* @@ -15406,6 +15422,7 @@ static void dumpTable(Archive *fout, TableInfo *tbinfo) { DumpOptions *dopt =3D fout->dopt; + DumpId tableAclDumpId =3D InvalidDumpId; char *namecopy; = /* @@ -15427,11 +15444,12 @@ dumpTable(Archive *fout, TableInfo *tbinfo) const char *objtype =3D (tbinfo->relkind =3D=3D 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 =3D + 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 =3D 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); ------- =_aaaaaaaaaa0--