Re: Add checkpoint and redo LSN to LogCheckpointEnd log message

At Tue, 15 Mar 2022 12:19:47 +0530, Bharath Rupireddy <bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote in
> On Fri, Mar 4, 2022 at 10:40 AM Kyotaro Horiguchi
> <horikyota(dot)ntt(at)gmail(dot)com> wrote:
> 0001 - I don't think you need to do this as UpdateControlFile
> (update_controlfile) will anyway update it, no?
> + /* Update control file using current time */
> + ControlFile->time = (pg_time_t) time(NULL);

Ugh.. Yes. It is a copy-pasto from older versions. They may have the
same copy-pasto..

> > 0002: Add REDO/Checkpiont LSNs to checkpoinkt-end log message.
> > (The main patch in this thread)
>
> 0002 - If at all the intention is to say that no ControlFileLock is
> required while reading ControlFile->checkPoint and
> ControlFile->checkPointCopy.redo, let's say it, no? How about
> something like "No ControlFileLock is required while reading
> ControlFile->checkPoint and ControlFile->checkPointCopy.redo as there
> can't be any other process updating them concurrently."?
>
> + /* we are the only updator of these variables */
> + LSN_FORMAT_ARGS(ControlFile->checkPoint),
> + LSN_FORMAT_ARGS(ControlFile->checkPointCopy.redo))));

I thought the comment explains that. But it would be better to be more
specific. It is changed as the follows.

> * ControlFileLock is not required as we are the only
> * updator of these variables.

> > 0003: Replace (WAL-)location to LSN in pg_controldata.
> >
> > 0004: Replace (WAL-)location to LSN in user-facing texts.
> > (This doesn't reflect my recent comments.)
>
> If you don't mind, can you please put the comments here?

Okay. It's the following message.

https://www.postgresql.org/message-id/20220209.115204.1794224638476710282.horikyota.ntt@gmail.com

> The old 0003 (attached 0004):
>
>
>
> +++ b/src/backend/access/rmgrdesc/xlogdesc.c
> - appendStringInfo(buf, "redo %X/%X; "
> + appendStringInfo(buf, "redo lsn %X/%X; "
>
>
>
> It is shown in the context of a checkpoint record, so I think it is
> not needed or rather lengthning the dump line uselessly.
>
>
>
> +++ b/src/backend/access/transam/xlog.c
> - (errmsg("request to flush past end of generated WAL; request %X/%X, current position %X/%X",
> + (errmsg("request to flush past end of generated WAL; request lsn %X/%X, current lsn %X/%X",
>
>
>
> +++ b/src/backend/replication/walsender.c
> - (errmsg("requested starting point %X/%X is ahead of the WAL flush position of this server %X/%X",
> + (errmsg("requested starting point %X/%X is ahead of the WAL flush LSN of this server %X/%X",
>
>
>
> "WAL" is upper-cased. So it seems rather strange that the "lsn" is
> lower-cased. In the first place the message doesn't look like a
> user-facing error message and I feel we don't need position or lsn
> there..
>
>
>
> +++ b/src/bin/pg_rewind/pg_rewind.c
> - pg_log_info("servers diverged at WAL location %X/%X on timeline %u",
> + pg_log_info("servers diverged at WAL LSN %X/%X on timeline %u",
>
>
>
> I feel that we don't need "WAL" there.
>
>
>
> +++ b/src/bin/pg_waldump/pg_waldump.c
> - printf(_(" -e, --end=RECPTR stop reading at WAL location RECPTR\n"));
> + printf(_(" -e, --end=RECPTR stop reading at WAL LSN RECPTR\n"));
>
>
>
> Mmm.. "WAL LSN RECPTR" looks strange to me. In the first place I
> don't think "RECPTR" is a user-facing term. Doesn't something like the
> follows work?
>
>
>
> + printf(_(" -e, --end=WAL-LSN stop reading at WAL-LSN\n"));
>
>
>
> In some changes in this patch shorten the main message text of
> fprintf-ish functions. That makes the succeeding parameters can be
> inlined.

> > 0005: Unhyphenate the word archive-recovery and similars.
>
> 0005 - How about replacing "crash-recovery" to "crash recovery" in
> postgres-ref.sgml too?

Oh, that's a left-over. Fixed. Thanks!

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center