RE: persist logical slots to disk during shutdown checkpoint

On Monday, September 4, 2023 6:15 PM vignesh C <vignesh21@gmail.com> wrote:
> 
> On Mon, 4 Sept 2023 at 15:20, Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Fri, Sep 1, 2023 at 10:50 AM vignesh C <vignesh21@gmail.com> wrote:
> > >
> > > On Fri, 1 Sept 2023 at 10:06, Amit Kapila <amit.kapila16@gmail.com> wrote:
> > > >
> > > > On Thu, Aug 31, 2023 at 6:12 PM Ashutosh Bapat
> > > > <ashutosh.bapat.oss@gmail.com> wrote:
> > > > >
> > > > > On Thu, Aug 31, 2023 at 2:52 PM Amit Kapila
> <amit.kapila16@gmail.com> wrote:
> > > > > >
> > > > > > 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 think we should also ensure that slots are not invalidated
> > > > (slot.data.invalidated != RS_INVAL_NONE) before marking them dirty
> > > > because we don't allow decoding from such slots, so we shouldn't
> > > > include those.
> > >
> > > Added this check.
> > >
> > > Apart from this I have also fixed the following issues that were
> > > agreed on: a) Setting slots to dirty in CheckPointReplicationSlots
> > > instead of setting it in SaveSlotToPath
> > >
> >
> > + if (is_shutdown && SlotIsLogical(s)) { SpinLockAcquire(&s->mutex);
> > + if (s->data.invalidated == RS_INVAL_NONE &&
> > + s->data.confirmed_flush != s->last_saved_confirmed_flush) dirty =
> > + s->true;
> >
> > I think it is better to use ReplicationSlotMarkDirty() as that would
> > be consistent with all other usages.
> 
> ReplicationSlotMarkDirty works only on MyReplicationSlot whereas
> CheckpointReplicationSlots loops through all the slots and marks the
> appropriate slot as dirty, we might have to change ReplicationSlotMarkDirty to
> take the slot as input parameter and all caller should pass MyReplicationSlot.

Personally, I feel if we want to centralize the code of marking dirty into a
function, we can introduce a new static function MarkSlotDirty(slot) to mark
passed slot dirty and let ReplicationSlotMarkDirty and
CheckpointReplicationSlots call it. Like:

void
ReplicationSlotMarkDirty(void)
{
    MarkSlotDirty(MyReplicationSlot);
}

+static void
+MarkSlotDirty(ReplicationSlot *slot)
+{
+    Assert(slot != NULL);
+
+    SpinLockAcquire(&slot->mutex);
+    slot->just_dirtied = true;
+    slot->dirty = true;
+    SpinLockRelease(&slot->mutex);
+}

This is somewhat similar to the relation between ReplicationSlotSave(serialize
my backend's replications slot) and SaveSlotToPath(save the passed slot).

> Another thing is we have already taken spin lock to access
> last_confirmed_flush_lsn from CheckpointReplicationSlots, we could set dirty
> flag here itself, else we will have to release the lock and call
> ReplicationSlotMarkDirty which will take lock again.

Yes, this is unavoidable, but maybe it's not a big problem as
we only do it at shutdown.

> Instead shall we set just_dirtied also in CheckpointReplicationSlots?
> Thoughts?

I agree we'd better set just_dirtied to true to ensure we will serialize slot
info here, because if some other processes just serialized the slot, the dirty
flag will be reset to false if we don't set just_dirtied to true in
CheckpointReplicationSlots(), this race condition may not exists for now, but
seems better to completely forbid it by setting just_dirtied.

Best Regards,
Hou zj