Re: Replace known_assigned_xids_lck by memory barrier

2023-09-05 Thread Michail Nikolaev
Thanks everyone for help!

Re: Replace known_assigned_xids_lck by memory barrier

2023-09-05 Thread Nathan Bossart
On Fri, Sep 01, 2023 at 03:53:54PM -0400, Robert Haas wrote: > I'm not an expert on this code but I looked at this patch briefly and > it seems OK to me. Thanks for taking a look. Committed. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com

Re: Replace known_assigned_xids_lck by memory barrier

2023-09-01 Thread Robert Haas
On Fri, Sep 1, 2023 at 3:41 PM Nathan Bossart wrote: > On Wed, Aug 16, 2023 at 01:07:15PM -0700, Nathan Bossart wrote: > > Ah, that explains it. v5 of the patch is attached. > > Barring additional feedback, I plan to commit this patch in the current > commitfest. I'm not an expert on this code

Re: Replace known_assigned_xids_lck by memory barrier

2023-09-01 Thread Nathan Bossart
On Wed, Aug 16, 2023 at 01:07:15PM -0700, Nathan Bossart wrote: > Ah, that explains it. v5 of the patch is attached. Barring additional feedback, I plan to commit this patch in the current commitfest. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com

Re: Replace known_assigned_xids_lck by memory barrier

2023-08-16 Thread Nathan Bossart
On Wed, Aug 16, 2023 at 09:29:10PM +0200, Michail Nikolaev wrote: > As answer: probably we need to change > "If we know that we're holding ProcArrayLock exclusively, we don't > need the read barrier." > to > "If we're removing xid, we don't need the read barrier because only > the startup process

Re: Replace known_assigned_xids_lck by memory barrier

2023-08-16 Thread Michail Nikolaev
Hello, good question! Thanks for your edits. As answer: probably we need to change "If we know that we're holding ProcArrayLock exclusively, we don't need the read barrier." to "If we're removing xid, we don't need the read barrier because only the startup process can remove and add xids to

Re: Replace known_assigned_xids_lck by memory barrier

2023-08-16 Thread Nathan Bossart
On Wed, Aug 16, 2023 at 05:30:59PM +0200, Michail Nikolaev wrote: > Updated version (with read barriers is attached). Thanks for the updated patch. I've attached v4 in which I've made a number of cosmetic edits. > I'll think more, but can't find something wrong here so far. IIUC this memory

Re: Replace known_assigned_xids_lck by memory barrier

2023-08-16 Thread Michail Nikolaev
Hello! Updated version (with read barriers is attached). > One remaining question I have is whether it is okay if we see an updated value > for one of the head/tail variables but not the other. It looks like the > tail variable is only updated with ProcArrayLock held exclusively, which > IIUC

Re: Replace known_assigned_xids_lck by memory barrier

2023-08-15 Thread Nathan Bossart
On Tue, Aug 15, 2023 at 12:29:24PM +0200, Michail Nikolaev wrote: >> What sort of benefits do you see from this patch? It might be worthwhile >> in itself to remove spinlocks when possible, but IME it's much easier to >> justify such changes when there is a tangible benefit we can point to. > >

Re: Replace known_assigned_xids_lck by memory barrier

2023-08-15 Thread Michail Nikolaev
Hello, Nathan. > What sort of benefits do you see from this patch? It might be worthwhile > in itself to remove spinlocks when possible, but IME it's much easier to > justify such changes when there is a tangible benefit we can point to. Oh, it is not an easy question :) The answer, probably,

Re: Replace known_assigned_xids_lck by memory barrier

2023-08-14 Thread Nathan Bossart
On Sun, Mar 19, 2023 at 12:43:43PM +0300, Michail Nikolaev wrote: > In a nutshell: KnownAssignedXids as well as the head/tail pointers are > modified only by the startup process, so spinlock is used to ensure > that updates of the array and head/tail pointers are seen in a correct > order. It is

Replace known_assigned_xids_lck by memory barrier

2023-03-19 Thread Michail Nikolaev
Hello everyone and Tom. Tom, this is about your idea (1) from 2010 to replace spinlock with a memory barrier in a known assignment xids machinery. It was mentioned by you again in (2) and in (3) we have decided to extract this change into a separate commitfest entry. So, creating it here with a