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

On Wed, Jul 31, 2024 at 3:27 AM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>
> On Wed, Jul 24, 2024 at 9:53 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> >
> > 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:
> > > >
> > > > 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.
> > >
> >
> > Indeed, locking all tables in the database to prevent concurrent DMLs
> > for this scenario also looks odd to me. The other alternative
> > previously suggested by Andres is to distribute catalog modifying
> > transactions to all concurrent in-progress transactions [1] but as
> > mentioned this could add an overhead. One possibility to reduce
> > overhead is that we selectively distribute invalidations for
> > catalogs-related publications but I haven't analyzed the feasibility.
> >
> > We need more opinions to decide here, so let me summarize the problem
> > and solutions discussed. As explained with an example in an email [1],
> > the problem related to logical decoding is that it doesn't process
> > invalidations corresponding to DDLs for the already in-progress
> > transactions. We discussed preventing DMLs in the first place when
> > concurrent DDLs like ALTER PUBLICATION ... ADD TABLE ... are in
> > progress. The solution discussed was to acquire
> > ShareUpdateExclusiveLock for all the tables being added via such
> > commands. Further analysis revealed that the same handling is required
> > for ALTER PUBLICATION ... ADD TABLES IN SCHEMA which means locking all
> > the tables in the specified schemas. Then DROP PUBLICATION also seems
> > to have similar symptoms which means in the worst case (where
> > publication is for ALL TABLES) we have to lock all the tables in the
> > database. We are not sure if that is good so the other alternative we
> > can pursue is to distribute invalidations in logical decoding
> > infrastructure [1] which has its downsides.
> >
> > Thoughts?
>
> Thank you for summarizing the problem and solutions!
>
> I think it's worth trying the idea of distributing invalidation
> messages, and we will see if there could be overheads or any further
> obstacles. IIUC this approach would resolve another issue we discussed
> before too[1].
>

Yes, and we also discussed having a similar solution at the time when
that problem was reported. So, it is clear that even though locking
tables can work for commands alter ALTER PUBLICATION ... ADD TABLE
..., we need a solution for distributing invalidations to the
in-progress transactions during logical decoding for other cases as
reported by you previously.

Thanks for looking into this.

--
With Regards,
Amit Kapila.