MIME-Version: 1.0
References: <CALj2ACUGWAC56yqhVOisNPXhewfRdeHG4NGTOK_sOStL=7yP+w@mail.gmail.com>
 <CAMm1aWY_wThAxmhC_7mkFsYHcNP2_JQLAKJEoY5EcHBi0Q42QQ@mail.gmail.com>
 <CAEze2WhfmEWDKcFx8jhcSFhbSoP7ettdYiikNSsVWCNpe5ME0g@mail.gmail.com>
 <CAMm1aWYsfgO91mXxy7kzrDcML8nuY3Ab4w9RnSjZ_mr6=Y-hwQ@mail.gmail.com>
 <20220217110515.zssjnez4lq2lmrdq@jrouhaud> <CAMm1aWZ1YY6n8T=0OkwQJJdua7fpkJdKr1fRqfTSrJb1ME9Q5g@mail.gmail.com>
 <20220217172724.uhs2v3mgio7y6ga5@jrouhaud> <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>
In-Reply-To: <CAMm1aWa8p=o60y1Z71bjAjWH8XH2Y+kZ6cNjQ21uK2_i65soOQ@mail.gmail.com>
From: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Date: Sun, 27 Feb 2022 20:44:39 +0530
Message-ID: <CALj2ACUNnqY7oc5C5yGS1ZbHV1RtQgPGiMkPTm989czCDwag6g@mail.gmail.com>
Subject: Re: Report checkpoint progress with pg_stat_progress_checkpoint (was:
 Report checkpoint progress in server logs)
To: Nitin Jadhav <nitinjadhavpostgres@gmail.com>
Cc: Ashutosh Sharma <ashu.coek88@gmail.com>, Julien Rouhaud <rjuju123@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/CALj2ACUNnqY7oc5C5yGS1ZbHV1RtQgPGiMkPTm989czCDwag6g%40mail.gmail.com>
Precedence: bulk

On Fri, Feb 25, 2022 at 8:38 PM Nitin Jadhav
<nitinjadhavpostgres@gmail.com> wrote:

Had a quick look over the v3 patch. I'm not sure if it's the best way
to have pg_stat_get_progress_checkpoint_type,
pg_stat_get_progress_checkpoint_kind and
pg_stat_get_progress_checkpoint_start_time just for printing info in
readable format in pg_stat_progress_checkpoint. I don't think these
functions will ever be useful for the users.

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?

2) Can't we just have these checks inside CASE-WHEN-THEN-ELSE blocks
directly instead of new function pg_stat_get_progress_checkpoint_kind?
+ snprintf(ckpt_kind, MAXPGPATH, "%s%s%s%s%s%s%s%s%s",
+ (flags == 0) ? "unknown" : "",
+ (flags & CHECKPOINT_IS_SHUTDOWN) ? "shutdown " : "",
+ (flags & CHECKPOINT_END_OF_RECOVERY) ? "end-of-recovery " : "",
+ (flags & CHECKPOINT_IMMEDIATE) ? "immediate " : "",
+ (flags & CHECKPOINT_FORCE) ? "force " : "",
+ (flags & CHECKPOINT_WAIT) ? "wait " : "",
+ (flags & CHECKPOINT_CAUSE_XLOG) ? "wal " : "",
+ (flags & CHECKPOINT_CAUSE_TIME) ? "time " : "",
+ (flags & CHECKPOINT_FLUSH_ALL) ? "flush-all" : "");

3) Why do we need this extra calculation for start_lsn? Do you ever
see a negative LSN or something here?
+    ('0/0'::pg_lsn + (
+        CASE
+            WHEN (s.param3 < 0) THEN pow((2)::numeric, (64)::numeric)
+            ELSE (0)::numeric
+        END + (s.param3)::numeric)) AS start_lsn,

4) Can't you use timestamptz_in(to_char(s.param4))  instead of
pg_stat_get_progress_checkpoint_start_time? I don't quite understand
the reasoning for having this function and it's named as *checkpoint*
when it doesn't do anything specific to the checkpoint at all?

Having 3 unnecessary functions that aren't useful to the users at all
in proc.dat will simply eatup the function oids IMO. Hence, I suggest
let's try to do without extra functions.

Regards,
Bharath Rupireddy.