Date: Tue, 1 Feb 2022 16:05:38 -0500
From: Stephen Frost <sfrost@snowman.net>
To: Fujii Masao <masao.fujii@oss.nttdata.com>
Cc: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>,
	Kyotaro Horiguchi <horikyota.ntt@gmail.com>,
	Nathan Bossart <nathandbossart@gmail.com>,
	"Bossart, Nathan" <bossartn@amazon.com>,
	Julien Rouhaud <rjuju123@gmail.com>,
	Michael Paquier <michael@paquier.xyz>,
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: Add checkpoint and redo LSN to LogCheckpointEnd log message
Message-ID: <20220201210538.GX10577@tamriel.snowman.net>
References: <CALj2ACXDHGfhKkwzi81BRqWft4n85AO1zmkeqf6xEg+0xTCH9g@mail.gmail.com>
 <a1de2fd3-4a88-0325-c698-580c044f38b2@oss.nttdata.com>
 <CALj2ACU9ynpCwfuL0a4Bx0SDbHcjRDk49pwjDwfOzoQ1-kx1pw@mail.gmail.com>
 <20220201.152756.1157327790387186473.horikyota.ntt@gmail.com>
 <CALj2ACUtZhTb=2ENkF3BQ3wi137uaGi__qzvXC-qFYC0XwjALw@mail.gmail.com>
 <be548399-4c5e-77a7-2a95-fad224f1d24d@oss.nttdata.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="9I0HVnGRacHebCDW"
Content-Disposition: inline
In-Reply-To: <be548399-4c5e-77a7-2a95-fad224f1d24d@oss.nttdata.com>
User-Agent: Mutt/1.5.24 (2015-08-30)
Archived-At: <https://www.postgresql.org/message-id/20220201210538.GX10577%40tamriel.snowman.net>
Precedence: bulk


--9I0HVnGRacHebCDW
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

Greetings,

* Fujii Masao (masao.fujii@oss.nttdata.com) wrote:
> On 2022/02/01 22:03, Bharath Rupireddy wrote:
> >On Tue, Feb 1, 2022 at 11:58 AM Kyotaro Horiguchi
> ><horikyota.ntt@gmail.com> wrote:
> >>>Modified in v8.
> >>
> >>0001 looks good to me.
>=20
> I found that CreateRestartPoint() already reported the redo lsn as follow=
s after emitting the restartpoint log message. To avoid duplicated logging =
of the same information, we should update this code?
>=20
> 	ereport((log_checkpoints ? LOG : DEBUG2),
> 			(errmsg("recovery restart point at %X/%X",
> 					LSN_FORMAT_ARGS(lastCheckPoint.redo)),
> 			 xtime ? errdetail("Last completed transaction was at log time %s.",
> 							   timestamptz_to_str(xtime)) : 0));
>=20
> This code reports lastCheckPoint.redo as redo lsn. OTOH, with the patch, =
LogCheckpointEnd() reports ControlFile->checkPointCopy.redo. They may be di=
fferent, for example, when the current DB state is not DB_IN_ARCHIVE_RECOVE=
RY. In this case, which lsn should we report as redo lsn?
>=20
> +						"lsn=3D%X/%X, redo lsn=3D%X/%X",
>=20
> Originally you proposed to use upper cases for "lsn". But the latest patc=
h uses the lower cases. Why? It seems better to use upper cases, i.e., LSN =
and REDO LSN because LSN is basically used in other errmsg().

We do use 'lsn=3D' quite a bit in verify_nbtree.c already and lowercase is
also what's in the various functions and views in the catalog in the
database, of course.  I don't see even one usage of "LSN=3D" in the tree
today.  We also use 'lsn %X/%X' in replorigindesc.c, xactdesc.c,
xactdesc.c, tablesync.c, standby.c, parsexlog.c, then 'redo %X/%X' in
xactdesc.c.

xlog.c does have a number of "WAL location (LSN)", along with a bunch of
other usages.  logical.c uses both "LSN" and "lsn".  worker.c uses
"LSN".  slot.c uses "restart_lsn". pg_rewind.c uses "WAL location" while
pg_waldump.c uses, "lsn:", "WAL location", and "WAL record".

Overall, we don't seem to be super consistent, but I'd say that 'lsn=3D'
looks the best, to my eyes anyway, and isn't out of place in the code
base.  Lowercase seems to generally be more common overall.=20

> >Attaching the above changes 0003 (0001 and 0002 remain the same). If
> >the committer doesn't agree on the text or wording in 0003, I would
> >like the 0001 and 0002 to be taken here and I can start a new thread
> >for discussing 0003 separately.
>=20
> Personally I'm ok with 001, but regarding 0002 and 0003 patches, I'm not =
sure if it's really worth replacing "location" with "lsn" there. BTW, the s=
imilar idea was proposed at [1] before, but seems "location" was left as it=
 was.
>=20
> [1]
> https://postgr.es/m/20487.1494514594@sss.pgh.pa.us

This discussion strikes me as sufficient reason to make the change, with
the prior comment not really having all that much weight.  When we're
actually pretty consistent with one term, having random places where we
are inconsistent leads people to be unsure about which way to go and
then we end up having to have this discussion.  Would be great to avoid
having to have it again in the future.

Thanks,

Stephen

--9I0HVnGRacHebCDW
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iQIcBAEBCgAGBQJh+aCiAAoJEO1sijiDR2RVzpAP/29ttLDBOZERdAuY5FSJ6uS0
Ib2QUBeN8/+glRG2g60g7ETY1S3JW2OcGoWgn/h7C3mgoZN6dZI0Z/0XeUqyu8eB
JaNGcjza3TeYv5N6hapcJnZBNUDeOm5G02D965fSoj2a6FLN0TZAd/XAb079SjEs
Rp0rNxYHS1wSnSEbj+vmPlPH+qUNMJwm+CO2NXOJgvGTmjkhT6qAPGuX4EtaRaXU
3Vk8uXnuC5zOdPjzjn9scpj9E3jGYKvuXdj8KI29nd4I5fIPT9WwarWiSGsUbclK
z5K1MUH+Oo1Pn+QpFGBMxrISdJGxYe1srSwqUkGd7mcxKNmakd8GATkAD2IIRyjg
JIMALm9tfWAmp7GxxhZmRdkG6nTccJuIDsVzJcWwElkEpT2XmboTKB8YQ0GWDO+x
a72p1TvGCC12to53pCf35mU/becndvOZhVaZnUhKe86Eqs8k8Wn5Av+UBkippjtE
yJYra5xUf8tu+NIyCVrmmzxYMRoM9dX64wBKfZpKk33xONp4WE8UoTS3oaoKmauJ
bzZHotDFAX5HoB9yIYAX2RdHDL3+ZEJFNmWVT+0HmDoKmnqz9eF4YP64/eiB3U3k
qLxd5tNkqPZ7MfPjvWacVtszk0oMfS6Mz8Z9hEFKg4hxJU9kPVXsEGgcq4/eNsEK
WeRONnBIrxVHp9VGmhEZ
=OVzX
-----END PGP SIGNATURE-----

--9I0HVnGRacHebCDW--