On Thu, Aug 31, 2023 at 2:52 PM Amit Kapila <amit.kapil...@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
