MIME-Version: 1.0
References: <CAMm1aWbjzP9g9ebkYqqJvNAW4jh6wP0maKW2Ozcv8PLVu76cFA@mail.gmail.com>
 <20220218074351.wgfosjt5lql2vjbr@jrouhaud> <CAMm1aWadyUhb89DEfgrs2j5i2u--O06x6oTp0EEhgGHtnw-SXA@mail.gmail.com>
 <20220219053215.r4wqatln5awklecf@jrouhaud> <CAMm1aWZzPVpLs7k5sQU4DObvh6YpOhjibcq2ykDrC_n25jeJNA@mail.gmail.com>
 <CAMm1aWZ07VYUPpvmV9Ck_VX-o+xH=hEBs-67scd_Us6U6FKbHA@mail.gmail.com>
 <CAE9k0Png9tjW5M7wjMKzEoOqkHS-VToP9yC2pr=HX7aQ0yQRNA@mail.gmail.com>
 <CAMm1aWa8p=o60y1Z71bjAjWH8XH2Y+kZ6cNjQ21uK2_i65soOQ@mail.gmail.com>
 <CALj2ACUNnqY7oc5C5yGS1ZbHV1RtQgPGiMkPTm989czCDwag6g@mail.gmail.com>
 <CALj2ACXDpgbVBt48D+2X4Hb7XvROMAg7EnbM-tHB+gPoHvEQew@mail.gmail.com> <20220228063222.rhmlwtdloeetibom@jrouhaud>
In-Reply-To: <20220228063222.rhmlwtdloeetibom@jrouhaud>
From: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Date: Mon, 28 Feb 2022 18:03:54 +0530
Message-ID: <CALj2ACXZ_TZHFJCh2Yn0p7huEzis4rw_aqJ=DyF3tB2vD646Mg@mail.gmail.com>
Subject: Re: Report checkpoint progress with pg_stat_progress_checkpoint (was:
 Report checkpoint progress in server logs)
To: Julien Rouhaud <rjuju123@gmail.com>
Cc: Nitin Jadhav <nitinjadhavpostgres@gmail.com>, Ashutosh Sharma <ashu.coek88@gmail.com>, 
	Matthias van de Meent <boekewurm+postgres@gmail.com>, Bruce Momjian <bruce@momjian.us>, 
	Tom Lane <tgl@sss.pgh.pa.us>, Magnus Hagander <magnus@hagander.net>, 
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Content-Type: text/plain; charset="UTF-8"
Archived-At: <https://www.postgresql.org/message-id/CALj2ACXZ_TZHFJCh2Yn0p7huEzis4rw_aqJ%3DDyF3tB2vD646Mg%40mail.gmail.com>
Precedence: bulk

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.