Skip to content

Commit 4fdc559

Browse files
tglsfdctglsfdc
committed
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
1 parent 0ecd36e commit 4fdc559

File tree

3 files changed

+59
-30
lines changed

3 files changed

+59
-30
lines changed

‎src/bin/pg_dump/common.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@
3030
*/
3131
static DumpableObject **dumpIdMap = NULL;
3232
static int allocedDumpIds = 0;
33-
static DumpId lastDumpId = 0;
33+
static DumpId lastDumpId = 0; /* Note: 0 is InvalidDumpId */
3434

3535
/*
3636
* Variables for mapping CatalogId to DumpableObject

‎src/bin/pg_dump/pg_backup.h

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -234,6 +234,12 @@ typedef struct
234234

235235
typedef int DumpId;
236236

237+
#define InvalidDumpId 0
238+
239+
/*
240+
* Function pointer prototypes for assorted callback methods.
241+
*/
242+
237243
typedef int (*DataDumperPtr) (Archive *AH, void *userArg);
238244

239245
typedef void (*SetupWorkerPtrType) (Archive *AH);

‎src/bin/pg_dump/pg_dump.c

Lines changed: 52 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -216,7 +216,7 @@ static void dumpUserMappings(Archive *fout,
216216
const char *owner, CatalogId catalogId, DumpId dumpId);
217217
static void dumpDefaultACL(Archive *fout, DefaultACLInfo *daclinfo);
218218

