Replace known_assigned_xids_lck by memory barrier
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 rebased version of (4).
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 enough to pass the barrier after writing to the array
(but before updating the pointers) to achieve the same result.
Best regards.
[2]: /messages/by-id/1249332.1668553589@sss.pgh.pa.us
[3]: /messages/by-id/1225350.1669757944@sss.pgh.pa.us
[4]: /messages/by-id/CANtu0oiPoSdQsjRd6Red5WMHi1E83d2+-bM9J6dtWR3c5Tap9g@mail.gmail.com
Attachments:
v2-0001-Removes-known_assigned_xids_lck-using-the-write-m.patchapplication/x-patch; name=v2-0001-Removes-known_assigned_xids_lck-using-the-write-m.patchDownload
From d968645551412abdb3fac6b8514c3d6746e8ac3d Mon Sep 17 00:00:00 2001
From: Michail Nikolaev <michail.nikolaev@gmail.com>
Date: Sat, 18 Mar 2023 21:28:00 +0300
Subject: [PATCH v2] Removes known_assigned_xids_lck, using the write memory
barrier as suggested earlier.
---
src/backend/storage/ipc/procarray.c | 41 +++++++----------------------
1 file changed, 9 insertions(+), 32 deletions(-)
diff --git a/src/backend/storage/ipc/procarray.c b/src/backend/storage/ipc/procarray.c
index ea91ce355f..95e2672f9a 100644
--- a/src/backend/storage/ipc/procarray.c
+++ b/src/backend/storage/ipc/procarray.c
@@ -61,7 +61,6 @@
#include "port/pg_lfind.h"
#include "storage/proc.h"
#include "storage/procarray.h"
-#include "storage/spin.h"
#include "utils/acl.h"
#include "utils/builtins.h"
#include "utils/rel.h"
@@ -82,7 +81,6 @@ typedef struct ProcArrayStruct
int numKnownAssignedXids; /* current # of valid entries */
int tailKnownAssignedXids; /* index of oldest valid element */
int headKnownAssignedXids; /* index of newest element, + 1 */
- slock_t known_assigned_xids_lck; /* protects head/tail pointers */
/*
* Highest subxid that has been removed from KnownAssignedXids array to
@@ -444,7 +442,6 @@ CreateSharedProcArray(void)
procArray->numKnownAssignedXids = 0;
procArray->tailKnownAssignedXids = 0;
procArray->headKnownAssignedXids = 0;
- SpinLockInit(&procArray->known_assigned_xids_lck);
procArray->lastOverflowedXid = InvalidTransactionId;
procArray->replication_slot_xmin = InvalidTransactionId;
procArray->replication_slot_catalog_xmin = InvalidTransactionId;
@@ -4651,10 +4648,9 @@ KnownAssignedTransactionIdsIdleMaintenance(void)
* pointer. This wouldn't require any lock at all, except that on machines
* with weak memory ordering we need to be careful that other processors
* see the array element changes before they see the head pointer change.
- * We handle this by using a spinlock to protect reads and writes of the
- * head/tail pointers. (We could dispense with the spinlock if we were to
- * create suitable memory access barrier primitives and use those instead.)
- * The spinlock must be taken to read or write the head/tail pointers unless
+ * We handle this by using a memory barrier to protect writes of the
+ * head pointer.
+ * The memory barrier is taken before write the head pointer unless
* the caller holds ProcArrayLock exclusively.
*
* Algorithmic analysis:
@@ -4712,7 +4708,7 @@ KnownAssignedXidsCompress(KAXCompressReason reason, bool haveLock)
/*
* Since only the startup process modifies the head/tail pointers, we
- * don't need a lock to read them here.
+ * are safe read them here.
*/
head = pArray->headKnownAssignedXids;
tail = pArray->tailKnownAssignedXids;
@@ -4893,21 +4889,20 @@ KnownAssignedXidsAdd(TransactionId from_xid, TransactionId to_xid,
pArray->numKnownAssignedXids += nxids;
/*
- * Now update the head pointer. We use a spinlock to protect this
+ * Now update the head pointer. We use a memory barrier to protect this
* pointer, not because the update is likely to be non-atomic, but to
* ensure that other processors see the above array updates before they
* see the head pointer change.
*
* If we're holding ProcArrayLock exclusively, there's no need to take the
- * spinlock.
+ * barrier.
*/
if (exclusive_lock)
pArray->headKnownAssignedXids = head;
else
{
- SpinLockAcquire(&pArray->known_assigned_xids_lck);
+ pg_write_barrier();
pArray->headKnownAssignedXids = head;
- SpinLockRelease(&pArray->known_assigned_xids_lck);
}
}
@@ -4930,20 +4925,8 @@ KnownAssignedXidsSearch(TransactionId xid, bool remove)
int tail;
int result_index = -1;
- if (remove)
- {
- /* we hold ProcArrayLock exclusively, so no need for spinlock */
- tail = pArray->tailKnownAssignedXids;
- head = pArray->headKnownAssignedXids;
- }
- else
- {
- /* take spinlock to ensure we see up-to-date array contents */
- SpinLockAcquire(&pArray->known_assigned_xids_lck);
- tail = pArray->tailKnownAssignedXids;
- head = pArray->headKnownAssignedXids;
- SpinLockRelease(&pArray->known_assigned_xids_lck);
- }
+ tail = pArray->tailKnownAssignedXids;
+ head = pArray->headKnownAssignedXids;
/*
* Standard binary search. Note we can ignore the KnownAssignedXidsValid
@@ -5181,13 +5164,9 @@ KnownAssignedXidsGetAndSetXmin(TransactionId *xarray, TransactionId *xmin,
* cannot enter and then leave the array while we hold ProcArrayLock. We
* might miss newly-added xids, but they should be >= xmax so irrelevant
* anyway.
- *
- * Must take spinlock to ensure we see up-to-date array contents.
*/
- SpinLockAcquire(&procArray->known_assigned_xids_lck);
tail = procArray->tailKnownAssignedXids;
head = procArray->headKnownAssignedXids;
- SpinLockRelease(&procArray->known_assigned_xids_lck);
for (i = tail; i < head; i++)
{
@@ -5234,10 +5213,8 @@ KnownAssignedXidsGetOldestXmin(void)
/*
* Fetch head just once, since it may change while we loop.
*/
- SpinLockAcquire(&procArray->known_assigned_xids_lck);
tail = procArray->tailKnownAssignedXids;
head = procArray->headKnownAssignedXids;
- SpinLockRelease(&procArray->known_assigned_xids_lck);
for (i = tail; i < head; i++)
{
--
2.34.1
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 enough to pass the barrier after writing to the array
(but before updating the pointers) to achieve the same result.
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.
/*
- * Now update the head pointer. We use a spinlock to protect this
+ * Now update the head pointer. We use a memory barrier to protect this
* pointer, not because the update is likely to be non-atomic, but to
* ensure that other processors see the above array updates before they
* see the head pointer change.
*
* If we're holding ProcArrayLock exclusively, there's no need to take the
- * spinlock.
+ * barrier.
*/Are the assignments in question guaranteed to be atomic? IIUC we assume
that aligned 4-byte loads/stores are atomic, so we should be okay as long
as we aren't handling anything larger.
- SpinLockAcquire(&pArray->known_assigned_xids_lck);
+ pg_write_barrier();
pArray->headKnownAssignedXids = head;
- SpinLockRelease(&pArray->known_assigned_xids_lck);This use of pg_write_barrier() looks correct to me, but don't we need
corresponding read barriers wherever we obtain the pointers? FWIW I tend
to review src/backend/storage/lmgr/README.barrier in its entirety whenever
I deal with this stuff.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
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, looks like this:
1) performance benefits of spin lock acquire removing in
KnownAssignedXidsGetOldestXmin and KnownAssignedXidsSearch
2) it is closing 13-year-old tech depth
But in reality, it is not easy to measure performance improvement
consistently for this change.
Are the assignments in question guaranteed to be atomic? IIUC we assume
that aligned 4-byte loads/stores are atomic, so we should be okay as long
as we aren't handling anything larger.
Yes, 4-bytes assignment are atomic, locking is used to ensure memory
write ordering in this place.
This use of pg_write_barrier() looks correct to me, but don't we need
corresponding read barriers wherever we obtain the pointers? FWIW I tend
to review src/backend/storage/lmgr/README.barrier in its entirety whenever
I deal with this stuff.
Oh, yeah, you're right! (1)
I'll prepare an updated version of the patch soon. I don't why I was
assuming pg_write_barrier is enough (⊙＿⊙')
[1]: https://github.com/postgres/postgres/blob/master/src/backend/storage/lmgr/README.barrier#L125
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.Oh, it is not an easy question :)
The answer, probably, looks like this:
1) performance benefits of spin lock acquire removing in
KnownAssignedXidsGetOldestXmin and KnownAssignedXidsSearch
2) it is closing 13-year-old tech depthBut in reality, it is not easy to measure performance improvement
consistently for this change.
Okay. Elsewhere, it seems like folks are fine with patches that reduce
shared memory space via atomics or barriers even if there's no immediate
benefit [0]/messages/by-id/20230524214958.mt6f5xokpumvnrio@awork3.anarazel.de, so I think it's fine to proceed.
Are the assignments in question guaranteed to be atomic? IIUC we assume
that aligned 4-byte loads/stores are atomic, so we should be okay as long
as we aren't handling anything larger.Yes, 4-bytes assignment are atomic, locking is used to ensure memory
write ordering in this place.
Yeah, it looks like both the values that are protected by
known_assigned_xids_lck are integers, so this should be okay. 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 wouldn't prevent such mismatches even today, since we use a separate
spinlock for reading them in some cases.
[0]: /messages/by-id/20230524214958.mt6f5xokpumvnrio@awork3.anarazel.de
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
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 wouldn't prevent such mismatches even today, since we use a separate
spinlock for reading them in some cases.
1) "The convention is that backends must hold shared ProcArrayLock to
examine the array", it is applied to pointers as well
2) Also, "only the startup process modifies the head/tail pointers."
So, the "tail" variable is updated by the startup process with
ProcArrayLock held in exclusive-only mode - so, no issues here.
Regarding "head" variable - updates by the startup processes are
possible in next cases:
* ProcArrayLock in exclusive mode (like KnownAssignedXidsCompress or
KnownAssignedXidsSearch(remove=true)), no issues here
* ProcArrayLock not taken at all (like
KnownAssignedXidsAdd(exclusive_lock=false)) in such case we rely on
memory barrier machinery
Both head and tail variables are changed only with exclusive lock held.
I'll think more, but can't find something wrong here so far.
Attachments:
v3-0001-Removes-known_assigned_xids_lck-using-the-write-m.patchtext/x-patch; charset=US-ASCII; name=v3-0001-Removes-known_assigned_xids_lck-using-the-write-m.patchDownload
diff --git a/src/backend/storage/ipc/procarray.c b/src/backend/storage/ipc/procarray.c
index 2a3da49b8f..e4093ed06d 100644
--- a/src/backend/storage/ipc/procarray.c
+++ b/src/backend/storage/ipc/procarray.c
@@ -61,7 +61,6 @@
#include "port/pg_lfind.h"
#include "storage/proc.h"
#include "storage/procarray.h"
-#include "storage/spin.h"
#include "utils/acl.h"
#include "utils/builtins.h"
#include "utils/rel.h"
@@ -82,7 +81,6 @@ typedef struct ProcArrayStruct
int numKnownAssignedXids; /* current # of valid entries */
int tailKnownAssignedXids; /* index of oldest valid element */
int headKnownAssignedXids; /* index of newest element, + 1 */
- slock_t known_assigned_xids_lck; /* protects head/tail pointers */
/*
* Highest subxid that has been removed from KnownAssignedXids array to
@@ -441,7 +439,6 @@ CreateSharedProcArray(void)
procArray->numKnownAssignedXids = 0;
procArray->tailKnownAssignedXids = 0;
procArray->headKnownAssignedXids = 0;
- SpinLockInit(&procArray->known_assigned_xids_lck);
procArray->lastOverflowedXid = InvalidTransactionId;
procArray->replication_slot_xmin = InvalidTransactionId;
procArray->replication_slot_catalog_xmin = InvalidTransactionId;
@@ -4564,11 +4561,11 @@ KnownAssignedTransactionIdsIdleMaintenance(void)
* pointer. This wouldn't require any lock at all, except that on machines
* with weak memory ordering we need to be careful that other processors
* see the array element changes before they see the head pointer change.
- * We handle this by using a spinlock to protect reads and writes of the
- * head/tail pointers. (We could dispense with the spinlock if we were to
- * create suitable memory access barrier primitives and use those instead.)
- * The spinlock must be taken to read or write the head/tail pointers unless
- * the caller holds ProcArrayLock exclusively.
+ * We handle this by using a memory barrier to protect writes of the
+ * head pointer.
+ * The memory barrier is taken before write the head pointer unless
+ * the caller holds ProcArrayLock exclusively. Appropriate read memory barrier
+ * is taken accordingly.
*
* Algorithmic analysis:
*
@@ -4755,7 +4752,7 @@ KnownAssignedXidsAdd(TransactionId from_xid, TransactionId to_xid,
/*
* Since only the startup process modifies the head/tail pointers, we
- * don't need a lock to read them here.
+ * are safe read them here.
*/
head = pArray->headKnownAssignedXids;
tail = pArray->tailKnownAssignedXids;
@@ -4806,22 +4803,17 @@ KnownAssignedXidsAdd(TransactionId from_xid, TransactionId to_xid,
pArray->numKnownAssignedXids += nxids;
/*
- * Now update the head pointer. We use a spinlock to protect this
+ * Now update the head pointer. We use a memory barrier to protect this
* pointer, not because the update is likely to be non-atomic, but to
* ensure that other processors see the above array updates before they
* see the head pointer change.
*
* If we're holding ProcArrayLock exclusively, there's no need to take the
- * spinlock.
+ * barrier.
*/
- if (exclusive_lock)
- pArray->headKnownAssignedXids = head;
- else
- {
- SpinLockAcquire(&pArray->known_assigned_xids_lck);
- pArray->headKnownAssignedXids = head;
- SpinLockRelease(&pArray->known_assigned_xids_lck);
- }
+ if (!exclusive_lock)
+ pg_write_barrier();
+ pArray->headKnownAssignedXids = head;
}
/*
@@ -4843,20 +4835,12 @@ KnownAssignedXidsSearch(TransactionId xid, bool remove)
int tail;
int result_index = -1;
- if (remove)
- {
- /* we hold ProcArrayLock exclusively, so no need for spinlock */
- tail = pArray->tailKnownAssignedXids;
- head = pArray->headKnownAssignedXids;
- }
- else
- {
- /* take spinlock to ensure we see up-to-date array contents */
- SpinLockAcquire(&pArray->known_assigned_xids_lck);
- tail = pArray->tailKnownAssignedXids;
- head = pArray->headKnownAssignedXids;
- SpinLockRelease(&pArray->known_assigned_xids_lck);
- }
+ tail = pArray->tailKnownAssignedXids;
+ head = pArray->headKnownAssignedXids;
+
+ /* in case of remove we hold ProcArrayLock exclusively, so no need for barrier */
+ if (!remove)
+ pg_read_barrier();
/*
* Standard binary search. Note we can ignore the KnownAssignedXidsValid
@@ -5095,12 +5079,11 @@ KnownAssignedXidsGetAndSetXmin(TransactionId *xarray, TransactionId *xmin,
* might miss newly-added xids, but they should be >= xmax so irrelevant
* anyway.
*
- * Must take spinlock to ensure we see up-to-date array contents.
+ * Must take read memory barrier to ensure we see up-to-date array contents.
*/
- SpinLockAcquire(&procArray->known_assigned_xids_lck);
tail = procArray->tailKnownAssignedXids;
head = procArray->headKnownAssignedXids;
- SpinLockRelease(&procArray->known_assigned_xids_lck);
+ pg_read_barrier();
for (i = tail; i < head; i++)
{
@@ -5147,10 +5130,9 @@ KnownAssignedXidsGetOldestXmin(void)
/*
* Fetch head just once, since it may change while we loop.
*/
- SpinLockAcquire(&procArray->known_assigned_xids_lck);
tail = procArray->tailKnownAssignedXids;
head = procArray->headKnownAssignedXids;
- SpinLockRelease(&procArray->known_assigned_xids_lck);
+ pg_read_barrier();
for (i = tail; i < head; i++)
{
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 barrier stuff is only applicable to KnownAssignedXidsAdd()
(without an exclusive lock) when we add entries to the end of the array and
then update the head pointer. Otherwise, appropriate locks are taken when
reading/writing the array. For example, say we have the following array:
head
|
v
[ 0, 1, 2, 3 ]
When adding elements, we keep the head pointer where it is:
head
|
v
[ 0, 1, 2, 3, 4, 5 ]
If another processor sees this intermediate state, it's okay because it
will only inspect elements 0 through 3. Only at the end do we update the
head pointer:
head
|
v
[ 0, 1, 2, 3, 4, 5 ]
With weak memory ordering and no barriers, another process may see this
(which is obviously no good):
head
|
v
[ 0, 1, 2, 3 ]
One thing that I'm still trying to understand is this code in
KnownAssignedXidsSearch():
/* we hold ProcArrayLock exclusively, so no need for spinlock */
tail = pArray->tailKnownAssignedXids;
head = pArray->headKnownAssignedXids;
It's not clear to me why holding ProcArrayLock exclusively means we don't
need to worry about the spinlock/barriers. If KnownAssignedXidsAdd() adds
entries without a lock, holding ProcArrayLock won't protect you, and I
don't see anything else that acts as a read barrier before the array
entries are inspected.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
Attachments:
v4-0001-Replace-known_assigned_xids_lck-with-memory-barri.patchtext/x-diff; charset=us-asciiDownload
From 11de5076adc060c0dde32e8538714301f9b98d02 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathan@postgresql.org>
Date: Wed, 16 Aug 2023 10:52:56 -0700
Subject: [PATCH v4 1/1] Replace known_assigned_xids_lck with memory barriers.
Suggested-by: Tom Lane
Author: Michail Nikolaev
Discussion: https://postgr.es/m/CANtu0oh0si%3DjG5z_fLeFtmYcETssQ08kLEa8b6TQqDm_cinroA%40mail.gmail.com
---
src/backend/storage/ipc/procarray.c | 71 ++++++++++-------------------
1 file changed, 25 insertions(+), 46 deletions(-)
diff --git a/src/backend/storage/ipc/procarray.c b/src/backend/storage/ipc/procarray.c
index 2a3da49b8f..a3def020b6 100644
--- a/src/backend/storage/ipc/procarray.c
+++ b/src/backend/storage/ipc/procarray.c
@@ -61,7 +61,6 @@
#include "port/pg_lfind.h"
#include "storage/proc.h"
#include "storage/procarray.h"
-#include "storage/spin.h"
#include "utils/acl.h"
#include "utils/builtins.h"
#include "utils/rel.h"
@@ -82,7 +81,6 @@ typedef struct ProcArrayStruct
int numKnownAssignedXids; /* current # of valid entries */
int tailKnownAssignedXids; /* index of oldest valid element */
int headKnownAssignedXids; /* index of newest element, + 1 */
- slock_t known_assigned_xids_lck; /* protects head/tail pointers */
/*
* Highest subxid that has been removed from KnownAssignedXids array to
@@ -441,7 +439,6 @@ CreateSharedProcArray(void)
procArray->numKnownAssignedXids = 0;
procArray->tailKnownAssignedXids = 0;
procArray->headKnownAssignedXids = 0;
- SpinLockInit(&procArray->known_assigned_xids_lck);
procArray->lastOverflowedXid = InvalidTransactionId;
procArray->replication_slot_xmin = InvalidTransactionId;
procArray->replication_slot_catalog_xmin = InvalidTransactionId;
@@ -4561,14 +4558,10 @@ KnownAssignedTransactionIdsIdleMaintenance(void)
* during normal running). Compressing unused entries out of the array
* likewise requires exclusive lock. To add XIDs to the array, we just insert
* them into slots to the right of the head pointer and then advance the head
- * pointer. This wouldn't require any lock at all, except that on machines
- * with weak memory ordering we need to be careful that other processors
- * see the array element changes before they see the head pointer change.
- * We handle this by using a spinlock to protect reads and writes of the
- * head/tail pointers. (We could dispense with the spinlock if we were to
- * create suitable memory access barrier primitives and use those instead.)
- * The spinlock must be taken to read or write the head/tail pointers unless
- * the caller holds ProcArrayLock exclusively.
+ * pointer. This doesn't require any lock at all, but on machines with weak
+ * memory ordering, we need to be careful that other processors see the array
+ * element changes before they see the head pointer change. We handle this by
+ * using memory barriers.
*
* Algorithmic analysis:
*
@@ -4806,22 +4799,15 @@ KnownAssignedXidsAdd(TransactionId from_xid, TransactionId to_xid,
pArray->numKnownAssignedXids += nxids;
/*
- * Now update the head pointer. We use a spinlock to protect this
- * pointer, not because the update is likely to be non-atomic, but to
- * ensure that other processors see the above array updates before they
- * see the head pointer change.
- *
- * If we're holding ProcArrayLock exclusively, there's no need to take the
- * spinlock.
+ * Now update the head pointer. We use a write barrier to ensure that
+ * other processors see the above array updates before they see the head
+ * pointer change. The barrier isn't required if we're holding
+ * ProcArrayLock exclusively.
*/
- if (exclusive_lock)
- pArray->headKnownAssignedXids = head;
- else
- {
- SpinLockAcquire(&pArray->known_assigned_xids_lck);
- pArray->headKnownAssignedXids = head;
- SpinLockRelease(&pArray->known_assigned_xids_lck);
- }
+ if (!exclusive_lock)
+ pg_write_barrier();
+
+ pArray->headKnownAssignedXids = head;
}
/*
@@ -4843,20 +4829,15 @@ KnownAssignedXidsSearch(TransactionId xid, bool remove)
int tail;
int result_index = -1;
- if (remove)
- {
- /* we hold ProcArrayLock exclusively, so no need for spinlock */
- tail = pArray->tailKnownAssignedXids;
- head = pArray->headKnownAssignedXids;
- }
- else
- {
- /* take spinlock to ensure we see up-to-date array contents */
- SpinLockAcquire(&pArray->known_assigned_xids_lck);
- tail = pArray->tailKnownAssignedXids;
- head = pArray->headKnownAssignedXids;
- SpinLockRelease(&pArray->known_assigned_xids_lck);
- }
+ tail = pArray->tailKnownAssignedXids;
+ head = pArray->headKnownAssignedXids;
+
+ /*
+ * If we know that we're holding ProcArrayLock exclusively, we don't need
+ * the read barrier.
+ */
+ if (!remove)
+ pg_read_barrier(); /* pairs with KnownAssignedXidsAdd */
/*
* Standard binary search. Note we can ignore the KnownAssignedXidsValid
@@ -5094,13 +5075,11 @@ KnownAssignedXidsGetAndSetXmin(TransactionId *xarray, TransactionId *xmin,
* cannot enter and then leave the array while we hold ProcArrayLock. We
* might miss newly-added xids, but they should be >= xmax so irrelevant
* anyway.
- *
- * Must take spinlock to ensure we see up-to-date array contents.
*/
- SpinLockAcquire(&procArray->known_assigned_xids_lck);
tail = procArray->tailKnownAssignedXids;
head = procArray->headKnownAssignedXids;
- SpinLockRelease(&procArray->known_assigned_xids_lck);
+
+ pg_read_barrier(); /* pairs with KnownAssignedXidsAdd */
for (i = tail; i < head; i++)
{
@@ -5147,10 +5126,10 @@ KnownAssignedXidsGetOldestXmin(void)
/*
* Fetch head just once, since it may change while we loop.
*/
- SpinLockAcquire(&procArray->known_assigned_xids_lck);
tail = procArray->tailKnownAssignedXids;
head = procArray->headKnownAssignedXids;
- SpinLockRelease(&procArray->known_assigned_xids_lck);
+
+ pg_read_barrier(); /* pairs with KnownAssignedXidsAdd */
for (i = tail; i < head; i++)
{
--
2.25.1
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 KnownAssignedXids"
Best regards,
Mikhail.
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 can remove and add xids to KnownAssignedXids"
Ah, that explains it. v5 of the patch is attached.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
Attachments:
v5-0001-Replace-known_assigned_xids_lck-with-memory-barri.patchtext/x-diff; charset=us-asciiDownload
From 6038182ac96226f0eec43b1c6ab2e060e8b1e3a8 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathan@postgresql.org>
Date: Wed, 16 Aug 2023 10:52:56 -0700
Subject: [PATCH v5 1/1] Replace known_assigned_xids_lck with memory barriers.
Suggested-by: Tom Lane
Author: Michail Nikolaev
Discussion: https://postgr.es/m/CANtu0oh0si%3DjG5z_fLeFtmYcETssQ08kLEa8b6TQqDm_cinroA%40mail.gmail.com
---
src/backend/storage/ipc/procarray.c | 72 +++++++++++------------------
1 file changed, 26 insertions(+), 46 deletions(-)
diff --git a/src/backend/storage/ipc/procarray.c b/src/backend/storage/ipc/procarray.c
index 2a3da49b8f..401e816275 100644
--- a/src/backend/storage/ipc/procarray.c
+++ b/src/backend/storage/ipc/procarray.c
@@ -61,7 +61,6 @@
#include "port/pg_lfind.h"
#include "storage/proc.h"
#include "storage/procarray.h"
-#include "storage/spin.h"
#include "utils/acl.h"
#include "utils/builtins.h"
#include "utils/rel.h"
@@ -82,7 +81,6 @@ typedef struct ProcArrayStruct
int numKnownAssignedXids; /* current # of valid entries */
int tailKnownAssignedXids; /* index of oldest valid element */
int headKnownAssignedXids; /* index of newest element, + 1 */
- slock_t known_assigned_xids_lck; /* protects head/tail pointers */
/*
* Highest subxid that has been removed from KnownAssignedXids array to
@@ -441,7 +439,6 @@ CreateSharedProcArray(void)
procArray->numKnownAssignedXids = 0;
procArray->tailKnownAssignedXids = 0;
procArray->headKnownAssignedXids = 0;
- SpinLockInit(&procArray->known_assigned_xids_lck);
procArray->lastOverflowedXid = InvalidTransactionId;
procArray->replication_slot_xmin = InvalidTransactionId;
procArray->replication_slot_catalog_xmin = InvalidTransactionId;
@@ -4561,14 +4558,11 @@ KnownAssignedTransactionIdsIdleMaintenance(void)
* during normal running). Compressing unused entries out of the array
* likewise requires exclusive lock. To add XIDs to the array, we just insert
* them into slots to the right of the head pointer and then advance the head
- * pointer. This wouldn't require any lock at all, except that on machines
- * with weak memory ordering we need to be careful that other processors
- * see the array element changes before they see the head pointer change.
- * We handle this by using a spinlock to protect reads and writes of the
- * head/tail pointers. (We could dispense with the spinlock if we were to
- * create suitable memory access barrier primitives and use those instead.)
- * The spinlock must be taken to read or write the head/tail pointers unless
- * the caller holds ProcArrayLock exclusively.
+ * pointer. This doesn't require any lock at all, but on machines with weak
+ * memory ordering, we need to be careful that other processors see the array
+ * element changes before they see the head pointer change. We handle this by
+ * using memory barriers when reading or writing the head/tail pointers (unless
+ * the caller holds ProcArrayLock exclusively).
*
* Algorithmic analysis:
*
@@ -4806,22 +4800,15 @@ KnownAssignedXidsAdd(TransactionId from_xid, TransactionId to_xid,
pArray->numKnownAssignedXids += nxids;
/*
- * Now update the head pointer. We use a spinlock to protect this
- * pointer, not because the update is likely to be non-atomic, but to
- * ensure that other processors see the above array updates before they
- * see the head pointer change.
- *
- * If we're holding ProcArrayLock exclusively, there's no need to take the
- * spinlock.
+ * Now update the head pointer. We use a write barrier to ensure that
+ * other processors see the above array updates before they see the head
+ * pointer change. The barrier isn't required if we're holding
+ * ProcArrayLock exclusively.
*/
- if (exclusive_lock)
- pArray->headKnownAssignedXids = head;
- else
- {
- SpinLockAcquire(&pArray->known_assigned_xids_lck);
- pArray->headKnownAssignedXids = head;
- SpinLockRelease(&pArray->known_assigned_xids_lck);
- }
+ if (!exclusive_lock)
+ pg_write_barrier();
+
+ pArray->headKnownAssignedXids = head;
}
/*
@@ -4843,20 +4830,15 @@ KnownAssignedXidsSearch(TransactionId xid, bool remove)
int tail;
int result_index = -1;
- if (remove)
- {
- /* we hold ProcArrayLock exclusively, so no need for spinlock */
- tail = pArray->tailKnownAssignedXids;
- head = pArray->headKnownAssignedXids;
- }
- else
- {
- /* take spinlock to ensure we see up-to-date array contents */
- SpinLockAcquire(&pArray->known_assigned_xids_lck);
- tail = pArray->tailKnownAssignedXids;
- head = pArray->headKnownAssignedXids;
- SpinLockRelease(&pArray->known_assigned_xids_lck);
- }
+ tail = pArray->tailKnownAssignedXids;
+ head = pArray->headKnownAssignedXids;
+
+ /*
+ * Only the startup process removes entries, so we don't need the read
+ * barrier in that case.
+ */
+ if (!remove)
+ pg_read_barrier(); /* pairs with KnownAssignedXidsAdd */
/*
* Standard binary search. Note we can ignore the KnownAssignedXidsValid
@@ -5094,13 +5076,11 @@ KnownAssignedXidsGetAndSetXmin(TransactionId *xarray, TransactionId *xmin,
* cannot enter and then leave the array while we hold ProcArrayLock. We
* might miss newly-added xids, but they should be >= xmax so irrelevant
* anyway.
- *
- * Must take spinlock to ensure we see up-to-date array contents.
*/
- SpinLockAcquire(&procArray->known_assigned_xids_lck);
tail = procArray->tailKnownAssignedXids;
head = procArray->headKnownAssignedXids;
- SpinLockRelease(&procArray->known_assigned_xids_lck);
+
+ pg_read_barrier(); /* pairs with KnownAssignedXidsAdd */
for (i = tail; i < head; i++)
{
@@ -5147,10 +5127,10 @@ KnownAssignedXidsGetOldestXmin(void)
/*
* Fetch head just once, since it may change while we loop.
*/
- SpinLockAcquire(&procArray->known_assigned_xids_lck);
tail = procArray->tailKnownAssignedXids;
head = procArray->headKnownAssignedXids;
- SpinLockRelease(&procArray->known_assigned_xids_lck);
+
+ pg_read_barrier(); /* pairs with KnownAssignedXidsAdd */
for (i = tail; i < head; i++)
{
--
2.25.1
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
On Fri, Sep 1, 2023 at 3:41 PM Nathan Bossart <nathandbossart@gmail.com> 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 but I looked at this patch briefly and
it seems OK to me.
--
Robert Haas
EDB: http://www.enterprisedb.com
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