On Fri, Aug 15, 2025 at 01:39:52PM -0400, Andres Freund wrote:
> On 2025-08-14 11:29:08 +0200, Álvaro Herrera wrote:
>> However, changing that spinlock to an lwlock doesn't look easy, because of
>> the way each pgss entry is created as a dynahash entry, and then deallocated
>> from there. With spinlocks we can just reinit the spinlock each time, but
>> that doesn't work with lwlocks. We have no easy way to associate then
>> disassociate each entry from a specific lwlock.
>
> I'm not following? The lwlock can just be inside the struct, just like the
> spinlock is? "Association" is just LWLockInitialize() and deassociation is not
> needed.
Indeed. I rebased an old patch that I had lying around to demonstrate. If
my past testing [0] is to be trusted, this actually hurts performance,
unfortunately.
[0] https://postgr.es/m/Zs4hJ6-Fg8DMgU_P@nathan
--
nathan
>From ecb70a2bb492c5ca47e86cadbce91d395f011316 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nat...@postgresql.org>
Date: Thu, 1 Aug 2024 09:30:32 -0500
Subject: [PATCH v1 1/1] replace p_s_s entry spinlocks with lwlocks
---
contrib/pg_stat_statements/pg_stat_statements.c | 16 ++++++++++------
1 file changed, 10 insertions(+), 6 deletions(-)
diff --git a/contrib/pg_stat_statements/pg_stat_statements.c
b/contrib/pg_stat_statements/pg_stat_statements.c
index 9fc9635d330..d1fdd77acc3 100644
--- a/contrib/pg_stat_statements/pg_stat_statements.c
+++ b/contrib/pg_stat_statements/pg_stat_statements.c
@@ -240,7 +240,7 @@ typedef struct pgssEntry
int encoding; /* query text encoding
*/
TimestampTz stats_since; /* timestamp of entry allocation */
TimestampTz minmax_stats_since; /* timestamp of last min/max values
reset */
- slock_t mutex; /* protects the counters only */
+ LWLock mutex; /* protects the counters only */
} pgssEntry;
/*
@@ -256,6 +256,7 @@ typedef struct pgssSharedState
int n_writers; /* number of active
writers to query file */
int gc_count; /* query file garbage
collection cycle count */
pgssGlobalStats stats; /* global statistics for pgss */
+ int tranche;
} pgssSharedState;
/*---- Local variables ----*/
@@ -554,6 +555,7 @@ pgss_shmem_startup(void)
pgss->gc_count = 0;
pgss->stats.dealloc = 0;
pgss->stats.stats_reset = GetCurrentTimestamp();
+ pgss->tranche = LWLockNewTrancheId();
}
info.keysize = sizeof(pgssHashKey);
@@ -565,6 +567,8 @@ pgss_shmem_startup(void)
LWLockRelease(AddinShmemInitLock);
+ LWLockRegisterTranche(pgss->tranche, "pg_stat_statements counters");
+
/*
* If we're in the postmaster (or a standalone backend...), set up a
shmem
* exit hook to dump the statistics to disk.
@@ -1411,7 +1415,7 @@ pgss_store(const char *query, int64 queryId,
* Grab the spinlock while updating the counters (see comment
about
* locking rules at the head of the file)
*/
- SpinLockAcquire(&entry->mutex);
+ LWLockAcquire(&entry->mutex, LW_EXCLUSIVE);
/* "Unstick" entry if it was previously sticky */
if (IS_STICKY(entry->counters))
@@ -1511,7 +1515,7 @@ pgss_store(const char *query, int64 queryId,
else if (planOrigin == PLAN_STMT_CACHE_CUSTOM)
entry->counters.custom_plan_calls++;
- SpinLockRelease(&entry->mutex);
+ LWLockRelease(&entry->mutex);
}
done:
@@ -1901,9 +1905,9 @@ pg_stat_statements_internal(FunctionCallInfo fcinfo,
}
/* copy counters to a local variable to keep locking time short
*/
- SpinLockAcquire(&entry->mutex);
+ LWLockAcquire(&entry->mutex, LW_SHARED);
tmp = entry->counters;
- SpinLockRelease(&entry->mutex);
+ LWLockRelease(&entry->mutex);
/*
* The spinlock is not required when reading these two as they
are
@@ -2134,7 +2138,7 @@ entry_alloc(pgssHashKey *key, Size query_offset, int
query_len, int encoding,
/* set the appropriate initial usage count */
entry->counters.usage = sticky ? pgss->cur_median_usage :
USAGE_INIT;
/* re-initialize the mutex each time ... we assume no one using
it */
- SpinLockInit(&entry->mutex);
+ LWLockInitialize(&entry->mutex, pgss->tranche);
/* ... and don't forget the query text metadata */
Assert(query_len >= 0);
entry->query_offset = query_offset;
--
2.39.5 (Apple Git-154)
