Re: persist logical slots to disk during shutdown checkpoint

On Thu, Aug 31, 2023 at 2:52 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > > > I
> > > > think we should shut down subscriber, restart publisher and then make this
> > > > check based on the contents of the replication slot instead of server log.
> > > > Shutting down subscriber will ensure that the subscriber won't send any new
> > > > confirmed flush location to the publisher after restart.
> > > >
> > >
> > > But if we shutdown the subscriber before the publisher there is no
> > > guarantee that the publisher has sent all outstanding logs up to the
> > > shutdown checkpoint record (i.e., the latest record). Such a guarantee
> > > can only be there if we do a clean shutdown of the publisher before
> > > the subscriber.
> >
> > So the sequence is shutdown publisher node, shutdown subscriber node,
> > start publisher node and carry out the checks.
> >
>
> This can probably work but I still prefer the current approach as that
> will be closer to the ideal values on the disk instead of comparison
> with a later in-memory value of confirmed_flush LSN. Ideally, if we
> would have a tool like pg_replslotdata which can read the on-disk
> state of slots that would be better but missing that, the current one
> sounds like the next best possibility. Do you see any problem with the
> current approach of test?

> +  qr/Streaming transactions committing after ([A-F0-9]+\/[A-F0-9]+),
> reading WAL from ([A-F0-9]+\/[A-F0-9]+)./

I don't think the LSN reported in this message is guaranteed to be the
confirmed_flush LSN of the slot. It's usually confirmed_flush but not
always. It's the LSN that snapshot builder computes based on factors
including confirmed_flush. There's a chance that this test will fail
sometimes because  of this behaviour. Reading directly from
replication slot is better that this. pg_replslotdata might help if we
read replication slot content between shutdown and restart of
publisher.

>
> BTW, I think we can keep autovacuum = off for this test just to avoid
> any extra record generation even though that doesn't matter for the
> purpose of test.

Autovacuum is one thing, but we can't guarantee the absence of any
concurrent activity forever.

>
> > >
> > > > All the places which call ReplicationSlotSave() mark the slot as dirty.  All
> > > > the places where SaveSlotToPath() is called, the slot is marked dirty except
> > > > when calling from CheckPointReplicationSlots(). So I am wondering whether we
> > > > should be marking the slot dirty in CheckPointReplicationSlots() and avoid
> > > > passing down is_shutdown flag to SaveSlotToPath().
> > > >
> > >
> > > I feel that will add another spinlock acquire/release pair without
> > > much benefit. Sure, it may not be performance-sensitive but still
> > > adding another pair of lock/release doesn't seem like a better idea.
> >
> > We call ReplicatioinSlotMarkDirty() followed by ReplicationSlotSave()
> > at all the places, even those which are more frequent than this.
> >
>
> All but one. Normally, the idea of marking dirty is to indicate that
> we will actually write/flush the contents at a later point (except
> when required for correctness) as even indicated in the comments atop
> ReplicatioinSlotMarkDirty(). However, I see your point that we use
> that protocol at all the current places including CreateSlotOnDisk().
> So, we can probably do it here as well.

yes

I didn't see this entry in commitfest. Since we are discussing it and
the next CF is about to begin, probably it's good to add one there.
--
Best Wishes,
Ashutosh Bapat