pg.ddx.io pgsql-hackers@postgresql.org mailing list archive
help / color / mirror / Atom feed"tuple concurrently updated" in pg_restore --jobs
6+ messages / 2 participants
[nested] [flat]
* "tuple concurrently updated" in pg_restore --jobs
@ 2020-07-06 05:01 Justin Pryzby <pryzby@telsasoft.com>
2020-07-10 20:54 ` Re: "tuple concurrently updated" in pg_restore --jobs Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 6+ messages in thread
From: Justin Pryzby @ 2020-07-06 05:01 UTC (permalink / raw)
To: David Gilman <davidgilman1@gmail.com>; +Cc: pgsql-hackers@lists.postgresql.org
I hit this issue intermittently (roughly half the time) while working with a
patch David submitted, and finally found a recipe to reproduce it on an
unpatched v12 instance.
I was surprised to see pg_restore -j2 is restoring ACLs in pre-data in
parallel. Note different session IDs and PIDs:
2020-07-05 23:31:27.448 CDT,"pryzbyj","secondary_dump",24037,"[local]",5f02a91f.5de5,70,,LOG,00000,"statement: REVOKE SELECT ON TABLE pg_catalog.pg_proc FROM PUBLIC; ",,,,,,,,,"pg_restore","client backend"
2020-07-05 23:31:27.448 CDT,"pryzbyj","secondary_dump",24036,"[local]",5f02a91f.5de4,78,,LOG,00000,"statement: GRANT SELECT(tableoid) ON TABLE pg_catalog.pg_proc TO PUBLIC; ",,,,,,,,,"pg_restore","client backend"
2020-07-05 23:31:27.450 CDT,"pryzbyj","secondary_dump",24036,"[local]",5f02a91f.5de4,79,,LOG,00000,"statement: GRANT SELECT(oid) ON TABLE pg_catalog.pg_proc TO PUBLIC; ",,,,,,,,,"pg_restore","client backend"
2020-07-05 23:31:27.450 CDT,"pryzbyj","secondary_dump",24037,"[local]",5f02a91f.5de5,71,,ERROR,XX000,"tuple concurrently updated",,,,,,"REVOKE SELECT ON TABLE pg_catalog.pg_proc FROM PUBLIC;
postgres=# CREATE DATABASE pryzbyj;
postgres=# \c pryzbyj
pryzbyj=# REVOKE ALL ON pg_proc FROM postgres;
pryzbyj=# GRANT SELECT (tableoid, oid, proname) ON pg_proc TO public;
pryzbyj=# \dp+ pg_catalog.pg_proc
Schema | Name | Type | Access privileges | Column privileges | Policies
------------+---------+-------+-------------------+-------------------+----------
pg_catalog | pg_proc | table | =r/postgres | tableoid: +|
| | | | =r/postgres +|
| | | | oid: +|
| | | | =r/postgres +|
| | | | proname: +|
| | | | =r/postgres |
[pryzbyj@database ~]$ pg_dump pryzbyj -Fc -f pg_dump.out
[pryzbyj@database ~]$ pg_restore pg_dump.out -j2 -d pryzbyj --clean -v
...
pg_restore: entering main parallel loop
pg_restore: launching item 3744 ACL TABLE pg_proc
pg_restore: launching item 3745 ACL COLUMN pg_proc.proname
pg_restore: creating ACL "pg_catalog.TABLE pg_proc"
pg_restore: creating ACL "pg_catalog.COLUMN pg_proc.proname"
pg_restore:pg_restore: while PROCESSING TOC:
finished item 3745 ACL COLUMN pg_proc.proname
pg_restore: from TOC entry 3744; 0 0 ACL TABLE pg_proc postgres
pg_restore: error: could not execute query: ERROR: tuple concurrently updated
Command was: REVOKE ALL ON TABLE pg_catalog.pg_proc FROM postgres;
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: "tuple concurrently updated" in pg_restore --jobs
2020-07-06 05:01 "tuple concurrently updated" in pg_restore --jobs Justin Pryzby <pryzby@telsasoft.com>
@ 2020-07-10 20:54 ` Tom Lane <tgl@sss.pgh.pa.us>
2020-07-10 21:06 ` Re: "tuple concurrently updated" in pg_restore --jobs Justin Pryzby <pryzby@telsasoft.com>
0 siblings, 1 reply; 6+ messages in thread
From: Tom Lane @ 2020-07-10 20:54 UTC (permalink / raw)
To: Justin Pryzby <pryzby@telsasoft.com>; +Cc: David Gilman <davidgilman1@gmail.com>; pgsql-hackers@lists.postgresql.org
Justin Pryzby <pryzby@telsasoft.com> writes:
> I hit this issue intermittently (roughly half the time) while working with a
> patch David submitted, and finally found a recipe to reproduce it on an
> unpatched v12 instance.
> I was surprised to see pg_restore -j2 is restoring ACLs in pre-data in
> parallel.
It's not pre-data. But it's true that pg_restore figures it can restore
ACLs in parallel during the ACL-restoring pass, on the theory that pg_dump
will not emit two different ACL entries for the same object, so that we
can do all the catalog updates in parallel without conflicts.
This works about 99% of the time, in fact. It falls down in the --clean
case if we have to revoke existing table permissions, because in that case
the REVOKE at table level is required to clear the table's per-column ACLs
as well, so that that ACL entry involves touching the same catalog rows
that the per-column ACLs want to touch.
I think the right fix is to give the per-column ACL entries dependencies
on the per-table ACL, if there is one. This will not fix the problem
for the case of restoring from an existing pg_dump archive that lacks
such dependency links --- but given the lack of field complaints, I'm
okay with that.
This looks straightforward, if somewhat tedious because we'll have to
change the API of pg_dump's dumpACL() function, which is called by
a lot of places. Barring objections, I'll go do that.
regards, tom lane
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: "tuple concurrently updated" in pg_restore --jobs
2020-07-06 05:01 "tuple concurrently updated" in pg_restore --jobs Justin Pryzby <pryzby@telsasoft.com>
2020-07-10 20:54 ` Re: "tuple concurrently updated" in pg_restore --jobs Tom Lane <tgl@sss.pgh.pa.us>
@ 2020-07-10 21:06 ` Justin Pryzby <pryzby@telsasoft.com>
2020-07-10 21:36 ` Re: "tuple concurrently updated" in pg_restore --jobs Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 6+ messages in thread
From: Justin Pryzby @ 2020-07-10 21:06 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: David Gilman <davidgilman1@gmail.com>; pgsql-hackers@lists.postgresql.org
On Fri, Jul 10, 2020 at 04:54:40PM -0400, Tom Lane wrote:
> Justin Pryzby <pryzby@telsasoft.com> writes:
> > I hit this issue intermittently (roughly half the time) while working with a
> > patch David submitted, and finally found a recipe to reproduce it on an
> > unpatched v12 instance.
>
> > I was surprised to see pg_restore -j2 is restoring ACLs in pre-data in
> > parallel.
>
> It's not pre-data. But it's true that pg_restore figures it can restore
> ACLs in parallel during the ACL-restoring pass, on the theory that pg_dump
> will not emit two different ACL entries for the same object, so that we
> can do all the catalog updates in parallel without conflicts.
>
> 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.
$ pg_restore pg_dump.out -j2 -d pryzbyj -v --section pre-data
pg_restore: entering main parallel loop
pg_restore: launching item 3395 ACL TABLE pg_proc
pg_restore: launching item 3396 ACL COLUMN pg_proc.proname
pg_restore: creating ACL "pg_catalog.TABLE pg_proc"
pg_restore: creating ACL "pg_catalog.COLUMN pg_proc.proname"
pg_restore: finished item 3395 ACL TABLE pg_proc
pg_restore: launching item 3397 ACL COLUMN pg_proc.pronamespace
pg_restore: creating ACL "pg_catalog.COLUMN pg_proc.pronamespace"
pg_restore: while PROCESSING TOC:
pg_restore: from TOC entry 3396; 0 0 ACL COLUMN pg_proc.proname postgres
pg_restore: error: could not execute query: ERROR: tuple concurrently updated
Command was: GRANT SELECT(proname) ON TABLE pg_catalog.pg_proc TO PUBLIC;
--
Justin
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: "tuple concurrently updated" in pg_restore --jobs
2020-07-06 05:01 "tuple concurrently updated" in pg_restore --jobs Justin Pryzby <pryzby@telsasoft.com>
2020-07-10 20:54 ` Re: "tuple concurrently updated" in pg_restore --jobs Tom Lane <tgl@sss.pgh.pa.us>
2020-07-10 21:06 ` Re: "tuple concurrently updated" in pg_restore --jobs Justin Pryzby <pryzby@telsasoft.com>
@ 2020-07-10 21:36 ` Tom Lane <tgl@sss.pgh.pa.us>
2020-07-10 21:45 ` Re: "tuple concurrently updated" in pg_restore --jobs Justin Pryzby <pryzby@telsasoft.com>
0 siblings, 1 reply; 6+ messages in thread
From: Tom Lane @ 2020-07-10 21:36 UTC (permalink / raw)
To: Justin Pryzby <pryzby@telsasoft.com>; +Cc: David Gilman <davidgilman1@gmail.com>; pgsql-hackers@lists.postgresql.org
Justin Pryzby <pryzby@telsasoft.com> 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
Attachments:
[text/x-diff] make-column-ACLs-depend-on-table-ACLs-1.patch (11.5K, ../../2164430.1594416988@sss.pgh.pa.us/2-make-column-ACLs-depend-on-table-ACLs-1.patch)
download | inline diff:
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 = 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 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 *nspinfo)
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 = 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 *fdwinfo)
/* 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 *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.
@@ -15012,25 +15013,29 @@ dumpDefaultACL(Archive *fout, DefaultACLInfo *daclinfo)
* NB: initacls/initracls are needed because extensions can set privileges 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 = 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();
@@ -15062,25 +15067,36 @@ 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,
ARCHIVE_OPTS(.tag = tag->data,
.namespace = nspname,
.owner = owner,
.description = "ACL",
.section = SECTION_NONE,
.createStmt = sql->data,
- .deps = &objDumpId,
- .nDeps = 1));
+ .deps = aclDeps,
+ .nDeps = nDeps));
+
destroyPQExpBuffer(tag);
}
destroyPQExpBuffer(sql);
+
+ return aclDumpId;
}
/*
@@ -15406,6 +15422,7 @@ static void
dumpTable(Archive *fout, TableInfo *tbinfo)
{
DumpOptions *dopt = fout->dopt;
+ DumpId tableAclDumpId = InvalidDumpId;
char *namecopy;
/*
@@ -15427,11 +15444,12 @@ dumpTable(Archive *fout, TableInfo *tbinfo)
const char *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, tbinfo->rrelacl,
- tbinfo->initrelacl, tbinfo->initrrelacl);
+ tableAclDumpId =
+ 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 = 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);
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: "tuple concurrently updated" in pg_restore --jobs
2020-07-06 05:01 "tuple concurrently updated" in pg_restore --jobs Justin Pryzby <pryzby@telsasoft.com>
2020-07-10 20:54 ` Re: "tuple concurrently updated" in pg_restore --jobs Tom Lane <tgl@sss.pgh.pa.us>
2020-07-10 21:06 ` Re: "tuple concurrently updated" in pg_restore --jobs Justin Pryzby <pryzby@telsasoft.com>
2020-07-10 21:36 ` Re: "tuple concurrently updated" in pg_restore --jobs Tom Lane <tgl@sss.pgh.pa.us>
@ 2020-07-10 21:45 ` Justin Pryzby <pryzby@telsasoft.com>
2020-07-11 17:11 ` Re: "tuple concurrently updated" in pg_restore --jobs Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 6+ messages in thread
From: Justin Pryzby @ 2020-07-10 21:45 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: David Gilman <davidgilman1@gmail.com>; pgsql-hackers@lists.postgresql.org
On Fri, Jul 10, 2020 at 05:36:28PM -0400, Tom Lane wrote:
> Justin Pryzby <pryzby@telsasoft.com> 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.
Yes, and the test case in David's patch on other thread [0] can't be
backpatched further than this patch is. A variant on his test case could just
as well be included in this patch (with pg_dump writing to a seekable FD) and
then amended later to also test writing to an unseekable FD.
[0] https://commitfest.postgresql.org/28/2568/
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: "tuple concurrently updated" in pg_restore --jobs
2020-07-06 05:01 "tuple concurrently updated" in pg_restore --jobs Justin Pryzby <pryzby@telsasoft.com>
2020-07-10 20:54 ` Re: "tuple concurrently updated" in pg_restore --jobs Tom Lane <tgl@sss.pgh.pa.us>
2020-07-10 21:06 ` Re: "tuple concurrently updated" in pg_restore --jobs Justin Pryzby <pryzby@telsasoft.com>
2020-07-10 21:36 ` Re: "tuple concurrently updated" in pg_restore --jobs Tom Lane <tgl@sss.pgh.pa.us>
2020-07-10 21:45 ` Re: "tuple concurrently updated" in pg_restore --jobs Justin Pryzby <pryzby@telsasoft.com>
@ 2020-07-11 17:11 ` Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 0 replies; 6+ messages in thread
From: Tom Lane @ 2020-07-11 17:11 UTC (permalink / raw)
To: Justin Pryzby <pryzby@telsasoft.com>; +Cc: David Gilman <davidgilman1@gmail.com>; pgsql-hackers@lists.postgresql.org
Justin Pryzby <pryzby@telsasoft.com> writes:
> On Fri, Jul 10, 2020 at 05:36:28PM -0400, Tom Lane wrote:
>> 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.
> Yes, and the test case in David's patch on other thread [0] can't be
> backpatched further than this patch is.
Actually, the answer seems to be that we'd better back-patch all the way,
because this is a live bug much further back than I'd guessed. pg_restore
is willing to run these ACL restores in parallel in all active branches.
The given test case only shows a failure back to 9.6, because older
versions don't dump ACLs on system catalogs; but of course you can just
try it with a user table instead.
Oddly, I could not get the "tuple concurrently updated" syndrome to
appear on 9.5. Not sure why not; the GRANT/REVOKE code looks the
same as in 9.6. What I *could* demonstrate in 9.5 is that sometimes
the post-restore state is flat out wrong: the column-level grants go
missing, presumably as a result of the table-level REVOKE executing
after the column-level GRANTs. Probably that syndrome occurs sometimes
in later branches too, depending on timing; but I didn't look.
regards, tom lane
^ permalink raw reply [nested|flat] 6+ messages in thread
end of thread, other threads:[~2020-07-11 17:11 UTC | newest]
Thread overview: 6+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2020-07-06 05:01 "tuple concurrently updated" in pg_restore --jobs Justin Pryzby <pryzby@telsasoft.com>
2020-07-10 20:54 ` Tom Lane <tgl@sss.pgh.pa.us>
2020-07-10 21:06 ` Justin Pryzby <pryzby@telsasoft.com>
2020-07-10 21:36 ` Tom Lane <tgl@sss.pgh.pa.us>
2020-07-10 21:45 ` Justin Pryzby <pryzby@telsasoft.com>
2020-07-11 17:11 ` Tom Lane <tgl@sss.pgh.pa.us>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox