track_planning causing performance regression

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
32 messages Options
12
Reply | Threaded
Open this post in threaded view
|

Re: track_planning causing performance regression

Peter Geoghegan-4
On Tue, Jun 30, 2020 at 6:40 AM Fujii Masao <[hidden email]> wrote:
> Ants and Andres suggested to replace the spinlock used in pgss_store() with
> LWLock. I agreed with them and posted the POC patch doing that. But I think
> the patch is an item for v14. The patch may address the reported performance
> issue, but may cause other performance issues in other workloads. We would
> need to measure how the patch affects the performance in various workloads.
> It seems too late to do that at this stage of v13. Thought?

I agree that it's too late for v13.

Thanks
--
Peter Geoghegan


Reply | Threaded
Open this post in threaded view
|

Re: track_planning causing performance regression

Fujii Masao-4
In reply to this post by Andres Freund


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


Reply | Threaded
Open this post in threaded view
|

Re: track_planning causing performance regression

Andres Freund
Hi,

On 2020-07-01 22:20:50 +0900, Fujii Masao wrote:

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

But why not just do it exactly at the place the SpinLockInit() is done
currently?

Greetings,

Andres Freund


Reply | Threaded
Open this post in threaded view
|

Re: track_planning causing performance regression

Fujii Masao-4


On 2020/07/02 1:54, Andres Freund wrote:

> Hi,
>
> On 2020-07-01 22:20:50 +0900, Fujii Masao wrote:
>> On 2020/07/01 4:03, Andres Freund wrote:
>>> 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.
>
> But why not just do it exactly at the place the SpinLockInit() is done
> currently?

Sorry I failed to understand your point... You mean that new lwlock should
be initialized at the place the SpinLockInit() is done currently instead of
requesting postmaster to initialize all the lwlocks required for pgss
at _PG_init()?

Regards,


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


Reply | Threaded
Open this post in threaded view
|

Re: track_planning causing performance regression

Fujii Masao-4
In reply to this post by Peter Geoghegan-4


On 2020/07/01 7:37, Peter Geoghegan wrote:
> On Tue, Jun 30, 2020 at 6:40 AM Fujii Masao <[hidden email]> wrote:
>> Ants and Andres suggested to replace the spinlock used in pgss_store() with
>> LWLock. I agreed with them and posted the POC patch doing that. But I think
>> the patch is an item for v14. The patch may address the reported performance
>> issue, but may cause other performance issues in other workloads. We would
>> need to measure how the patch affects the performance in various workloads.
>> It seems too late to do that at this stage of v13. Thought?
>
> I agree that it's too late for v13.

Thanks for the comment!

So I pushed the patch and changed default of track_planning to off.

Regards,

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


Reply | Threaded
Open this post in threaded view
|

Re: track_planning causing performance regression

Peter Geoghegan-4
On Thu, Jul 2, 2020 at 7:39 PM Fujii Masao <[hidden email]> wrote:
> So I pushed the patch and changed default of track_planning to off.

I have closed out the open item I created for this.

Thanks!
--
Peter Geoghegan


Reply | Threaded
Open this post in threaded view
|

Re: track_planning causing performance regression

Fujii Masao-4


On 2020/07/03 11:43, Peter Geoghegan wrote:
> On Thu, Jul 2, 2020 at 7:39 PM Fujii Masao <[hidden email]> wrote:
>> So I pushed the patch and changed default of track_planning to off.
>
> I have closed out the open item I created for this.

Thanks!!

I added the patch that replaces spinlock with lwlock in pgss, into CF-2020-09.

Regards,

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


Reply | Threaded
Open this post in threaded view
|

Re: track_planning causing performance regression

Pavel Stehule
In reply to this post by Fujii Masao-4
Hi

pá 3. 7. 2020 v 4:39 odesílatel Fujii Masao <[hidden email]> napsal:


