Re: track_planning causing performance regression - Mailing list pgsql-hackers

On 2020/07/01 4:03, Andres Freund wrote:
> 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.

Understood. Thanks!



> 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.

Because pgssEntry is not array entry but hashtable entry. First I was
thinking to assign per-process lwlock to each entry in the array at the
startup. But each entry is created every time new entry is required.
So lwlock needs to be assigned to each entry at that creation time.
We cannnot easily assign lwlock to all the entries at the startup.

Also each entry can be dropped from the hashtable. In this case,
maybe already-assigned lwlock needs to be moved back to "freelist"
so that it will be able to be assigned again to new entry later. We can
implement this probably, but which looks a bit complicated.

Since the hasing addresses these issues, I just used it in POC patch.
But I'd like to hear better idea!

> +#define      PGSS_NUM_LOCK_PARTITIONS()              (pgss_max)

Currently pgss_max is used as the number of lwlock for entries.
But if too large number of lwlock is useless (or a bit harmful?), we can
set the upper limit here, e.g., max(pgss_max, 10000).

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION