Horton

Thread

Commits

  1. Convert unloggedLSN to an atomic variable.

  2. Introduce atomic read/write functions with full barrier semantics.

  1. Re: Atomic ops for unlogged LSN

    John Morris <john.morris@crunchydata.com> — 2023-10-26T15:00:58Z

    Keeping things up to date.  Here is a rebased patch with no changes from previous one.


  *   John Morris

  2. Re: Atomic ops for unlogged LSN

    Nathan Bossart <nathandbossart@gmail.com> — 2023-10-26T20:34:33Z

    On Thu, Oct 26, 2023 at 03:00:58PM +0000, John Morris wrote:
> Keeping things up to date.  Here is a rebased patch with no changes from previous one.

This patch looks a little different than the last version I see posted [0].
That last version of the patch (which appears to be just about committable)
still applies for me, too.

[0] https://postgr.es/m/BYAPR13MB2677ED1797C81779D17B414CA03EA%40BYAPR13MB2677.namprd13.prod.outlook.com

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

  3. Re: Atomic ops for unlogged LSN

    John Morris <john.morris@crunchydata.com> — 2023-10-31T18:13:24Z

      *   This patch looks a little different than the last version I see posted [0].
That last version of the patch (which appears to be just about committable)

My oops – I was looking at the wrong commit. The newer patch has already been committed,  so pretend that last message didn’t happen. Thanks,
   John

  4. Re: Atomic ops for unlogged LSN

    John Morris <john.morris@crunchydata.com> — 2023-11-01T21:15:20Z

    Here is what I meant to do earlier. As it turns out, this patch has not been merged yet.

This is a rebased version . Even though I labelled it “v3”, there should be no changes from “v2”.

  5. Re: Atomic ops for unlogged LSN

    Nathan Bossart <nathandbossart@gmail.com> — 2023-11-02T03:40:06Z

    On Wed, Nov 01, 2023 at 09:15:20PM +0000, John Morris wrote:
> This is a rebased version . Even though I labelled it “v3”, there should be no changes from “v2”.

Thanks.  I think this is almost ready, but I have to harp on the
pg_atomic_read_u64() business once more.  The relevant comment in atomics.h
has this note:

 * The read is guaranteed to return a value as it has been written by this or
 * another process at some point in the past. There's however no cache
 * coherency interaction guaranteeing the value hasn't since been written to
 * again.

However unlikely, this seems to suggest that CreateCheckPoint() could see
an old value with your patch.  Out of an abundance of caution, I'd
recommend changing this to pg_atomic_compare_exchange_u64() like
pg_atomic_read_u64_impl() does in generic.h.

@@ -4635,7 +4629,6 @@ XLOGShmemInit(void)
 
 	SpinLockInit(&XLogCtl->Insert.insertpos_lck);
 	SpinLockInit(&XLogCtl->info_lck);
-	SpinLockInit(&XLogCtl->ulsn_lck);
 }

Shouldn't we do the pg_atomic_init_u64() here?  We can still set the
initial value in StartupXLOG(), but it might be safer to initialize the
variable where we are initializing the other shared memory stuff.

Since this isn't a tremendously performance-sensitive area, IMHO we should
code defensively to eliminate any doubts about correctness and to make it
easier to reason about.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

  6. Re: Atomic ops for unlogged LSN

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2023-11-02T18:19:38Z

    On Thu, Nov 2, 2023 at 9:10 AM Nathan Bossart <nathandbossart@gmail.com> wrote:
>
> On Wed, Nov 01, 2023 at 09:15:20PM +0000, John Morris wrote:
> > This is a rebased version . Even though I labelled it “v3”, there should be no changes from “v2”.
>
> Thanks.  I think this is almost ready, but I have to harp on the
> pg_atomic_read_u64() business once more.  The relevant comment in atomics.h
> has this note:
>
>  * The read is guaranteed to return a value as it has been written by this or
>  * another process at some point in the past. There's however no cache
>  * coherency interaction guaranteeing the value hasn't since been written to
>  * again.
>
> However unlikely, this seems to suggest that CreateCheckPoint() could see
> an old value with your patch.  Out of an abundance of caution, I'd
> recommend changing this to pg_atomic_compare_exchange_u64() like
> pg_atomic_read_u64_impl() does in generic.h.

+1. pg_atomic_read_u64 provides no barrier semantics whereas
pg_atomic_compare_exchange_u64 does. Without the barrier, it might
happen that the value is read while the other backend is changing it.
I think something like below providing full barrier semantics looks
fine to me:

XLogRecPtr ulsn;

pg_atomic_compare_exchange_u64(&XLogCtl->unloggedLSN, &ulsn, 0);
ControlFile->unloggedLSN = ulsn;

