Skip to content

Commit 2d43c30

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 da76f05 commit 2d43c30

File tree

3 files changed

+58
-29
lines changed

3 files changed

+58
-29
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
@@ -231,6 +231,12 @@ typedef struct
231231

232232
typedef int DumpId;
233233

234+
#define InvalidDumpId 0
235+
236+
/*
237+
* Function pointer prototypes for assorted callback methods.
238+
*/
239+
234240
typedef int (*DataDumperPtr) (Archive *AH, void *userArg);
235241

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

‎src/bin/pg_dump/pg_dump.c

Lines changed: 51 additions & 28 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,
@@ -3080,7 +3080,7 @@ dumpBlob(Archive *fout, BlobInfo *binfo)
30803080

30813081
/* Dump ACL if any */
30823082
if (binfo->blobacl && (binfo->dobj.dump & DUMP_COMPONENT_ACL))
3083-
dumpACL(fout, binfo->dobj.catId, binfo->dobj.dumpId, "LARGE OBJECT",
3083+
dumpACL(fout, binfo->dobj.dumpId, InvalidDumpId, "LARGE OBJECT",
30843084
binfo->dobj.name, NULL,
30853085
NULL, binfo->rolname, binfo->blobacl, binfo->rblobacl,
30863086
binfo->initblobacl, binfo->initrblobacl);
@@ -9571,7 +9571,7 @@ dumpNamespace(Archive *fout, NamespaceInfo *nspinfo)
95719571
nspinfo->dobj.catId, 0, nspinfo->dobj.dumpId);
95729572

95739573
if (nspinfo->dobj.dump & DUMP_COMPONENT_ACL)
9574-
dumpACL(fout, nspinfo->dobj.catId, nspinfo->dobj.dumpId, "SCHEMA",
9574+
dumpACL(fout, nspinfo->dobj.dumpId, InvalidDumpId, "SCHEMA",
95759575
qnspname, NULL, NULL,
95769576
nspinfo->rolname, nspinfo->nspacl, nspinfo->rnspacl,
95779577
nspinfo->initnspacl, nspinfo->initrnspacl);
@@ -9851,7 +9851,7 @@ dumpEnumType(Archive *fout, TypeInfo *tyinfo)
98519851
tyinfo->dobj.catId, 0, tyinfo->dobj.dumpId);
98529852

98539853
if (tyinfo->dobj.dump & DUMP_COMPONENT_ACL)
9854-
dumpACL(fout, tyinfo->dobj.catId, tyinfo->dobj.dumpId, "TYPE",
9854+
dumpACL(fout, tyinfo->dobj.dumpId, InvalidDumpId, "TYPE",
98559855
qtypname, NULL,
98569856
tyinfo->dobj.namespace->dobj.name,
98579857
tyinfo->rolname, tyinfo->typacl, tyinfo->rtypacl,
@@ -9977,7 +9977,7 @@ dumpRangeType(Archive *fout, TypeInfo *tyinfo)
99779977
tyinfo->dobj.catId, 0, tyinfo->dobj.dumpId);
99789978

99799979
if (tyinfo->dobj.dump & DUMP_COMPONENT_ACL)
9980-
dumpACL(fout, tyinfo->dobj.catId, tyinfo->dobj.dumpId, "TYPE",
9980+
dumpACL(fout, tyinfo->dobj.dumpId, InvalidDumpId, "TYPE",
99819981
qtypname, NULL,
99829982
tyinfo->dobj.namespace->dobj.name,
99839983
tyinfo->rolname, tyinfo->typacl, tyinfo->rtypacl,
@@ -10049,7 +10049,7 @@ dumpUndefinedType(Archive *fout, TypeInfo *tyinfo)
1004910049
tyinfo->dobj.catId, 0, tyinfo->dobj.dumpId);
1005010050

1005110051
if (tyinfo->dobj.dump & DUMP_COMPONENT_ACL)
10052-
dumpACL(fout, tyinfo->dobj.catId, tyinfo->dobj.dumpId, "TYPE",
10052+
dumpACL(fout, tyinfo->dobj.dumpId, InvalidDumpId, "TYPE",
1005310053
qtypname, NULL,
1005410054
tyinfo->dobj.namespace->dobj.name,
1005510055
tyinfo->rolname, tyinfo->typacl, tyinfo->rtypacl,
@@ -10327,7 +10327,7 @@ dumpBaseType(Archive *fout, TypeInfo *tyinfo)
1032710327
tyinfo->dobj.catId, 0, tyinfo->dobj.dumpId);
1032810328

1032910329
if (tyinfo->dobj.dump & DUMP_COMPONENT_ACL)
10330-
dumpACL(fout, tyinfo->dobj.catId, tyinfo->dobj.dumpId, "TYPE",
10330+
dumpACL(fout, tyinfo->dobj.dumpId, InvalidDumpId, "TYPE",
1033110331
qtypname, NULL,
1033210332
tyinfo->dobj.namespace->dobj.name,
1033310333
tyinfo->rolname, tyinfo->typacl, tyinfo->rtypacl,
@@ -10483,7 +10483,7 @@ dumpDomain(Archive *fout, TypeInfo *tyinfo)
1048310483
tyinfo->dobj.catId, 0, tyinfo->dobj.dumpId);
1048410484

1048510485
if (tyinfo->dobj.dump & DUMP_COMPONENT_ACL)
10486-
dumpACL(fout, tyinfo->dobj.catId, tyinfo->dobj.dumpId, "TYPE",
10486+
dumpACL(fout, tyinfo->dobj.dumpId, InvalidDumpId, "TYPE",
1048710487
qtypname, NULL,
1048810488
tyinfo->dobj.namespace->dobj.name,
1048910489
tyinfo->rolname, tyinfo->typacl, tyinfo->rtypacl,
@@ -10705,7 +10705,7 @@ dumpCompositeType(Archive *fout, TypeInfo *tyinfo)
1070510705
tyinfo->dobj.catId, 0, tyinfo->dobj.dumpId);
1070610706

1070710707
if (tyinfo->dobj.dump & DUMP_COMPONENT_ACL)
10708-
dumpACL(fout, tyinfo->dobj.catId, tyinfo->dobj.dumpId, "TYPE",
10708+
dumpACL(fout, tyinfo->dobj.dumpId, InvalidDumpId, "TYPE",
1070910709
qtypname, NULL,
1071010710
tyinfo->dobj.namespace->dobj.name,
1071110711
tyinfo->rolname, tyinfo->typacl, tyinfo->rtypacl,
@@ -11000,7 +11000,7 @@ dumpProcLang(Archive *fout, ProcLangInfo *plang)
1100011000
plang->dobj.catId, 0, plang->dobj.dumpId);
1100111001

1100211002
if (plang->lanpltrusted && plang->dobj.dump & DUMP_COMPONENT_ACL)
11003-
dumpACL(fout, plang->dobj.catId, plang->dobj.dumpId, "LANGUAGE",
11003+
dumpACL(fout, plang->dobj.dumpId, InvalidDumpId, "LANGUAGE",
1100411004
qlanname, NULL, NULL,
1100511005
plang->lanowner, plang->lanacl, plang->rlanacl,
1100611006
plang->initlanacl, plang->initrlanacl);
@@ -11645,7 +11645,7 @@ dumpFunc(Archive *fout, FuncInfo *finfo)
1164511645
finfo->dobj.catId, 0, finfo->dobj.dumpId);
1164611646

1164711647
if (finfo->dobj.dump & DUMP_COMPONENT_ACL)
11648-
dumpACL(fout, finfo->dobj.catId, finfo->dobj.dumpId, "FUNCTION",
11648+
dumpACL(fout, finfo->dobj.dumpId, InvalidDumpId, "FUNCTION",
1164911649
funcsig, NULL,
1165011650
finfo->dobj.namespace->dobj.name,
1165111651
finfo->rolname, finfo->proacl, finfo->rproacl,
@@ -13580,7 +13580,7 @@ dumpAgg(Archive *fout, AggInfo *agginfo)
1358013580
aggsig = format_function_signature(fout, &agginfo->aggfn, true);
1358113581

1358213582
if (agginfo->aggfn.dobj.dump & DUMP_COMPONENT_ACL)
13583-
dumpACL(fout, agginfo->aggfn.dobj.catId, agginfo->aggfn.dobj.dumpId,
13583+
dumpACL(fout, agginfo->aggfn.dobj.dumpId, InvalidDumpId,
1358413584
"FUNCTION", aggsig, NULL,
1358513585
agginfo->aggfn.dobj.namespace->dobj.name,
1358613586
agginfo->aggfn.rolname, agginfo->aggfn.proacl,
@@ -13984,7 +13984,7 @@ dumpForeignDataWrapper(Archive *fout, FdwInfo *fdwinfo)
1398413984

1398513985
/* Handle the ACL */
1398613986
if (fdwinfo->dobj.dump & DUMP_COMPONENT_ACL)
13987-
dumpACL(fout, fdwinfo->dobj.catId, fdwinfo->dobj.dumpId,
13987+
dumpACL(fout, fdwinfo->dobj.dumpId, InvalidDumpId,
1398813988
"FOREIGN DATA WRAPPER", qfdwname, NULL,
1398913989
NULL, fdwinfo->rolname,
1399013990
fdwinfo->fdwacl, fdwinfo->rfdwacl,
@@ -14075,7 +14075,7 @@ dumpForeignServer(Archive *fout, ForeignServerInfo *srvinfo)
1407514075

1407614076
/* Handle the ACL */
1407714077
if (srvinfo->dobj.dump & DUMP_COMPONENT_ACL)
14078-
dumpACL(fout, srvinfo->dobj.catId, srvinfo->dobj.dumpId,
14078+
dumpACL(fout, srvinfo->dobj.dumpId, InvalidDumpId,
1407914079
"FOREIGN SERVER", qsrvname, NULL,
1408014080
NULL, srvinfo->rolname,
1408114081
srvinfo->srvacl, srvinfo->rsrvacl,
@@ -14277,8 +14277,9 @@ dumpDefaultACL(Archive *fout, DefaultACLInfo *daclinfo)
1427714277
/*----------
1427814278
* Write out grant/revoke information
1427914279
*
14280-
* 'objCatId' is the catalog ID of the underlying object.
1428114280
* 'objDumpId' is the dump ID of the underlying object.
14281+
* 'altDumpId' can be a second dumpId that the ACL entry must also depend on,
14282+
* or InvalidDumpId if there is no need for a second dependency.
1428214283
* 'type' must be one of
1428314284
* TABLE, SEQUENCE, FUNCTION, LANGUAGE, SCHEMA, DATABASE, TABLESPACE,
1428414285
* FOREIGN DATA WRAPPER, SERVER, or LARGE OBJECT.
@@ -14301,25 +14302,29 @@ dumpDefaultACL(Archive *fout, DefaultACLInfo *daclinfo)
1430114302
* NB: initacls/initracls are needed because extensions can set privileges on
1430214303
* an object during the extension's script file and we record those into
1430314304
* pg_init_privs as that object's initial privileges.
14305+
*
14306+
* Returns the dump ID assigned to the ACL TocEntry, or InvalidDumpId if
14307+
* no ACL entry was created.
1430414308
*----------
1430514309
*/
14306-
static void
14307-
dumpACL(Archive *fout, CatalogId objCatId, DumpId objDumpId,
14310+
static DumpId
14311+
dumpACL(Archive *fout, DumpId objDumpId, DumpId altDumpId,
1430814312
const char *type, const char *name, const char *subname,
1430914313
const char *nspname, const char *owner,
1431014314
const char *acls, const char *racls,
1431114315
const char *initacls, const char *initracls)
1431214316
{
14317+
DumpId aclDumpId = InvalidDumpId;
1431314318
DumpOptions *dopt = fout->dopt;
1431414319
PQExpBuffer sql;
1431514320

1431614321
/* Do nothing if ACL dump is not enabled */
1431714322
if (dopt->aclsSkip)
14318-
return;
14323+
return InvalidDumpId;
1431914324

1432014325
/* --data-only skips ACLs *except* BLOB ACLs */
1432114326
if (dopt->dataOnly && strcmp(type, "LARGE OBJECT") != 0)
14322-
return;
14327+
return InvalidDumpId;
1432314328

1432414329
sql = createPQExpBuffer();
1432514330

@@ -14353,24 +14358,35 @@ dumpACL(Archive *fout, CatalogId objCatId, DumpId objDumpId,
1435314358
if (sql->len > 0)
1435414359
{
1435514360
PQExpBuffer tag = createPQExpBuffer();
14361+
DumpId aclDeps[2];
14362+
int nDeps = 0;
1435614363

1435714364
if (subname)
1435814365
appendPQExpBuffer(tag, "COLUMN %s.%s", name, subname);
1435914366
else
1436014367
appendPQExpBuffer(tag, "%s %s", type, name);
1436114368

14362-
ArchiveEntry(fout, nilCatalogId, createDumpId(),
14369+
aclDeps[nDeps++] = objDumpId;
14370+
if (altDumpId != InvalidDumpId)
14371+
aclDeps[nDeps++] = altDumpId;
14372+
14373+
aclDumpId = createDumpId();
14374+
14375+
ArchiveEntry(fout, nilCatalogId, aclDumpId,
1436314376
tag->data, nspname,
1436414377
NULL,
1436514378
owner ? owner : "",
1436614379
false, "ACL", SECTION_NONE,
1436714380
sql->data, "", NULL,
14368-
&(objDumpId), 1,
14381+
aclDeps, nDeps,
1436914382
NULL, NULL);
14383+
1437014384
destroyPQExpBuffer(tag);
1437114385
}
1437214386

1437314387
destroyPQExpBuffer(sql);
14388+
14389+
return aclDumpId;
1437414390
}
1437514391

1437614392
/*
@@ -14692,6 +14708,7 @@ static void
1469214708
dumpTable(Archive *fout, TableInfo *tbinfo)
1469314709
{
1469414710
DumpOptions *dopt = fout->dopt;
14711+
DumpId tableAclDumpId = InvalidDumpId;
1469514712
char *namecopy;
1469614713

1469714714
/*
@@ -14713,11 +14730,12 @@ dumpTable(Archive *fout, TableInfo *tbinfo)
1471314730
const char *objtype =
1471414731
(tbinfo->relkind == RELKIND_SEQUENCE) ? "SEQUENCE" : "TABLE";
1471514732

14716-
dumpACL(fout, tbinfo->dobj.catId, tbinfo->dobj.dumpId,
14717-
objtype, namecopy, NULL,
14718-
tbinfo->dobj.namespace->dobj.name, tbinfo->rolname,
14719-
tbinfo->relacl, tbinfo->rrelacl,
14720-
tbinfo->initrelacl, tbinfo->initrrelacl);
14733+
tableAclDumpId =
14734+
dumpACL(fout, tbinfo->dobj.dumpId, InvalidDumpId,
14735+
objtype, namecopy, NULL,
14736+
tbinfo->dobj.namespace->dobj.name, tbinfo->rolname,
14737+
tbinfo->relacl, tbinfo->rrelacl,
14738+
tbinfo->initrelacl, tbinfo->initrrelacl);
1472114739
}
1472214740

1472314741
/*
@@ -14801,8 +14819,13 @@ dumpTable(Archive *fout, TableInfo *tbinfo)
1480114819
char *attnamecopy;
1480214820

1480314821
attnamecopy = pg_strdup(fmtId(attname));
14804-
/* Column's GRANT type is always TABLE */
14805-
dumpACL(fout, tbinfo->dobj.catId, tbinfo->dobj.dumpId,
14822+
14823+
/*
14824+
* Column's GRANT type is always TABLE. Each column ACL depends
14825+
* on the table-level ACL, since we can restore column ACLs in
14826+
* parallel but the table-level ACL has to be done first.
14827+
*/
14828+
dumpACL(fout, tbinfo->dobj.dumpId, tableAclDumpId,
1480614829
"TABLE", namecopy, attnamecopy,
1480714830
tbinfo->dobj.namespace->dobj.name, tbinfo->rolname,
1480814831
attacl, rattacl, initattacl, initrattacl);

0 commit comments

Comments
 (0)