Re: WAL Insertion Lock Improvements

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(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:
>> On Mon, Apr 10, 2023 at 9:38 AM Michael Paquier <michael(at)paquier(dot)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