public inbox for [email protected]
help / color / mirror / Atom feedRe: simplify regular expression locale global variables
3+ messages / 3 participants
[nested] [flat]
* Re: simplify regular expression locale global variables
@ 2024-10-15 10:08 Alvaro Herrera <[email protected]>
0 siblings, 1 reply; 3+ messages in thread
From: Alvaro Herrera @ 2024-10-15 10:08 UTC (permalink / raw)
To: Peter Eisentraut <[email protected]>; +Cc: pgsql-hackers
On 2024-Oct-15, Peter Eisentraut wrote:
> @@ -253,8 +241,9 @@ pg_set_regex_collation(Oid collation)
> * catalog access is available, so we can't call
> * pg_newlocale_from_collation().
> */
> + static struct pg_locale_struct dummy_locale = {.ctype_is_c = true};
> +
> + locale = &dummy_locale;
> }
> else
> {
> @@ -264,121 +253,80 @@ pg_set_regex_collation(Oid collation)
> ereport(ERROR,
> (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> errmsg("nondeterministic collations are not supported for regular expressions")));
> [...]
> }
>
> pg_regex_locale = locale;
> }
Hmm, is it valid to make pg_regex_locale point to a function-local
static here? The lifetime of this static is not clear to me, and I
think this pattern works with at least some compilers, but I remember
comments on previous patch review threads that this pattern isn't
kosher.
--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
^ permalink raw reply [nested|flat] 3+ messages in thread
* Re: simplify regular expression locale global variables
@ 2024-10-15 13:27 Peter Eisentraut <[email protected]>
parent: Alvaro Herrera <[email protected]>
0 siblings, 0 replies; 3+ messages in thread
From: Peter Eisentraut @ 2024-10-15 13:27 UTC (permalink / raw)
To: Alvaro Herrera <[email protected]>; +Cc: pgsql-hackers
On 15.10.24 12:08, Alvaro Herrera wrote:
> On 2024-Oct-15, Peter Eisentraut wrote:
>
>> @@ -253,8 +241,9 @@ pg_set_regex_collation(Oid collation)
>> * catalog access is available, so we can't call
>> * pg_newlocale_from_collation().
>> */
>> + static struct pg_locale_struct dummy_locale = {.ctype_is_c = true};
>> +
>> + locale = &dummy_locale;
>> }
>> else
>> {
>> @@ -264,121 +253,80 @@ pg_set_regex_collation(Oid collation)
>> ereport(ERROR,
>> (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
>> errmsg("nondeterministic collations are not supported for regular expressions")));
>> [...]
>> }
>>
>> pg_regex_locale = locale;
>> }
>
> Hmm, is it valid to make pg_regex_locale point to a function-local
> static here? The lifetime of this static is not clear to me, and I
> think this pattern works with at least some compilers, but I remember
> comments on previous patch review threads that this pattern isn't
> kosher.
I think this must be okay. Some classic non-thread-safe C library
functions essentially work that way, e.g.,
char *strerror(int errnum)
{
static char buf[...];
strcpy(buf, ....);
return buf;
}
and then you can use the return pointer wherever you want.
^ permalink raw reply [nested|flat] 3+ messages in thread
* [PATCH v1 1/8] convert SISeg->maxMsgNum to an atomic
@ 2026-07-09 19:20 Nathan Bossart <[email protected]>
0 siblings, 0 replies; 3+ messages in thread
From: Nathan Bossart @ 2026-07-09 19:20 UTC (permalink / raw)
---
src/backend/storage/ipc/sinvaladt.c | 51 ++++++++++-------------------
1 file changed, 17 insertions(+), 34 deletions(-)
diff --git a/src/backend/storage/ipc/sinvaladt.c b/src/backend/storage/ipc/sinvaladt.c
index 37a21ffaf1a..17e98c9efdc 100644
--- a/src/backend/storage/ipc/sinvaladt.c
+++ b/src/backend/storage/ipc/sinvaladt.c
@@ -24,7 +24,6 @@
#include "storage/procsignal.h"
#include "storage/shmem.h"
#include "storage/sinvaladt.h"
-#include "storage/spin.h"
#include "storage/subsystems.h"
/*
@@ -87,19 +86,10 @@
* has no need to touch anyone's ProcState, except in the infrequent cases
* when SICleanupQueue is needed. The only point of overlap is that
* the writer wants to change maxMsgNum while readers need to read it.
- * We deal with that by having a spinlock that readers must take for just
- * long enough to read maxMsgNum, while writers take it for just long enough
- * to write maxMsgNum. (The exact rule is that you need the spinlock to
- * read maxMsgNum if you are not holding SInvalWriteLock, and you need the
- * spinlock to write maxMsgNum unless you are holding both locks.)
- *
- * Note: since maxMsgNum is an int and hence presumably atomically readable/
- * writable, the spinlock might seem unnecessary. The reason it is needed
- * is to provide a memory barrier: we need to be sure that messages written
- * to the array are actually there before maxMsgNum is increased, and that
- * readers will see that data after fetching maxMsgNum. Multiprocessors
- * that have weak memory-ordering guarantees can fail without the memory
- * barrier instructions that are included in the spinlock sequences.
+ * We deal with that by making maxMsgNum an atomic variable. (The exact rule
+ * is that you need to use a barrier-providing accessor to read maxMsgNum if
+ * you are not holding SInvalWriteLock, and you need a barrier-providing
+ * accessor to write maxMsgNum unless you are holding both locks.)
*/
@@ -169,11 +159,9 @@ typedef struct SISeg
* General state information
*/
int minMsgNum; /* oldest message still needed */
- int maxMsgNum; /* next message number to be assigned */
+ pg_atomic_uint32 maxMsgNum; /* next message number to be assigned */
int nextThreshold; /* # of messages to call SICleanupQueue */
- slock_t msgnumLock; /* spinlock protecting maxMsgNum */
-
/*
* Circular buffer holding shared-inval messages
*/
@@ -244,11 +232,10 @@ SharedInvalShmemInit(void *arg)
{
int i;
- /* Clear message counters, init spinlock */
+ /* Clear message counters */
shmInvalBuffer->minMsgNum = 0;
- shmInvalBuffer->maxMsgNum = 0;
+ pg_atomic_init_u32(&shmInvalBuffer->maxMsgNum, 0);
shmInvalBuffer->nextThreshold = CLEANUP_MIN;
- SpinLockInit(&shmInvalBuffer->msgnumLock);
/* The buffer[] array is initially all unused, so we need not fill it */
@@ -306,7 +293,7 @@ SharedInvalBackendInit(bool sendOnly)
/* mark myself active, with all extant messages already read */
stateP->procPid = MyProcPid;
- stateP->nextMsgNum = segP->maxMsgNum;
+ stateP->nextMsgNum = pg_atomic_read_u32(&segP->maxMsgNum);
stateP->resetState = false;
stateP->signaled = false;
stateP->hasMessages = false;
@@ -402,7 +389,7 @@ SIInsertDataEntries(const SharedInvalidationMessage *data, int n)
*/
for (;;)
{
- numMsgs = segP->maxMsgNum - segP->minMsgNum;
+ numMsgs = pg_atomic_read_u32(&segP->maxMsgNum) - segP->minMsgNum;
if (numMsgs + nthistime > MAXNUMMESSAGES ||
numMsgs >= segP->nextThreshold)
SICleanupQueue(true, nthistime);
@@ -413,17 +400,15 @@ SIInsertDataEntries(const SharedInvalidationMessage *data, int n)
/*
* Insert new message(s) into proper slot of circular buffer
*/
- max = segP->maxMsgNum;
+ max = pg_atomic_read_u32(&segP->maxMsgNum);
while (nthistime-- > 0)
{
segP->buffer[max % MAXNUMMESSAGES] = *data++;
max++;
}
- /* Update current value of maxMsgNum using spinlock */
- SpinLockAcquire(&segP->msgnumLock);
- segP->maxMsgNum = max;
- SpinLockRelease(&segP->msgnumLock);
+ /* Update current value of maxMsgNum using barrier */
+ pg_atomic_write_membarrier_u32(&segP->maxMsgNum, max);
/*
* Now that the maxMsgNum change is globally visible, we give everyone
@@ -509,10 +494,8 @@ SIGetDataEntries(SharedInvalidationMessage *data, int datasize)
*/
stateP->hasMessages = false;
- /* Fetch current value of maxMsgNum using spinlock */
- SpinLockAcquire(&segP->msgnumLock);
- max = segP->maxMsgNum;
- SpinLockRelease(&segP->msgnumLock);
+ /* Fetch current value of maxMsgNum using barrier */
+ max = pg_atomic_read_membarrier_u32(&segP->maxMsgNum);
if (stateP->resetState)
{
@@ -598,7 +581,7 @@ SICleanupQueue(bool callerHasWriteLock, int minFree)
* backends here it is possible for them to keep sending messages without
* a problem even when they are the only active backend.
*/
- min = segP->maxMsgNum;
+ min = pg_atomic_read_u32(&segP->maxMsgNum);
minsig = min - SIG_THRESHOLD;
lowbound = min - MAXNUMMESSAGES + minFree;
@@ -644,7 +627,7 @@ SICleanupQueue(bool callerHasWriteLock, int minFree)
if (min >= MSGNUMWRAPAROUND)
{
segP->minMsgNum -= MSGNUMWRAPAROUND;
- segP->maxMsgNum -= MSGNUMWRAPAROUND;
+ pg_atomic_fetch_sub_u32(&segP->maxMsgNum, MSGNUMWRAPAROUND);
for (i = 0; i < segP->numProcs; i++)
segP->procState[segP->pgprocnos[i]].nextMsgNum -= MSGNUMWRAPAROUND;
}
@@ -653,7 +636,7 @@ SICleanupQueue(bool callerHasWriteLock, int minFree)
* Determine how many messages are still in the queue, and set the
* threshold at which we should repeat SICleanupQueue().
*/
- numMsgs = segP->maxMsgNum - segP->minMsgNum;
+ numMsgs = pg_atomic_read_u32(&segP->maxMsgNum) - segP->minMsgNum;
if (numMsgs < CLEANUP_MIN)
segP->nextThreshold = CLEANUP_MIN;
else
--
2.50.1 (Apple Git-155)
--Ot5x3ffkWEuxilWO
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment;
filename=v1-0002-convert-ParallelBitmapHeapState-state-to-an-atomi.patch
^ permalink raw reply [nested|flat] 3+ messages in thread
end of thread, other threads:[~2026-07-09 19:20 UTC | newest]
Thread overview: 3+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2024-10-15 10:08 Re: simplify regular expression locale global variables Alvaro Herrera <[email protected]>
2024-10-15 13:27 ` Peter Eisentraut <[email protected]>
2026-07-09 19:20 [PATCH v1 1/8] convert SISeg->maxMsgNum to an atomic Nathan Bossart <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox