public inbox for [email protected]
help / color / mirror / Atom feedFrom: Nathan Bossart <[email protected]>
To: Michail Nikolaev <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Cc: Tom Lane <[email protected]>
Subject: Re: Replace known_assigned_xids_lck by memory barrier
Date: Wed, 16 Aug 2023 13:07:15 -0700
Message-ID: <20230816200715.GB2908487@nathanxps13> (raw)
In-Reply-To: <CANtu0ohXFhY2gq=D9cHFLfJjLrNMOGcUGR5thJCSpqweNnqbpQ@mail.gmail.com>
References: <CANtu0oh0si=jG5z_fLeFtmYcETssQ08kLEa8b6TQqDm_cinroA@mail.gmail.com>
<20230814153634.GB1395427@nathanxps13>
<CANtu0ohnGXW6wfM7eXts=m23A1g9D7s1X0ms7bV3-x5oQugX5g@mail.gmail.com>
<20230815152224.GA2296544@nathanxps13>
<CANtu0ogdo86z-+oQVT7LENPPe_W4N5Q4HsisM_Xpf-H=v5vaow@mail.gmail.com>
<20230816183236.GA2812457@nathanxps13>
<CANtu0ohXFhY2gq=D9cHFLfJjLrNMOGcUGR5thJCSpqweNnqbpQ@mail.gmail.com>
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:
[text/x-diff] v5-0001-Replace-known_assigned_xids_lck-with-memory-barri.patch (5.9K, ../20230816200715.GB2908487@nathanxps13/2-v5-0001-Replace-known_assigned_xids_lck-with-memory-barri.patch)
download | inline diff:
From 6038182ac96226f0eec43b1c6ab2e060e8b1e3a8 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email protected]>
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
view thread (3+ messages)
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]
Subject: Re: Replace known_assigned_xids_lck by memory barrier
In-Reply-To: <20230816200715.GB2908487@nathanxps13>
* 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