Re: persist logical slots to disk during shutdown checkpoint

On Tue, Sep 5, 2023 at 7:54 AM Zhijie Hou (Fujitsu)
<houzj(dot)fnst(at)fujitsu(dot)com> wrote:
>
> On Monday, September 4, 2023 6:15 PM vignesh C <vignesh21(at)gmail(dot)com> wrote:
> >
> > On Mon, 4 Sept 2023 at 15:20, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> > >
> > > On Fri, Sep 1, 2023 at 10:50 AM vignesh C <vignesh21(at)gmail(dot)com> wrote:
> > > >
> > > > On Fri, 1 Sept 2023 at 10:06, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> > > > >
> > > > > On Thu, Aug 31, 2023 at 6:12 PM Ashutosh Bapat
> > > > > <ashutosh(dot)bapat(dot)oss(at)gmail(dot)com> wrote:
> > > > > >
> > > > > > On Thu, Aug 31, 2023 at 2:52 PM Amit Kapila
> > <amit(dot)kapila16(at)gmail(dot)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.
>

True but still it doesn't look elegant. I also thought about having a
probably inline function that marks both just_dirty and dirty fields.
However, that requires us to assert that the caller has already
acquired a spinlock. I see a macro SpinLockFree() that might help but
it didn't seem to be used anywhere in the code so not sure if we can
rely on it.

> > 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.
>

Agreed, and it is better to close any such possibility because we
can't say with certainty about manual slots. This seems better than
the other ideas we discussed.

--
With Regards,
Amit Kapila.