Re: long-standing data loss bug in initial sync of logical replication

On Tue, 16 Jul 2024 at 11:59, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Tue, Jul 16, 2024 at 9:29 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)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].

[1] - https://www.postgresql.org/message-id/CAA4eK1LZDW2AVDYFZdZcvmsKVGajH2-gZmjXr9BsYiy8ct_fEw%40mail.gmail.com

Regards,
Vignesh