Re: WAL Insertion Lock Improvements - Mailing list pgsql-hackers

From Michael Paquier
Subject Re: WAL Insertion Lock Improvements
Date
Msg-id ZFnVvJbcBsniEm6m@paquier.xyz
Whole thread Raw
In response to Re: WAL Insertion Lock Improvements  (Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>)
Responses Re: WAL Insertion Lock Improvements
List pgsql-hackers
On Mon, May 08, 2023 at 08:18:04PM +0530, Bharath Rupireddy wrote:
> On Mon, May 8, 2023 at 5:57 PM Bharath Rupireddy
> <bharath.rupireddyforpostgres@gmail.com> wrote:
>> On Mon, Apr 10, 2023 at 9:38 AM Michael Paquier <michael@paquier.xyz> wrote:
>>> The sensitive change is in LWLockUpdateVar().  I am not completely
>>> sure to understand this removal, though.  Does that influence the case
>>> where there are waiters?
>>
>> I'll send about this in a follow-up email to not overload this
>> response with too much data.
>
> It helps the case when there are no waiters. IOW, it updates value
> without waitlist lock when there are no waiters, so no extra waitlist
> lock acquisition/release just to update the value. In turn, it helps
> the other backend wanting to flush the WAL looking for the new updated
> value of insertingAt in WaitXLogInsertionsToFinish(), now the flushing
> backend can get the new value faster.

Sure, which is what the memory barrier given by exchange_u64
guarantees.  My thoughts on this one is that I am not completely sure
to understand that we won't miss any waiters that should have been
awaken.

> The fastpath exit in LWLockUpdateVar() doesn't seem to influence the
> results much, see below results. However, it avoids waitlist lock
> acquisition when there are no waiters.
>
> test-case 1: -T5, WAL ~16 bytes
> clients    HEAD    PATCHED with fastpath    PATCHED no fast path
> 64    50135    45528    46653
> 128    94903    89791    89103
> 256    82289    152915    152835
> 512    62498    138838    142084
> 768    57083    125074    126768
> 1024    51308    113593    115930
> 2048    41084    88764    85110
> 4096    19939    42257    43917

Considering that there could be a few percents of noise mixed into
that, that's not really surprising as the workload is highly
concurrent on inserts so the fast path won't really shine :)

Should we split this patch into two parts, as they aim at tackling two
different cases then?  One for LWLockConflictsWithVar() and
LWLockReleaseClearVar() which are the straight-forward pieces
(using one pg_atomic_write_u64() in LWLockUpdateVar instead), then
a second for LWLockUpdateVar()?

Also, the fast path treatment in LWLockUpdateVar() may show some
better benefits when there are really few backends and a bunch of very
little records?  Still, even that sounds a bit limited..

> Out of 3 functions that got rid of waitlist lock
> LWLockConflictsWithVar/LWLockWaitForVar, LWLockUpdateVar,
> LWLockReleaseClearVar, perf reports tell that the biggest gain (for
> the use-cases that I've tried) is for
> LWLockConflictsWithVar/LWLockWaitForVar:
>
> test-case 6: -T5, WAL 4096 bytes
> HEAD:
> +   29.66%     0.07%  postgres  [.] LWLockWaitForVar
> +   20.94%     0.08%  postgres  [.] LWLockConflictsWithVar
>      0.19%     0.03%  postgres  [.] LWLockUpdateVar
>
> PATCHED:
> +    3.95%     0.08%  postgres  [.] LWLockWaitForVar
>      0.19%     0.03%  postgres  [.] LWLockConflictsWithVar
> +    1.73%     0.00%  postgres  [.] LWLockReleaseClearVar

Indeed.
--
Michael

Attachment

pgsql-hackers by date:

Previous
From: "Hayato Kuroda (Fujitsu)"
Date:
Subject: RE: Subscription statistics are not dropped at DROP SUBSCRIPTION in some cases
Next
From: Amit Kapila
Date:
Subject: Re: Add two missing tests in 035_standby_logical_decoding.pl