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

On Wed, 17 Jul 2024 at 11:54, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Tue, Jul 16, 2024 at 6:54 PM vignesh C <vignesh21(at)gmail(dot)com> wrote:
> >
> > 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].
> >
>
> 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.

I tested these scenarios, and as you expected, it throws an error for
the create publication case:
2024-07-17 14:50:01.145 IST [481526] 481526 ERROR: could not receive
data from WAL stream: ERROR: publication "pub1" does not exist
CONTEXT: slot "sub1", output plugin "pgoutput", in the change
callback, associated LSN 0/1510CD8
2024-07-17 14:50:01.147 IST [481450] 481450 LOG: background worker
"logical replication apply worker" (PID 481526) exited with exit code
1

The steps for this process are as follows:
1) Create tables in both the publisher and subscriber.
2) On the publisher: Create a replication slot.
3) On the subscriber: Create a subscription using the slot created by
the publisher.
4) On the publisher:
4.a) Session 1: BEGIN; INSERT INTO T1;
4.b) Session 2: CREATE PUBLICATION FOR ALL TABLES
4.c) Session 1: COMMIT;

Since we are throwing out a "publication does not exist" error, there
is no inconsistency issue here.

However, an issue persists with DROP ALL TABLES publication, where
data continues to replicate even after the publication is dropped.
This happens because the open transaction consumes the invalidation,
causing the publications to be revalidated using old snapshot. As a
result, both the open transactions and the subsequent transactions are
getting replicated.

We can reproduce this issue by following these steps in a logical
replication setup with an "ALL TABLES" publication:
On the publisher:
Session 1: BEGIN; INSERT INTO T1 VALUES (val1);
In another session on the publisher:
Session 2: DROP PUBLICATION
Back in Session 1 on the publisher:
COMMIT;
Finally, in Session 1 on the publisher:
INSERT INTO T1 VALUES (val2);

Even after dropping the publication, both val1 and val2 are still
being replicated to the subscriber. This means that both the
in-progress concurrent transaction and the subsequent transactions are
being replicated.

I don't think locking all tables is a viable solution in this case, as
it would require asking the user to refrain from performing any
operations on any of the tables in the database while creating a
publication.

Thoughts?

Regards,
Vignesh