Date: Tue, 7 Nov 2023 11:47:46 -0500
From: Stephen Frost <sfrost@snowman.net>
To: Nathan Bossart <nathandbossart@gmail.com>
Cc: John Morris <john.morris@crunchydata.com>,
	Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>,
	Andres Freund <andres@anarazel.de>,
	Michael Paquier <michael@paquier.xyz>,
	Robert Haas <robertmhaas@gmail.com>,
	"pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>
Subject: Re: Atomic ops for unlogged LSN
Message-ID: <ZUpqMrERC+48/a3p@tamriel.snowman.net>
References: <MN2PR13MB2688FD8B757316CB5C54C8A2A0DDA@MN2PR13MB2688.namprd13.prod.outlook.com>
 <20231026203433.GA1088329@nathanxps13>
 <BYAPR13MB26779988F4C73DD395401C59A0DCA@BYAPR13MB2677.namprd13.prod.outlook.com>
 <BYAPR13MB267762560386F4FB5BFDA235A0A7A@BYAPR13MB2677.namprd13.prod.outlook.com>
 <20231102034006.GA85609@nathanxps13>
 <20231106203350.GA598180@nathanxps13>
 <BYAPR13MB26777BA0E688ECE294BDE4A6A0AAA@BYAPR13MB2677.namprd13.prod.outlook.com>
 <20231107033558.GC729644@nathanxps13>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="YlO15m++s4N5xoGd"
Content-Disposition: inline
In-Reply-To: <20231107033558.GC729644@nathanxps13>
User-Agent: Mutt/2.1.4 (2021-12-11)
Archived-At: <https://www.postgresql.org/message-id/ZUpqMrERC%2B48/a3p%40tamriel.snowman.net>
Precedence: bulk


--YlO15m++s4N5xoGd
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

Greetings,

* Nathan Bossart (nathandbossart@gmail.com) wrote:
> On Tue, Nov 07, 2023 at 12:57:32AM +0000, John Morris wrote:
> > I incorporated your suggestions and added a few more. The changes are
> > mainly related to catching potential errors if some basic assumptions
> > aren=E2=80=99t met.
>=20
> Hm.  Could we move that to a separate patch?  We've lived without these
> extra checks for a very long time, and I'm not aware of any related issue=
s,
> so I'm not sure it's worth the added complexity.  And IMO it'd be better =
to
> keep it separate from the initial atomics conversion, anyway.

I do see the value in adding in an Assert though I don't want to throw
away the info about what the recent unlogged LSN was when we crash.  As
that basically boils down to a one-line addition, I don't think it
really needs to be in a separate patch.

> > I found the comment about cache coherency a bit confusing. We are deali=
ng
> > with a single address, so there should be no memory ordering or coheren=
cy
> > issues. (Did I misunderstand?) I see it more as a race condition. Rather
> > than merely explaining why it shouldn=E2=80=99t happen, the new version=
 verifies
> > the assumptions and throws an Assert() if something goes wrong.
>=20
> I was thinking of the comment for pg_atomic_read_u32() that I cited earli=
er
> [0].  This comment also notes that pg_atomic_read_u32/64() has no barrier
> semantics.  My interpretation of that comment is that these functions
> provide no guarantee that the value returned is the most up-to-date value.

There seems to be some serious misunderstanding about what is happening
here.  The value written into the control file for unlogged LSN during
normal operation does *not* need to be the most up-to-date value and
talking about it as if it needs to be the absolutely most up-to-date and
correct value is, if anything, adding to the confusion, not reducing
confusion.  The reason to write in anything other than a zero during
these routine checkpoints for unlogged LSN is entirely for forensics
purposes, not because we'll ever actually use the value- during crash
recovery and backup/restore, we're going to reset the unlogged LSN
counter anyway and we're going to throw away all of unlogged table
contents across the entire system.

We only care about the value of the unlogged LSN being correct during
normal shutdown when we're writing out the shutdown checkpoint, but by
that time everything else has been shut down and the value absolutely
should not be changing.

Thanks,

Stephen

--YlO15m++s4N5xoGd
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEwf6gbxKhD863zrx/7WyKOINHZFUFAmVKai8ACgkQ7WyKOINH
ZFU1CBAAq0gTRTl+eBFUbjycUd73Ehlt8b8+8SirkCr8LBhGmj4ru+hPujZ+y/7N
DgpriR/3TUnmbn8ZxocZc4liaV7LhrDgEzv0S2G+w9rAC62ptSAkmiE9qvzGGX/F
cGLZKp4VR6Y6N6GNCrr35pGLiogEv7iqssR4qbyieulY8bqMj/p5Z4FIuuZJmnia
NkAS3fjdKEB2EV0YlrQPrvB9vLPzuPFiN7nZPRltT+m4gQ6qvumQYe/HZ/VboasP
92RYu6M2tkFgZUkZhA381WaTvsOCAHQ4SbSucKbNby6zlaA2PvHVlG+s8Q2+EKS/
137OlzEbc40t1bkTV95sDQ3BBuZYZi9awINoUCwzfChHVh44ateJx23ifNjD9Xe2
biMEwAmV8k+duskS5ebQoELBDApXznPse2AWi7fLvFOyjv7rrlawl+wbjyggTPIO
jgGfXNfV9w5tRzdqnJYuSdbmQy2LhfN5ISkxou8dSsa9n+Gcltmex9IAbJIZ3Aen
4+L2MrFSTTH/KOlaGhc9UiFYpVefxv6LUFQuh6y8Iknwd4vqC9+Sc4lvT7q1Vqte
BZ7nZANwyJr3VZHwYd4mu2CwNpZRId1BXqrN/dfKGV4ngF/wX2tDES5HEwzeldBG
G4p2RYi8ZuAlj22+ZjcXJKpKxU4YE+95N43Z5c3rQy1g/rLAf0E=
=GDDu
-----END PGP SIGNATURE-----

--YlO15m++s4N5xoGd--