On Mon, Feb 28, 2022 at 12:02 PM Julien Rouhaud <rjuju123@gmail.com> wrote:
>
> Hi,
>
> On Mon, Feb 28, 2022 at 10:21:23AM +0530, Bharath Rupireddy wrote:
> >
> > Another thought for my review comment:
> > > 1) Can't we use pg_is_in_recovery to determine if it's a restartpoint
> > > or checkpoint instead of having a new function
> > > pg_stat_get_progress_checkpoint_type?
> >
> > I don't think using pg_is_in_recovery work here as it is taken after
> > the checkpoint has started. So, I think the right way here is to send
> > 1 in CreateCheckPoint and 2 in CreateRestartPoint and use
> > CASE-WHEN-ELSE-END to show "1": "checkpoint" "2":"restartpoint".
>
> I suggested upthread to store the starting timeline instead. This way you can
> deduce whether it's a restartpoint or a checkpoint, but you can also deduce
> other information, like what was the starting WAL.
I don't understand why we need the timeline here to just determine
whether it's a restartpoint or checkpoint. I know that the
InsertTimeLineID is 0 during recovery. IMO, emitting 1 for checkpoint
and 2 for restartpoint in CreateCheckPoint and CreateRestartPoint
respectively and using CASE-WHEN-ELSE-END to show it in readable
format is the easiest way.
Can't the checkpoint start LSN be deduced from
PROGRESS_CHECKPOINT_LSN, checkPoint.redo?
I'm completely against these pg_stat_get_progress_checkpoint_{type,
kind, start_time} functions unless there's a strong case. IMO, we can
achieve what we want without these functions as well.
> > 11) I think it's discussed, are we going to add the pid of the
> > checkpoint requestor?
>
> As mentioned upthread, there can be multiple backends that request a
> checkpoint, so unless we want to store an array of pid we should store a number
> of backend that are waiting for a new checkpoint.
Yeah, you are right. Let's not go that path and store an array of
pids. I don't see a strong use-case with the pid of the process
requesting checkpoint. If required, we can add it later once the
pg_stat_progress_checkpoint view gets in.
Regards,
Bharath Rupireddy.
