Hi, On 2020-06-30 14:43:39 +0900, Fujii Masao wrote: > > I did a prototype patch that replaces spinlocks with futexes, but was not > > able to find a workload where it mattered. > > I'm not familiar with futex, but could you tell me why you used futex instead > of LWLock that we already have? Is futex portable?
We can't rely on futexes, they're linux only. I also don't see much of a reason to use spinlocks (rather than lwlocks) here in the first place. > diff --git a/contrib/pg_stat_statements/pg_stat_statements.c > b/contrib/pg_stat_statements/pg_stat_statements.c > index cef8bb5a49..aa506f6c11 100644 > --- a/contrib/pg_stat_statements/pg_stat_statements.c > +++ b/contrib/pg_stat_statements/pg_stat_statements.c > @@ -39,7 +39,7 @@ > * in an entry except the counters requires the same. To look up an entry, > * one must hold the lock shared. To read or update the counters within > * an entry, one must hold the lock shared or exclusive (so the entry doesn't > - * disappear!) and also take the entry's mutex spinlock. > + * disappear!) and also take the entry's partition lock. > * The shared state variable pgss->extent (the next free spot in the external > * query-text file) should be accessed only while holding either the > * pgss->mutex spinlock, or exclusive lock on pgss->lock. We use the mutex > to > @@ -115,6 +115,11 @@ static const uint32 PGSS_PG_MAJOR_VERSION = > PG_VERSION_NUM / 100; > > #define JUMBLE_SIZE 1024 /* query serialization > buffer size */ > > +#define PGSS_NUM_LOCK_PARTITIONS() (pgss_max) > +#define PGSS_HASH_PARTITION_LOCK(key) \ > + (&(pgss->base + \ > + (get_hash_value(pgss_hash, key) % PGSS_NUM_LOCK_PARTITIONS()))->lock) > + > /* > * Extension version number, for supporting older extension versions' objects > */ > @@ -207,7 +212,7 @@ typedef struct pgssEntry > Size query_offset; /* query text offset in external file */ > int query_len; /* # of valid bytes in > query string, or -1 */ > int encoding; /* query text encoding > */ > - slock_t mutex; /* protects the counters only */ > + LWLock *lock; /* protects the counters only */ > } pgssEntry; Why did you add the hashing here? It seems a lot better to just add an lwlock in-place instead of the spinlock? The added size is neglegible compared to the size of pgssEntry. Greetings, Andres Freund
