On Thu, Sep 7, 2023 at 4:30 PM Ashutosh Bapat <ashutosh.bapat....@gmail.com> wrote: > > On Thu, Sep 7, 2023 at 4:11 PM Amit Kapila <amit.kapil...@gmail.com> wrote: > > > > On Thu, Sep 7, 2023 at 3:38 PM Ashutosh Bapat > > <ashutosh.bapat....@gmail.com> wrote: > > > > > > * This needn't actually be part of a checkpoint, but it's a convenient > > > - * location. > > > + * location. is_shutdown is true in case of a shutdown checkpoint. > > > > > > Relying on the first sentence, if we decide to not persist the > > > replication slot at the time of checkpoint, would that be OK? It > > > doesn't look like a convenience thing to me any more. > > > > > > > Instead of removing that comment, how about something like this: "This > > needn't actually be part of a checkpoint except for shutdown > > checkpoint, but it's a convenient location."? > > > > I find the wording a bit awkward. My version would be "Checkpoint is a > convenient location to persist all the slots. But in a shutdown > checkpoint, indicated by is_shutdown = true, we also update > confirmed_flush." But please feel free to choose whichever version you > are comfortable with. >
I think saying we also update confirmed_flush appears unclear to me. So, I tried another version by changing the entire comment to: "Normally, we can flush dirty replication slots at regular intervals by any background process like bgwriter but checkpoint is a convenient location to persist. Additionally, in case of a shutdown checkpoint, we also identify the slots for which confirmed_flush has been updated since the last time it persisted and flush them." -- With Regards, Amit Kapila.