On 2020/07/01 7:37, Peter Geoghegan wrote:
> On Tue, Jun 30, 2020 at 6:40 AM Fujii Masao <[hidden email]> wrote:
>> Ants and Andres suggested to replace the spinlock used in pgss_store() with
>> LWLock. I agreed with them and posted the POC patch doing that. But I think
>> the patch is an item for v14. The patch may address the reported performance
>> issue, but may cause other performance issues in other workloads. We would
>> need to measure how the patch affects the performance in various workloads.
>> It seems too late to do that at this stage of v13. Thought?
>
> I agree that it's too late for v13.

Thanks for the comment!

So I pushed the patch and changed default of track_planning to off.

Maybe there can be documented so enabling this option can have a negative impact on performance.

Regards

Pavel

Regards,

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


Reply | Threaded
Open this post in threaded view
|

Re: track_planning causing performance regression

Fujii Masao-4


On 2020/07/03 13:05, Pavel Stehule wrote:

> Hi
>
> pá 3. 7. 2020 v 4:39 odesílatel Fujii Masao <[hidden email] <mailto:[hidden email]>> napsal:
>
>
>
>     On 2020/07/01 7:37, Peter Geoghegan wrote:
>      > On Tue, Jun 30, 2020 at 6:40 AM Fujii Masao <[hidden email] <mailto:[hidden email]>> wrote:
>      >> Ants and Andres suggested to replace the spinlock used in pgss_store() with
>      >> LWLock. I agreed with them and posted the POC patch doing that. But I think
>      >> the patch is an item for v14. The patch may address the reported performance
>      >> issue, but may cause other performance issues in other workloads. We would
>      >> need to measure how the patch affects the performance in various workloads.
>      >> It seems too late to do that at this stage of v13. Thought?
>      >
>      > I agree that it's too late for v13.
>
>     Thanks for the comment!
>
>     So I pushed the patch and changed default of track_planning to off.
>
>
> Maybe there can be documented so enabling this option can have a negative impact on performance.

Yes. What about adding either of the followings into the doc?

     Enabling this parameter may incur a noticeable performance penalty.

or

     Enabling this parameter may incur a noticeable performance penalty,
     especially when a fewer kinds of queries are executed on many
     concurrent connections.

Regards,

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


Reply | Threaded
Open this post in threaded view
|

Re: track_planning causing performance regression

Pavel Stehule


pá 3. 7. 2020 v 8:57 odesílatel Fujii Masao <[hidden email]> napsal:


On 2020/07/03 13:05, Pavel Stehule wrote:
> Hi
>
> pá 3. 7. 2020 v 4:39 odesílatel Fujii Masao <[hidden email] <mailto:[hidden email]>> napsal:
>
>
>
>     On 2020/07/01 7:37, Peter Geoghegan wrote:
>      > On Tue, Jun 30, 2020 at 6:40 AM Fujii Masao <[hidden email] <mailto:[hidden email]>> wrote:
>      >> Ants and Andres suggested to replace the spinlock used in pgss_store() with
>      >> LWLock. I agreed with them and posted the POC patch doing that. But I think
>      >> the patch is an item for v14. The patch may address the reported performance
>      >> issue, but may cause other performance issues in other workloads. We would
>      >> need to measure how the patch affects the performance in various workloads.
>      >> It seems too late to do that at this stage of v13. Thought?
>      >
>      > I agree that it's too late for v13.
>
>     Thanks for the comment!
>
>     So I pushed the patch and changed default of track_planning to off.
>
>
> Maybe there can be documented so enabling this option can have a negative impact on performance.

Yes. What about adding either of the followings into the doc?

     Enabling this parameter may incur a noticeable performance penalty.

or

     Enabling this parameter may incur a noticeable performance penalty,
     especially when a fewer kinds of queries are executed on many
     concurrent connections.

This second variant looks perfect for this case.

Thank you

Pavel




Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
Reply | Threaded
Open this post in threaded view
|

Re: track_planning causing performance regression

Fujii Masao-4


On 2020/07/03 16:02, Pavel Stehule wrote:

