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