On Tue, Jul 16, 2024 at 6:54 PM vignesh C <vignes...@gmail.com> wrote:
>
> On Tue, 16 Jul 2024 at 11:59, Amit Kapila <amit.kapil...@gmail.com> wrote:
> >
> > On Tue, Jul 16, 2024 at 9:29 AM Amit Kapila <amit.kapil...@gmail.com> wrote:
> > >
> > > One related comment:
> > > @@ -1219,8 +1219,14 @@ AlterPublicationTables(AlterPublicationStmt
> > > *stmt, HeapTuple tup,
> > > oldrel = palloc(sizeof(PublicationRelInfo));
> > > oldrel->whereClause = NULL;
> > > oldrel->columns = NIL;
> > > +
> > > + /*
> > > + * Data loss due to concurrency issues are avoided by locking
> > > + * the relation in ShareRowExclusiveLock as described atop
> > > + * OpenTableList.
> > > + */
> > > oldrel->relation = table_open(oldrelid,
> > > - ShareUpdateExclusiveLock);
> > > + ShareRowExclusiveLock);
> > >
> > > Isn't it better to lock the required relations in
> > > RemovePublicationRelById()?
> > >
> >
> > On my CentOS VM, the test file '100_bugs.pl' takes ~11s without a
> > patch and ~13.3s with a patch. So, 2 to 2.3s additional time for newly
> > added tests. It isn't worth adding this much extra time for one bug
> > fix. Can we combine table and schema tests into one single test and
> > avoid inheritance table tests as the code for those will mostly follow
> > the same path as a regular table?
>
> Yes, that is better. The attached v6 version patch has the changes for the
> same.
> The patch also addresses the comments from [1].
>
Thanks, I don't see any noticeable difference in test timing with new
tests. I have slightly modified the comments in the attached diff
patch (please rename it to .patch).
BTW, I noticed that we don't take any table-level locks for Create
Publication .. For ALL TABLES (and Drop Publication). Can that create
a similar problem? I haven't tested so not sure but even if there is a
problem for the Create case, it should lead to some ERROR like missing
publication.
--
With Regards,
Amit Kapila.
diff --git a/src/backend/catalog/pg_publication.c
b/src/backend/catalog/pg_publication.c
index a7c257a994..a274ec0f7e 100644
--- a/src/backend/catalog/pg_publication.c
+++ b/src/backend/catalog/pg_publication.c
@@ -680,8 +680,8 @@ publication_add_schema(Oid pubid, Oid schemaid, bool
if_not_exists)
PUBLICATION_PART_ALL);
/*
- * Data loss due to concurrency issues are avoided by locking the
relation
- * in ShareRowExclusiveLock as described atop OpenTableList.
+ * Lock the tables so that concurrent transactions don't miss
replicating
+ * the changes. See comments atop OpenTableList for further details.
*/
foreach_oid(schrelid, schemaRels)
LockRelationOid(schrelid, ShareRowExclusiveLock);
diff --git a/src/backend/commands/publicationcmds.c
b/src/backend/commands/publicationcmds.c
index 9d9b5f6af9..95f83d5563 100644
--- a/src/backend/commands/publicationcmds.c
+++ b/src/backend/commands/publicationcmds.c
@@ -1467,8 +1467,8 @@ RemovePublicationRelById(Oid proid)
pubrel->prrelid);
/*
- * Data loss due to concurrency issues are avoided by locking the
relation
- * in ShareRowExclusiveLock as described atop OpenTableList.
+ * Lock the tables to avoid concurrent transactions from replicating the
+ * changes. See comments atop OpenTableList for further details.
*/
foreach_oid(relid, relids)
LockRelationOid(relid, ShareRowExclusiveLock);
@@ -1540,8 +1540,8 @@ RemovePublicationSchemaById(Oid psoid)
PUBLICATION_PART_ALL);
/*
- * Data loss due to concurrency issues are avoided by locking the
relation
- * in ShareRowExclusiveLock as described atop OpenTableList.
+ * Lock the tables to avoid concurrent transactions from replicating the
+ * changes. See comments atop OpenTableList for further details.
*/
foreach_oid(schrelid, schemaRels)
LockRelationOid(schrelid, ShareRowExclusiveLock);