> @@ -4635,7 +4629,6 @@ XLOGShmemInit(void)
>
>         SpinLockInit(&XLogCtl->Insert.insertpos_lck);
>         SpinLockInit(&XLogCtl->info_lck);
> -       SpinLockInit(&XLogCtl->ulsn_lck);
>  }
>
> Shouldn't we do the pg_atomic_init_u64() here?  We can still set the
> initial value in StartupXLOG(), but it might be safer to initialize the
> variable where we are initializing the other shared memory stuff.

I think no one accesses the unloggedLSN in between
CreateSharedMemoryAndSemaphores -> XLOGShmemInit and StartupXLOG.
However, I see nothing wrong in doing
pg_atomic_init_u64(&XLogCtl->unloggedLSN, InvalidXLogRecPtr); in
XLOGShmemInit.

> Since this isn't a tremendously performance-sensitive area, IMHO we should
> code defensively to eliminate any doubts about correctness and to make it
> easier to reason about.

Right.

-- 
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

  7. Re: Atomic ops for unlogged LSN

    Nathan Bossart <nathandbossart@gmail.com> — 2023-11-06T20:33:50Z

    On Wed, Nov 01, 2023 at 10:40:06PM -0500, Nathan Bossart wrote:
> Since this isn't a tremendously performance-sensitive area, IMHO we should
> code defensively to eliminate any doubts about correctness and to make it
> easier to reason about.

Concretely, like this.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

  8. Re: Atomic ops for unlogged LSN

    John Morris <john.morris@crunchydata.com> — 2023-11-07T00:57:32Z

    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.

There are basically 3 assumptions. Stating them as conditions we want to avoid.

  *   We should not get an unlogged LSN before reading the control file.
  *   We should not get an unlogged LSN when shutting down.
  *   The unlogged LSN written out during a checkpoint shouldn’t be used.

Your suggestion addressed the first problem, and it took only minor changes to address the other two.

The essential idea is, we set a value of zero in each of the 3 situations. Then we throw an Assert() If somebody tries to allocate an unlogged LSN with the value zero.

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.

  9. Re: Atomic ops for unlogged LSN

    Nathan Bossart <nathandbossart@gmail.com> — 2023-11-07T03:35:58Z

    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 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.
But my interpretation could be wrong, and maybe this is meant to highlight
that the value might change before we can use the return value in a
compare/exchange or something.

I spent a little time earlier today reviewing the various underlying
implementations, but apparently I need to spend some more time looking at
those...

[0] https://postgr.es/m/20231102034006.GA85609%40nathanxps13

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

  10. Re: Atomic ops for unlogged LSN

    Stephen Frost <sfrost@snowman.net> — 2023-11-07T16:47:46Z

    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’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

  11. Re: Atomic ops for unlogged LSN

    Nathan Bossart <nathandbossart@gmail.com> — 2023-11-07T17:02:49Z

    On Tue, Nov 07, 2023 at 11:47:46AM -0500, Stephen Frost wrote:
> 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.

I agree that's all true.  I'm trying to connect how this scenario ensures
we see the most up-to-date value in light of this comment above
pg_atomic_read_u32():

 * The read is guaranteed to return a value as it has been written by this or
 * another process at some point in the past. There's however no cache
 * coherency interaction guaranteeing the value hasn't since been written to
 * again.

Is there something special about all other backends being shut down that
ensures this returns the most up-to-date value and not something from "some
point in the past" as the stated contract for this function seems to
suggest?

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

  12. Re: Atomic ops for unlogged LSN

    Andres Freund <andres@anarazel.de> — 2023-11-08T00:58:16Z

    Hi,

On 2023-11-07 11:02:49 -0600, Nathan Bossart wrote:
> On Tue, Nov 07, 2023 at 11:47:46AM -0500, Stephen Frost wrote:
> > 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.
> 
> I agree that's all true.  I'm trying to connect how this scenario ensures
> we see the most up-to-date value in light of this comment above
> pg_atomic_read_u32():
> 
>  * The read is guaranteed to return a value as it has been written by this or
>  * another process at some point in the past. There's however no cache
>  * coherency interaction guaranteeing the value hasn't since been written to
>  * again.
> 
> Is there something special about all other backends being shut down that
> ensures this returns the most up-to-date value and not something from "some
> point in the past" as the stated contract for this function seems to
> suggest?

Practically yes - getting to the point of writing the shutdown checkpoint
implies having gone through a bunch of code that implies memory barriers
(spinlocks, lwlocks).

However, even if there's likely some other implied memory barrier that we
could piggyback on, the patch much simpler to understand if it doesn't change
coherency rules. There's no way the overhead could matter.

