Skip to content

Commit 5fea14f

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 f4ae676 commit 5fea14f

File tree

3 files changed

+64
-35
lines changed

3 files changed

+64
-35
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: 57 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -226,11 +226,11 @@ static void dumpUserMappings(Archive *fout,
226226
const char *owner, CatalogId catalogId, DumpId dumpId);
227227
static void dumpDefaultACL(Archive *fout, DefaultACLInfo *daclinfo);
228228

229-
static void dumpACL(Archive *fout, CatalogId objCatId, DumpId objDumpId,
230-
const char *type, const char *name, const char *subname,
231-
const char *nspname, const char *owner,
232-
const char *acls, const char *racls,
233-
const char *initacls, const char *initracls);
229+
static DumpId dumpACL(Archive *fout, DumpId objDumpId, DumpId altDumpId,
230+
const char *type, const char *name, const char *subname,
231+
const char *nspname, const char *owner,
232+
const char *acls, const char *racls,
233+
const char *initacls, const char *initracls);
234234

235235
static void getDependencies(Archive *fout);
236236
static void BuildArchiveDependencies(Archive *fout);
@@ -2922,7 +2922,7 @@ dumpDatabase(Archive *fout)
29222922
* Dump ACL if any. Note that we do not support initial privileges
29232923
* (pg_init_privs) on databases.
29242924
*/
2925-
dumpACL(fout, dbCatId, dbDumpId, "DATABASE",
2925+
dumpACL(fout, dbDumpId, InvalidDumpId, "DATABASE",
29262926
qdatname, NULL, NULL,
29272927
dba, datacl, rdatacl, "", "");
29282928

@@ -3407,7 +3407,7 @@ dumpBlob(Archive *fout, BlobInfo *binfo)
34073407

34083408
/* Dump ACL if any */
34093409
if (binfo->blobacl && (binfo->dobj.dump & DUMP_COMPONENT_ACL))
3410-
dumpACL(fout, binfo->dobj.catId, binfo->dobj.dumpId, "LARGE OBJECT",
3410+
dumpACL(fout, binfo->dobj.dumpId, InvalidDumpId, "LARGE OBJECT",
34113411
binfo->dobj.name, NULL,
34123412
NULL, binfo->rolname, binfo->blobacl, binfo->rblobacl,
34133413
binfo->initblobacl, binfo->initrblobacl);
@@ -10085,7 +10085,7 @@ dumpNamespace(Archive *fout, NamespaceInfo *nspinfo)
1008510085
nspinfo->dobj.catId, 0, nspinfo->dobj.dumpId);
1008610086

1008710087
if (nspinfo->dobj.dump & DUMP_COMPONENT_ACL)
10088-
dumpACL(fout, nspinfo->dobj.catId, nspinfo->dobj.dumpId, "SCHEMA",
10088+
dumpACL(fout, nspinfo->dobj.dumpId, InvalidDumpId, "SCHEMA",
1008910089
qnspname, NULL, NULL,
1009010090
nspinfo->rolname, nspinfo->nspacl, nspinfo->rnspacl,
1009110091
nspinfo->initnspacl, nspinfo->initrnspacl);
@@ -10369,7 +10369,7 @@ dumpEnumType(Archive *fout, TypeInfo *tyinfo)
1036910369
tyinfo->dobj.catId, 0, tyinfo->dobj.dumpId);
1037010370

1037110371
if (tyinfo->dobj.dump & DUMP_COMPONENT_ACL)
10372-
dumpACL(fout, tyinfo->dobj.catId, tyinfo->dobj.dumpId, "TYPE",
10372+
dumpACL(fout, tyinfo->dobj.dumpId, InvalidDumpId, "TYPE",
1037310373
qtypname, NULL,
1037410374
tyinfo->dobj.namespace->dobj.name,
1037510375
tyinfo->rolname, tyinfo->typacl, tyinfo->rtypacl,
@@ -10495,7 +10495,7 @@ dumpRangeType(Archive *fout, TypeInfo *tyinfo)
1049510495
tyinfo->dobj.catId, 0, tyinfo->dobj.dumpId);
1049610496

1049710497
if (tyinfo->dobj.dump & DUMP_COMPONENT_ACL)
10498-
dumpACL(fout, tyinfo->dobj.catId, tyinfo->dobj.dumpId, "TYPE",
10498+
dumpACL(fout, tyinfo->dobj.dumpId, InvalidDumpId, "TYPE",
1049910499
qtypname, NULL,
1050010500
tyinfo->dobj.namespace->dobj.name,
1050110501
tyinfo->rolname, tyinfo->typacl, tyinfo->rtypacl,
@@ -10567,7 +10567,7 @@ dumpUndefinedType(Archive *fout, TypeInfo *tyinfo)
1056710567
tyinfo->dobj.catId, 0, tyinfo->dobj.dumpId);
1056810568

1056910569
if (tyinfo->dobj.dump & DUMP_COMPONENT_ACL)
10570-
dumpACL(fout, tyinfo->dobj.catId, tyinfo->dobj.dumpId, "TYPE",
10570+
dumpACL(fout, tyinfo->dobj.dumpId, InvalidDumpId, "TYPE",
1057110571
qtypname, NULL,
1057210572
tyinfo->dobj.namespace->dobj.name,
1057310573
tyinfo->rolname, tyinfo->typacl, tyinfo->rtypacl,
@@ -10848,7 +10848,7 @@ dumpBaseType(Archive *fout, TypeInfo *tyinfo)
1084810848
tyinfo->dobj.catId, 0, tyinfo->dobj.dumpId);
1084910849

1085010850
if (tyinfo->dobj.dump & DUMP_COMPONENT_ACL)
10851-
dumpACL(fout, tyinfo->dobj.catId, tyinfo->dobj.dumpId, "TYPE",
10851+
dumpACL(fout, tyinfo->dobj.dumpId, InvalidDumpId, "TYPE",
1085210852
qtypname, NULL,
1085310853
tyinfo->dobj.namespace->dobj.name,
1085410854
tyinfo->rolname, tyinfo->typacl, tyinfo->rtypacl,
@@ -11004,7 +11004,7 @@ dumpDomain(Archive *fout, TypeInfo *tyinfo)
1100411004
tyinfo->dobj.catId, 0, tyinfo->dobj.dumpId);
1100511005

1100611006
if (tyinfo->dobj.dump & DUMP_COMPONENT_ACL)
11007-
dumpACL(fout, tyinfo->dobj.catId, tyinfo->dobj.dumpId, "TYPE",
11007+
dumpACL(fout, tyinfo->dobj.dumpId, InvalidDumpId, "TYPE",
1100811008
qtypname, NULL,
1100911009
tyinfo->dobj.namespace->dobj.name,
1101011010
tyinfo->rolname, tyinfo->typacl, tyinfo->rtypacl,
@@ -11226,7 +11226,7 @@ dumpCompositeType(Archive *fout, TypeInfo *tyinfo)
1122611226
tyinfo->dobj.catId, 0, tyinfo->dobj.dumpId);
1122711227

1122811228
if (tyinfo->dobj.dump & DUMP_COMPONENT_ACL)
11229-
dumpACL(fout, tyinfo->dobj.catId, tyinfo->dobj.dumpId, "TYPE",
11229+
dumpACL(fout, tyinfo->dobj.dumpId, InvalidDumpId, "TYPE",
1123011230
qtypname, NULL,
1123111231
tyinfo->dobj.namespace->dobj.name,
1123211232
tyinfo->rolname, tyinfo->typacl, tyinfo->rtypacl,
@@ -11526,7 +11526,7 @@ dumpProcLang(Archive *fout, ProcLangInfo *plang)
1152611526
plang->dobj.catId, 0, plang->dobj.dumpId);
1152711527

1152811528
if (plang->lanpltrusted && plang->dobj.dump & DUMP_COMPONENT_ACL)
11529-
dumpACL(fout, plang->dobj.catId, plang->dobj.dumpId, "LANGUAGE",
11529+
dumpACL(fout, plang->dobj.dumpId, InvalidDumpId, "LANGUAGE",
1153011530
qlanname, NULL, NULL,
1153111531
plang->lanowner, plang->lanacl, plang->rlanacl,
1153211532
plang->initlanacl, plang->initrlanacl);
@@ -12236,7 +12236,7 @@ dumpFunc(Archive *fout, FuncInfo *finfo)
1223612236
finfo->dobj.catId, 0, finfo->dobj.dumpId);
1223712237

1223812238
if (finfo->dobj.dump & DUMP_COMPONENT_ACL)
12239-
dumpACL(fout, finfo->dobj.catId, finfo->dobj.dumpId, keyword,
12239+
dumpACL(fout, finfo->dobj.dumpId, InvalidDumpId, keyword,
1224012240
funcsig, NULL,
1224112241
finfo->dobj.namespace->dobj.name,
1224212242
finfo->rolname, finfo->proacl, finfo->rproacl,
@@ -14257,7 +14257,7 @@ dumpAgg(Archive *fout, AggInfo *agginfo)
1425714257
aggsig = format_function_signature(fout, &agginfo->aggfn, true);
1425814258

1425914259
if (agginfo->aggfn.dobj.dump & DUMP_COMPONENT_ACL)
14260-
dumpACL(fout, agginfo->aggfn.dobj.catId, agginfo->aggfn.dobj.dumpId,
14260+
dumpACL(fout, agginfo->aggfn.dobj.dumpId, InvalidDumpId,
1426114261
"FUNCTION", aggsig, NULL,
1426214262
agginfo->aggfn.dobj.namespace->dobj.name,
1426314263
agginfo->aggfn.rolname, agginfo->aggfn.proacl,
@@ -14659,7 +14659,7 @@ dumpForeignDataWrapper(Archive *fout, FdwInfo *fdwinfo)
1465914659

1466014660
/* Handle the ACL */
1466114661
if (fdwinfo->dobj.dump & DUMP_COMPONENT_ACL)
14662-
dumpACL(fout, fdwinfo->dobj.catId, fdwinfo->dobj.dumpId,
14662+
dumpACL(fout, fdwinfo->dobj.dumpId, InvalidDumpId,
1466314663
"FOREIGN DATA WRAPPER", qfdwname, NULL,
1466414664
NULL, fdwinfo->rolname,
1466514665
fdwinfo->fdwacl, fdwinfo->rfdwacl,
@@ -14748,7 +14748,7 @@ dumpForeignServer(Archive *fout, ForeignServerInfo *srvinfo)
1474814748

1474914749
/* Handle the ACL */
1475014750
if (srvinfo->dobj.dump & DUMP_COMPONENT_ACL)
14751-
dumpACL(fout, srvinfo->dobj.catId, srvinfo->dobj.dumpId,
14751+
dumpACL(fout, srvinfo->dobj.dumpId, InvalidDumpId,
1475214752
"FOREIGN SERVER", qsrvname, NULL,
1475314753
NULL, srvinfo->rolname,
1475414754
srvinfo->srvacl, srvinfo->rsrvacl,
@@ -14941,8 +14941,9 @@ dumpDefaultACL(Archive *fout, DefaultACLInfo *daclinfo)
1494114941
/*----------
1494214942
* Write out grant/revoke information
1494314943
*
14944-
* 'objCatId' is the catalog ID of the underlying object.
1494514944
* 'objDumpId' is the dump ID of the underlying object.
14945+
* 'altDumpId' can be a second dumpId that the ACL entry must also depend on,
14946+
* or InvalidDumpId if there is no need for a second dependency.
1494614947
* 'type' must be one of
1494714948
* TABLE, SEQUENCE, FUNCTION, LANGUAGE, SCHEMA, DATABASE, TABLESPACE,
1494814949
* FOREIGN DATA WRAPPER, SERVER, or LARGE OBJECT.
@@ -14965,25 +14966,29 @@ dumpDefaultACL(Archive *fout, DefaultACLInfo *daclinfo)
1496514966
* NB: initacls/initracls are needed because extensions can set privileges on
1496614967
* an object during the extension's script file and we record those into
1496714968
* pg_init_privs as that object's initial privileges.
14969+
*
14970+
* Returns the dump ID assigned to the ACL TocEntry, or InvalidDumpId if
14971+
* no ACL entry was created.
1496814972
*----------
1496914973
*/
14970-
static void
14971-
dumpACL(Archive *fout, CatalogId objCatId, DumpId objDumpId,
14974+
static DumpId
14975+
dumpACL(Archive *fout, DumpId objDumpId, DumpId altDumpId,
1497214976
const char *type, const char *name, const char *subname,
1497314977
const char *nspname, const char *owner,
1497414978
const char *acls, const char *racls,
1497514979
const char *initacls, const char *initracls)
1497614980
{
14981+
DumpId aclDumpId = InvalidDumpId;
1497714982
DumpOptions *dopt = fout->dopt;
1497814983
PQExpBuffer sql;
1497914984

1498014985
/* Do nothing if ACL dump is not enabled */
1498114986
if (dopt->aclsSkip)
14982-
return;
14987+
return InvalidDumpId;
1498314988

1498414989
/* --data-only skips ACLs *except* BLOB ACLs */
1498514990
if (dopt->dataOnly && strcmp(type, "LARGE OBJECT") != 0)
14986-
return;
14991+
return InvalidDumpId;
1498714992

1498814993
sql = createPQExpBuffer();
1498914994

@@ -15015,25 +15020,36 @@ dumpACL(Archive *fout, CatalogId objCatId, DumpId objDumpId,
1501515020
if (sql->len > 0)
1501615021
{
1501715022
PQExpBuffer tag = createPQExpBuffer();
15023+
DumpId aclDeps[2];
15024+
int nDeps = 0;
1501815025

1501915026
if (subname)
1502015027
appendPQExpBuffer(tag, "COLUMN %s.%s", name, subname);
1502115028
else
1502215029
appendPQExpBuffer(tag, "%s %s", type, name);
1502315030

15024-
ArchiveEntry(fout, nilCatalogId, createDumpId(),
15031+
aclDeps[nDeps++] = objDumpId;
15032+
if (altDumpId != InvalidDumpId)
15033+
aclDeps[nDeps++] = altDumpId;
15034+
15035+
aclDumpId = createDumpId();
15036+
15037+
ArchiveEntry(fout, nilCatalogId, aclDumpId,
1502515038
ARCHIVE_OPTS(.tag = tag->data,
1502615039
.namespace = nspname,
1502715040
.owner = owner,
1502815041
.description = "ACL",
1502915042
.section = SECTION_NONE,
1503015043
.createStmt = sql->data,
15031-
.deps = &objDumpId,
15032-
.nDeps = 1));
15044+
.deps = aclDeps,
15045+
.nDeps = nDeps));
15046+
1503315047
destroyPQExpBuffer(tag);
1503415048
}
1503515049

1503615050
destroyPQExpBuffer(sql);
15051+
15052+
return aclDumpId;
1503715053
}
1503815054

1503915055
/*
@@ -15359,6 +15375,7 @@ static void
1535915375
dumpTable(Archive *fout, TableInfo *tbinfo)
1536015376
{
1536115377
DumpOptions *dopt = fout->dopt;
15378+
DumpId tableAclDumpId = InvalidDumpId;
1536215379
char *namecopy;
1536315380

1536415381
/*
@@ -15380,11 +15397,12 @@ dumpTable(Archive *fout, TableInfo *tbinfo)
1538015397
const char *objtype =
1538115398
(tbinfo->relkind == RELKIND_SEQUENCE) ? "SEQUENCE" : "TABLE";
1538215399

15383-
dumpACL(fout, tbinfo->dobj.catId, tbinfo->dobj.dumpId,
15384-
objtype, namecopy, NULL,
15385-
tbinfo->dobj.namespace->dobj.name, tbinfo->rolname,
15386-
tbinfo->relacl, tbinfo->rrelacl,
15387-
tbinfo->initrelacl, tbinfo->initrrelacl);
15400+
tableAclDumpId =
15401+
dumpACL(fout, tbinfo->dobj.dumpId, InvalidDumpId,
15402+
objtype, namecopy, NULL,
15403+
tbinfo->dobj.namespace->dobj.name, tbinfo->rolname,
15404+
tbinfo->relacl, tbinfo->rrelacl,
15405+
tbinfo->initrelacl, tbinfo->initrrelacl);
1538815406
}
1538915407

1539015408
/*
@@ -15468,8 +15486,13 @@ dumpTable(Archive *fout, TableInfo *tbinfo)
1546815486
char *attnamecopy;
1546915487

1547015488
attnamecopy = pg_strdup(fmtId(attname));
15471-
/* Column's GRANT type is always TABLE */
15472-
dumpACL(fout, tbinfo->dobj.catId, tbinfo->dobj.dumpId,
15489+
15490+
/*
15491+
* Column's GRANT type is always TABLE. Each column ACL depends
15492+
* on the table-level ACL, since we can restore column ACLs in
15493+
* parallel but the table-level ACL has to be done first.
15494+
*/
15495+
dumpACL(fout, tbinfo->dobj.dumpId, tableAclDumpId,
1547315496
"TABLE", namecopy, attnamecopy,
1547415497
tbinfo->dobj.namespace->dobj.name, tbinfo->rolname,
1547515498
attacl, rattacl, initattacl, initrattacl);

0 commit comments

Comments
 (0)