Horton

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

Amit Kapila <amit.kapila16@gmail.com>

View thread
From: Amit Kapila <amit.kapila16@gmail.com>
To: vignesh C <vignesh21@gmail.com>
Cc: Nitin Motiani <nitinmotiani@google.com>, Tomas Vondra <tomas.vondra@enterprisedb.com>, Andres Freund <andres@anarazel.de>, PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Date: 2024-07-17T06:24:45Z
Lists: pgsql-hackers

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Fix typo in test file name added in commit 4909b38af0.

  2. Fix data loss in logical replication.

  3. Avoid invalidating all RelationSyncCache entries on publication rename.

  4. Remove obsolete RECHECK keyword completely

  5. Backport BackgroundPsql perl test module

Attachments

On Tue, Jul 16, 2024 at 6:54 PM vignesh C <vignesh21@gmail.com> wrote:
>
> On Tue, 16 Jul 2024 at 11:59, Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Tue, Jul 16, 2024 at 9:29 AM Amit Kapila <amit.kapila16@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.