>
>
> pá 3. 7. 2020 v 8:57 odesílatel Fujii Masao <[hidden email] <mailto:[hidden email]>> napsal:
>
>
>
>     On 2020/07/03 13:05, Pavel Stehule wrote:
>      > Hi
>      >
>      > pá 3. 7. 2020 v 4:39 odesílatel Fujii Masao <[hidden email] <mailto:[hidden email]> <mailto:[hidden email] <mailto:[hidden email]>>> napsal:
>      >
>      >
>      >
>      >     On 2020/07/01 7:37, Peter Geoghegan wrote:
>      >      > On Tue, Jun 30, 2020 at 6:40 AM Fujii Masao <[hidden email] <mailto:[hidden email]> <mailto:[hidden email] <mailto:[hidden email]>>> wrote:
>      >      >> Ants and Andres suggested to replace the spinlock used in pgss_store() with
>      >      >> LWLock. I agreed with them and posted the POC patch doing that. But I think
>      >      >> the patch is an item for v14. The patch may address the reported performance
>      >      >> issue, but may cause other performance issues in other workloads. We would
>      >      >> need to measure how the patch affects the performance in various workloads.
>      >      >> It seems too late to do that at this stage of v13. Thought?
>      >      >
>      >      > I agree that it's too late for v13.
>      >
>      >     Thanks for the comment!
>      >
>      >     So I pushed the patch and changed default of track_planning to off.
>      >
>      >
>      > Maybe there can be documented so enabling this option can have a negative impact on performance.
>
>     Yes. What about adding either of the followings into the doc?
>
>           Enabling this parameter may incur a noticeable performance penalty.
>
>     or
>
>           Enabling this parameter may incur a noticeable performance penalty,
>           especially when a fewer kinds of queries are executed on many
>           concurrent connections.
>
>
> This second variant looks perfect for this case.
Ok, so patch attached.

Regards,

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

document_overhead_by_track_planning.patch (916 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: track_planning causing performance regression

Pavel Stehule


pá 3. 7. 2020 v 13:02 odesílatel Fujii Masao <[hidden email]> napsal:


On 2020/07/03 16:02, Pavel Stehule wrote:
>
>
> pá 3. 7. 2020 v 8:57 odesílatel Fujii Masao <[hidden email] <mailto:[hidden email]>> napsal:
>
>
>
>     On 2020/07/03 13:05, Pavel Stehule wrote:
>      > Hi
>      >
>      > pá 3. 7. 2020 v 4:39 odesílatel Fujii Masao <[hidden email] <mailto:[hidden email]> <mailto:[hidden email] <mailto:[hidden email]>>> napsal:
>      >
>      >
>      >
>      >     On 2020/07/01 7:37, Peter Geoghegan wrote:
>      >      > On Tue, Jun 30, 2020 at 6:40 AM Fujii Masao <[hidden email] <mailto:[hidden email]> <mailto:[hidden email] <mailto:[hidden email]>>> wrote:
>      >      >> Ants and Andres suggested to replace the spinlock used in pgss_store() with
>      >      >> LWLock. I agreed with them and posted the POC patch doing that. But I think
>      >      >> the patch is an item for v14. The patch may address the reported performance
>      >      >> issue, but may cause other performance issues in other workloads. We would
>      >      >> need to measure how the patch affects the performance in various workloads.
>      >      >> It seems too late to do that at this stage of v13. Thought?
>      >      >
>      >      > I agree that it's too late for v13.
>      >
>      >     Thanks for the comment!
>      >
>      >     So I pushed the patch and changed default of track_planning to off.
>      >
>      >
>      > Maybe there can be documented so enabling this option can have a negative impact on performance.
>
>     Yes. What about adding either of the followings into the doc?
>
>           Enabling this parameter may incur a noticeable performance penalty.
>
>     or
>
>           Enabling this parameter may incur a noticeable performance penalty,
>           especially when a fewer kinds of queries are executed on many
>           concurrent connections.
>
>
> This second variant looks perfect for this case.

Ok, so patch attached.

+1

Thank you

Pavel


Regards,

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