Re: persist logical slots to disk during shutdown checkpoint

On Tue, Sep 5, 2023 at 9:58 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Tue, Sep 5, 2023 at 9:10 AM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
> >
> > The comments don't mention anything about why we are just flushing at
> > the shutdown checkpoint. I mean the checkpoint is not that frequent
> > and we already perform a lot of I/O during checkpoints so isn't it
> > wise to flush during every checkpoint. We may argue that there is no
> > extra advantage of that as we are not optimizing for crash recovery
> > but OTOH there is no reason for not doing so for other checkpoints or
> > we are worried about the concurrency with parallel walsender running
> > during non shutdown checkpoint if so then better we explain that as
> > well? If it is already discussed in the thread and we have a
> > conclusion on this then maybe we can mention this in comments?
> >
>
> The point is that at the time of non-shutdown checkpoints, it is not
> clear that there is an extra advantage but we will definitely add
> extra I/O for this. Because at other times, we will already be saving
> the slot from time to time as the replication makes progress. And, we
> also need to flush such slots during shutdown for correctness for some
> use cases like upgrades. We can probably add something like: "At other
> times, the walsender keeps saving the slot from time to time as the
> replication progresses, so there is no clear advantage of flushing
> additional slots at the time of checkpoint". Will that work for you?

Yeah that comments will work out, my only concern was because we added
an explicit condition that it should be synced only during shutdown
checkpoint so better comments also explicitly explains the reason.
Anyway I am fine with either way whether we sync at the shutdown
checkpoint or all the checkpoint. Because I/O for slot sync during
checkpoint time should not be a real worry and with that if we can
avoid additional code with extra conditions then it's better because
such code branches will be frequently hit and I think for testability
pov we prefer to add code in common path unless there is some overhead
or it is specifically meant for that branch only.

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com