219-
static void dumpACL(Archive *fout, CatalogId objCatId, DumpId objDumpId,
219+
static DumpId dumpACL(Archive *fout, DumpId objDumpId, DumpId altDumpId,
220220
const char *type, const char *name, const char *subname,
221221
const char *nspname, const char *owner,
222222
const char *acls, const char *racls,
@@ -2827,7 +2827,7 @@ dumpDatabase(Archive *fout)
28272827
* Dump ACL if any. Note that we do not support initial privileges
28282828
* (pg_init_privs) on databases.
28292829
*/
2830-
dumpACL(fout, dbCatId, dbDumpId, "DATABASE",
2830+
dumpACL(fout, dbDumpId, InvalidDumpId, "DATABASE",
28312831
qdatname, NULL, NULL,
28322832
dba, datacl, rdatacl, "", "");
28332833

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

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

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

1032110321
if (tyinfo->dobj.dump & DUMP_COMPONENT_ACL)
10322-
dumpACL(fout, tyinfo->dobj.catId, tyinfo->dobj.dumpId, "TYPE",
10322+
dumpACL(fout, tyinfo->dobj.dumpId, InvalidDumpId, "TYPE",
1032310323
qtypname, NULL,
1032410324
tyinfo->dobj.namespace->dobj.name,
1032510325
tyinfo->rolname, tyinfo->typacl, tyinfo->rtypacl,
@@ -10446,7 +10446,7 @@ dumpRangeType(Archive *fout, TypeInfo *tyinfo)
1044610446
tyinfo->dobj.catId, 0, tyinfo->dobj.dumpId);
1044710447

1044810448
if (tyinfo->dobj.dump & DUMP_COMPONENT_ACL)
10449-
dumpACL(fout, tyinfo->dobj.catId, tyinfo->dobj.dumpId, "TYPE",
10449+
dumpACL(fout, tyinfo->dobj.dumpId, InvalidDumpId, "TYPE",
1045010450
qtypname, NULL,
1045110451
tyinfo->dobj.namespace->dobj.name,
1045210452
tyinfo->rolname, tyinfo->typacl, tyinfo->rtypacl,
@@ -10519,7 +10519,7 @@ dumpUndefinedType(Archive *fout, TypeInfo *tyinfo)
1051910519
tyinfo->dobj.catId, 0, tyinfo->dobj.dumpId);
1052010520

1052110521
if (tyinfo->dobj.dump & DUMP_COMPONENT_ACL)
10522-
dumpACL(fout, tyinfo->dobj.catId, tyinfo->dobj.dumpId, "TYPE",
10522+
dumpACL(fout, tyinfo->dobj.dumpId, InvalidDumpId, "TYPE",
1052310523
qtypname, NULL,
1052410524
tyinfo->dobj.namespace->dobj.name,
1052510525
tyinfo->rolname, tyinfo->typacl, tyinfo->rtypacl,
@@ -10801,7 +10801,7 @@ dumpBaseType(Archive *fout, TypeInfo *tyinfo)
1080110801
tyinfo->dobj.catId, 0, tyinfo->dobj.dumpId);
1080210802

1080310803
if (tyinfo->dobj.dump & DUMP_COMPONENT_ACL)
10804-
dumpACL(fout, tyinfo->dobj.catId, tyinfo->dobj.dumpId, "TYPE",
10804+
dumpACL(fout, tyinfo->dobj.dumpId, InvalidDumpId, "TYPE",
1080510805
qtypname, NULL,
1080610806
tyinfo->dobj.namespace->dobj.name,
1080710807
tyinfo->rolname, tyinfo->typacl, tyinfo->rtypacl,
@@ -10958,7 +10958,7 @@ dumpDomain(Archive *fout, TypeInfo *tyinfo)
1095810958
tyinfo->dobj.catId, 0, tyinfo->dobj.dumpId);
1095910959

1096010960
if (tyinfo->dobj.dump & DUMP_COMPONENT_ACL)
10961-
dumpACL(fout, tyinfo->dobj.catId, tyinfo->dobj.dumpId, "TYPE",
10961+
dumpACL(fout, tyinfo->dobj.dumpId, InvalidDumpId, "TYPE",
1096210962
qtypname, NULL,
1096310963
tyinfo->dobj.namespace->dobj.name,
1096410964
tyinfo->rolname, tyinfo->typacl, tyinfo->rtypacl,
@@ -11181,7 +11181,7 @@ dumpCompositeType(Archive *fout, TypeInfo *tyinfo)
1118111181
tyinfo->dobj.catId, 0, tyinfo->dobj.dumpId);
1118211182

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

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

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

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

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

1467814678
/* Handle the ACL */
1467914679
if (srvinfo->dobj.dump & DUMP_COMPONENT_ACL)
14680-
dumpACL(fout, srvinfo->dobj.catId, srvinfo->dobj.dumpId,
14680+
dumpACL(fout, srvinfo->dobj.dumpId, InvalidDumpId,
1468114681
"FOREIGN SERVER", qsrvname, NULL,
1468214682
NULL, srvinfo->rolname,
1468314683
srvinfo->srvacl, srvinfo->rsrvacl,
@@ -14873,8 +14873,9 @@ dumpDefaultACL(Archive *fout, DefaultACLInfo *daclinfo)
1487314873
/*----------
1487414874
* Write out grant/revoke information
1487514875
*
14876-
* 'objCatId' is the catalog ID of the underlying object.
1487714876
* 'objDumpId' is the dump ID of the underlying object.
14877+
* 'altDumpId' can be a second dumpId that the ACL entry must also depend on,
14878+
* or InvalidDumpId if there is no need for a second dependency.
1487814879
* 'type' must be one of
1487914880
* TABLE, SEQUENCE, FUNCTION, LANGUAGE, SCHEMA, DATABASE, TABLESPACE,
1488014881
* FOREIGN DATA WRAPPER, SERVER, or LARGE OBJECT.
@@ -14897,25 +14898,29 @@ dumpDefaultACL(Archive *fout, DefaultACLInfo *daclinfo)
1489714898
* NB: initacls/initracls are needed because extensions can set privileges on
1489814899
* an object during the extension's script file and we record those into
1489914900
* pg_init_privs as that object's initial privileges.
14901+
*
14902+
* Returns the dump ID assigned to the ACL TocEntry, or InvalidDumpId if
14903+
* no ACL entry was created.
1490014904
*----------
1490114905
*/
14902-
static void
14903-
dumpACL(Archive *fout, CatalogId objCatId, DumpId objDumpId,
14906+
static DumpId
14907+
dumpACL(Archive *fout, DumpId objDumpId, DumpId altDumpId,
1490414908
const char *type, const char *name, const char *subname,
1490514909
const char *nspname, const char *owner,
1490614910
const char *acls, const char *racls,
1490714911
const char *initacls, const char *initracls)
1490814912
{
14913+
DumpId aclDumpId = InvalidDumpId;
1490914914
DumpOptions *dopt = fout->dopt;
1491014915
PQExpBuffer sql;
1491114916

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

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

1492014925
sql = createPQExpBuffer();
1492114926

@@ -14949,24 +14954,35 @@ dumpACL(Archive *fout, CatalogId objCatId, DumpId objDumpId,
1494914954
if (sql->len > 0)
1495014955
{
1495114956
PQExpBuffer tag = createPQExpBuffer();
14957+
DumpId aclDeps[2];
14958+
int nDeps = 0;
1495214959

1495314960
if (subname)
1495414961
appendPQExpBuffer(tag, "COLUMN %s.%s", name, subname);
1495514962
else
1495614963
appendPQExpBuffer(tag, "%s %s", type, name);
1495714964

14958-
ArchiveEntry(fout, nilCatalogId, createDumpId(),
14965+
aclDeps[nDeps++] = objDumpId;
14966+
if (altDumpId != InvalidDumpId)
14967+
aclDeps[nDeps++] = altDumpId;
14968+
14969+
aclDumpId = createDumpId();
14970+
14971+
ArchiveEntry(fout, nilCatalogId, aclDumpId,
1495914972
tag->data, nspname,
1496014973
NULL,
1496114974
owner ? owner : "",
1496214975
false, "ACL", SECTION_NONE,
1496314976
sql->data, "", NULL,
14964-
&(objDumpId), 1,
14977+
aclDeps, nDeps,
1496514978
NULL, NULL);
14979+
1496614980
destroyPQExpBuffer(tag);
1496714981
}
1496814982

1496914983
destroyPQExpBuffer(sql);
14984+
14985+
return aclDumpId;
1497014986
}
1497114987

1497214988
/*
@@ -15288,6 +15304,7 @@ static void
1528815304
dumpTable(Archive *fout, TableInfo *tbinfo)
1528915305
{
1529015306
DumpOptions *dopt = fout->dopt;
15307+
DumpId tableAclDumpId = InvalidDumpId;
1529115308
char *namecopy;
1529215309

1529315310
/*
@@ -15309,11 +15326,12 @@ dumpTable(Archive *fout, TableInfo *tbinfo)
1530915326
const char *objtype =
1531015327
(tbinfo->relkind == RELKIND_SEQUENCE) ? "SEQUENCE" : "TABLE";
1531115328

15312-
dumpACL(fout, tbinfo->dobj.catId, tbinfo->dobj.dumpId,
15313-
objtype, namecopy, NULL,
15314-
tbinfo->dobj.namespace->dobj.name, tbinfo->rolname,
15315-
tbinfo->relacl, tbinfo->rrelacl,
15316-
tbinfo->initrelacl, tbinfo->initrrelacl);
15329+
tableAclDumpId =
15330+
dumpACL(fout, tbinfo->dobj.dumpId, InvalidDumpId,
15331+
objtype, namecopy, NULL,
15332+
tbinfo->dobj.namespace->dobj.name, tbinfo->rolname,
15333+
tbinfo->relacl, tbinfo->rrelacl,
15334+
tbinfo->initrelacl, tbinfo->initrrelacl);
1531715335
}
1531815336

1531915337
/*
@@ -15397,8 +15415,13 @@ dumpTable(Archive *fout, TableInfo *tbinfo)
1539715415
char *attnamecopy;
1539815416

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

0 commit comments

Comments
 (0)