Re: track_planning causing performance regression

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