Greetings,

Andres Freund

  13. Re: Atomic ops for unlogged LSN

    Andres Freund <andres@anarazel.de> — 2023-11-08T01:18:11Z

    Hi,

On 2023-11-07 00:57:32 +0000, John Morris wrote:
> 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.

IMO cache coherency covers the value a single variable has in different
threads / processes.

In fact, the only reason there effectively is a guarantee that you're not
seeing an outdated unloggedLSN value during shutdown checkpoints, even without
the spinlock or full barrier atomic op, is that the LWLockAcquire(), a few
lines above this, would prevent both the compiler and CPU from moving the read
of unloggedLSN to much earlier.  Obviously that lwlock has a different
address...


If the patch just had done the minimal conversion, it'd already have been
committed...  Even if there'd be a performance reason to get rid of the memory
barrier around reading unloggedLSN in CreateCheckPoint(), I'd do the
conversion in a separate commit.

Greetings,

Andres Freund

  14. Re: Atomic ops for unlogged LSN

    Nathan Bossart <nathandbossart@gmail.com> — 2023-11-09T21:27:33Z

    On Tue, Nov 07, 2023 at 04:58:16PM -0800, Andres Freund wrote:
> On 2023-11-07 11:02:49 -0600, Nathan Bossart wrote:
>> Is there something special about all other backends being shut down that
>> ensures this returns the most up-to-date value and not something from "some
>> point in the past" as the stated contract for this function seems to
>> suggest?
> 
> Practically yes - getting to the point of writing the shutdown checkpoint
> implies having gone through a bunch of code that implies memory barriers
> (spinlocks, lwlocks).

Sure.

> However, even if there's likely some other implied memory barrier that we
> could piggyback on, the patch much simpler to understand if it doesn't change
> coherency rules. There's no way the overhead could matter.

I wonder if it's worth providing a set of "locked read" functions.  Those
could just do a compare/exchange with 0 in the generic implementation.  For
patches like this one where the overhead really shouldn't matter, I'd
encourage folks to use those to make it easy to reason about correctness.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

  15. Re: Atomic ops for unlogged LSN

    Nathan Bossart <nathandbossart@gmail.com> — 2023-11-10T20:54:14Z

    On Thu, Nov 09, 2023 at 03:27:33PM -0600, Nathan Bossart wrote:
> I wonder if it's worth providing a set of "locked read" functions.  Those
> could just do a compare/exchange with 0 in the generic implementation.  For
> patches like this one where the overhead really shouldn't matter, I'd
> encourage folks to use those to make it easy to reason about correctness.

I moved this proposal to a new thread [0].

[0] https://postgr.es/m/20231110205128.GB1315705%40nathanxps13

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

  16. Re: Atomic ops for unlogged LSN

    Nathan Bossart <nathandbossart@gmail.com> — 2024-02-29T16:34:12Z

    Here is a new version of the patch that uses the new atomic read/write
functions with full barriers that were added in commit bd5132d.  Thoughts?

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

  17. Re: Atomic ops for unlogged LSN

    Stephen Frost <sfrost@snowman.net> — 2024-02-29T16:45:07Z

    Greetings,

* Nathan Bossart (nathandbossart@gmail.com) wrote:
> Here is a new version of the patch that uses the new atomic read/write
> functions with full barriers that were added in commit bd5132d.  Thoughts?

Saw that commit go in- glad to see it.  Thanks for updating this patch
too.  The changes look good to me.

Thanks again,

Stephen

  18. Re: Atomic ops for unlogged LSN

    John Morris <john.morris@crunchydata.com> — 2024-02-29T16:52:30Z

    Looks good to me.

  *   John

From: Nathan Bossart <nathandbossart@gmail.com>
Date: Thursday, February 29, 2024 at 8:34 AM
To: Andres Freund <andres@anarazel.de>
Cc: Stephen Frost <sfrost@snowman.net>, John Morris <john.morris@crunchydata.com>, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>, 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
Here is a new version of the patch that uses the new atomic read/write
functions with full barriers that were added in commit bd5132d.  Thoughts?

--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

  19. Re: Atomic ops for unlogged LSN

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2024-02-29T18:11:45Z

    On Thu, Feb 29, 2024 at 10:04 PM Nathan Bossart
<nathandbossart@gmail.com> wrote:
>
> Here is a new version of the patch that uses the new atomic read/write
> functions with full barriers that were added in commit bd5132d.  Thoughts?

Thanks for getting the other patch in. The attached v6 patch LGTM.

-- 
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

  20. Re: Atomic ops for unlogged LSN

    Nathan Bossart <nathandbossart@gmail.com> — 2024-02-29T20:37:52Z

    Committed.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com