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

On Wed, Jul 17, 2024 at 5:25 PM vignesh C <vignesh21(at)gmail(dot)com> wrote:
>
> 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.
>

Hi,

I tried the 'DROP PUBLICATION' command even for a publication with a
single table. And there also the data continues to get replicated.

To test this, I did a similar experiment as the above but instead of
creating publication on all tables, I did it for one specific table.

Here are the steps :
1. Create table test_1 and test_2 on both the publisher and subscriber
instances.
2. Create publication p for table test_1 on the publisher.
3. Create a subscription s which subscribes to p.
4. On the publisher
4a) Session 1 : BEGIN; INSERT INTO test_1 VALUES(val1);
4b) Session 2 : DROP PUBLICATION p;
4c) Session 1 : Commit;
5. On the publisher : INSERT INTO test_1 VALUES(val2);

After these, when I check the subscriber, both val1 and val2 have been
replicated. I tried a few more inserts on publisher after this and
they all got replicated to the subscriber. Only after explicitly
creating a new publication p2 for test_1 on the publisher, the
replication stopped. Most likely because the create publication
command invalidated the cache.

My guess is that this issue probably comes from the fact that
RemoveObjects in dropcmds.c doesn't do any special handling or
invalidation for the object drop command.

Please let me know if I'm missing something in my setup or if my
understanding of the drop commands is wrong.

Thanks