public inbox for [email protected]
help / color / mirror / Atom feedFrom: Stephen Frost <[email protected]>
To: Nathan Bossart <[email protected]>
Cc: John Morris <[email protected]>
Cc: Bharath Rupireddy <[email protected]>
Cc: Andres Freund <[email protected]>
Cc: Michael Paquier <[email protected]>
Cc: Robert Haas <[email protected]>
Cc: [email protected] <[email protected]>
Subject: Re: Atomic ops for unlogged LSN
Date: Tue, 7 Nov 2023 11:47:46 -0500
Message-ID: <ZUpqMrERC+48/[email protected]> (raw)
In-Reply-To: <20231107033558.GC729644@nathanxps13>
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>
Greetings,
* Nathan Bossart ([email protected]) 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’t met.
>
> 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 issues,
> 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 dealing
> > with a single address, so there should be no memory ordering or coherency
> > issues. (Did I misunderstand?) I see it more as a race condition. Rather
> > than merely explaining why it shouldn’t happen, the new version verifies
> > the assumptions and throws an Assert() if something goes wrong.
>
> I was thinking of the comment for pg_atomic_read_u32() that I cited earlier
> [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
Attachments:
[application/pgp-signature] signature.asc (833B, ../ZUpqMrERC+48%[email protected]/2-signature.asc)
download
view thread (9+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: Atomic ops for unlogged LSN
In-Reply-To: <ZUpqMrERC+48/[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox