Committed.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
On Thu, Feb 29, 2024 at 10:04 PM Nathan Bossart
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
Looks good to me.
* John
From: Nathan Bossart
Date: Thursday, February 29, 2024 at 8:34 AM
To: Andres Freund
Cc: Stephen Frost , John Morris
, Bharath Rupireddy
, Michael Paquier
, Robert Haas ,
pgsql-hack...@postgresql.org
Subject: Re: Atomic ops for unlogged LSN
Here is a new
Greetings,
* Nathan Bossart (nathandboss...@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 chan
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
>From c40594c62ccfbf75cb0d3787cb9367d15ae37de8 Mon Sep 17 00:00:00 2001
From: Nathan Boss
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
> encoura
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 th
Hi,
On 2023-11-07 00:57:32 +, 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 cove
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 els
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 cha
Greetings,
* Nathan Bossart (nathandboss...@gmail.com) wrote:
> On Tue, Nov 07, 2023 at 12:57:32AM +, 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.
On Tue, Nov 07, 2023 at 12:57:32AM +, 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 c
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 contr
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 S
On Thu, Nov 2, 2023 at 9:10 AM Nathan Bossart wrote:
>
> On Wed, Nov 01, 2023 at 09:15:20PM +, 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
On Wed, Nov 01, 2023 at 09:15:20PM +, 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 ato
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”.
atomicLSN-v3-Use-atomic-ops-for-unloggedLSNs.patch
Description: atomicLSN-v3-Use-atomic-ops-for-unloggedLSN
* 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. Than
On Thu, Oct 26, 2023 at 03:00:58PM +, 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 commit
Keeping things up to date. Here is a rebased patch with no changes from
previous one.
* John Morris
atomic-lsn.patch
Description: atomic-lsn.patch
On Fri, Jul 21, 2023 at 4:43 AM John Morris wrote:
>
> Based on your feedback, I’ve updated the patch with additional comments.
>
> Explain the two cases when writing to the control file, and
> a bit more emphasis on unloggedLSNs not being valid after a crash.
Given that the callers already have
Based on your feedback, I’ve updated the patch with additional comments.
1. Explain the two cases when writing to the control file, and
2. a bit more emphasis on unloggedLSNs not being valid after a crash.
Thanks to y’all.
* John
v2-0001-Use-atomic-ops-for-unloggedLSNs.patch
Descript
> what happens if … reader here stores the old unloggedLSN value
> to control file and the server restarts (clean exit). So, the old
>value is loaded back to unloggedLSN upon restart and the callers of
> GetFakeLSNForUnloggedRel() will see an old/repeated value too. Is it a
> problem?
First, a c
On Thu, Jul 20, 2023 at 12:25 AM John Morris
wrote:
>
> >> Why don't we just use a barrier when around reading the value? It's not
> >> like
> >> CreateCheckPoint() is frequent?
>
> One reason is that a barrier isn’t needed, and adding unnecessary barriers
> can also be confusing.
>
> With respe
>> Why don't we just use a barrier when around reading the value? It's not like
>> CreateCheckPoint() is frequent?
One reason is that a barrier isn’t needed, and adding unnecessary barriers can
also be confusing.
With respect to the “debug only” comment in the original code, whichever value
is
Hi,
On 2023-07-17 16:15:52 -0700, Nathan Bossart wrote:
> On Mon, Jul 17, 2023 at 07:08:03PM -0400, Stephen Frost wrote:
> > Awesome. Was there any other feedback on this change which basically is
> > just getting rid of a spinlock and replacing it with using atomics?
> > It's still in needs-revi
On Mon, Jul 17, 2023 at 07:08:03PM -0400, Stephen Frost wrote:
> Awesome. Was there any other feedback on this change which basically is
> just getting rid of a spinlock and replacing it with using atomics?
> It's still in needs-review status but there's been a number of
> comments/reviews (drive-
Greetings,
* Nathan Bossart (nathandboss...@gmail.com) wrote:
> On Mon, Jun 12, 2023 at 07:24:18PM -0400, Stephen Frost wrote:
> > * Nathan Bossart (nathandboss...@gmail.com) wrote:
> >> Is it? I see uses in GiST indexing (62401db), so it's not immediately
> >> obvious to me how it is debugging-o
On Mon, Jun 12, 2023 at 07:24:18PM -0400, Stephen Frost wrote:
> * Nathan Bossart (nathandboss...@gmail.com) wrote:
>> Is it? I see uses in GiST indexing (62401db), so it's not immediately
>> obvious to me how it is debugging-only. If it is, then I think this patch
>> ought to clearly document it
Greetings,
* Nathan Bossart (nathandboss...@gmail.com) wrote:
> On Thu, May 25, 2023 at 07:41:21AM +0900, Michael Paquier wrote:
> > On Wed, May 24, 2023 at 02:49:58PM -0700, Andres Freund wrote:
> >> So we indeed loose some "barrier strength" - but I think that's fine. For
> >> one,
> >> it's a
On Thu, May 25, 2023 at 07:41:21AM +0900, Michael Paquier wrote:
> On Wed, May 24, 2023 at 02:49:58PM -0700, Andres Freund wrote:
>> So we indeed loose some "barrier strength" - but I think that's fine. For
>> one,
>> it's a debugging-only value.
Is it? I see uses in GiST indexing (62401db), so
On Wed, May 24, 2023 at 02:49:58PM -0700, Andres Freund wrote:
> I was a bit confused by Michael's comment as well, due to the section of code
> quoted. But he does have a point: pg_atomic_read_u32() does indeed *not* have
> barrier semantics (it'd be way more expensive), and the patch does contain
Hi,
On 2023-05-24 08:22:08 -0400, Robert Haas wrote:
> On Tue, May 23, 2023 at 6:26 PM Michael Paquier wrote:
> > Spinlocks provide a full memory barrier, which may not the case with
> > add_u64() or read_u64(). Could memory ordering be a problem in these
> > code paths?
>
> It could be a huge
On Tue, May 23, 2023 at 6:26 PM Michael Paquier wrote:
> Spinlocks provide a full memory barrier, which may not the case with
> add_u64() or read_u64(). Could memory ordering be a problem in these
> code paths?
It could be a huge problem if what you say were true, but unless I'm
missing somethin
On Tue, May 23, 2023 at 08:24:45PM +, John Morris wrote:
> This is a short patch which cleans up code for unlogged LSNs. It
> replaces the existing spinlock with atomic ops. It could provide a
> performance benefit for future uses of unlogged LSNS, but for now
> it is simply a cleaner impleme
This is a short patch which cleans up code for unlogged LSNs. It replaces the
existing spinlock with
atomic ops. It could provide a performance benefit for future uses of
unlogged LSNS, but for now it is simply a cleaner implementation.
0001-Use-atomic-ops-for-unloggedLSNs.patch
Description: 0
36 matches
Mail list logo
