At Thu, 27 Oct 2022 09:59:14 -0700, Andres Freund <andres@anarazel.de> wrote in
> But I think we can solve that fairly reasonably nonetheless. We can change
> PGPROC->lwWaiting to not just be a boolean, but have three states:
> 0: not waiting
> 1: waiting in waitlist
> 2: waiting to be woken up
>
> which we then can use in LWLockDequeueSelf() to only remove ourselves from the
> list if we're on it. As removal from that list is protected by the wait list
> lock, there's no race to worry about.
Since LWLockDequeueSelf() is defined to be called in some restricted
situation, there's no chance that the proc to remove is in another
lock waiters list at the time the function is called. So it seems to
work well. It is simple and requires no additional memory or cycles...
No. It enlarges PRPC by 8 bytes, but changing lwWaiting to int8/uint8
keeps the size as it is. (Rocky8/x86-64)
It just shaves off looping cycles. So +1 for what the patch does.
> client patched HEAD
> 1 60109 60174
> 2 112694 116169
> 4 214287 208119
> 8 377459 373685
> 16 524132 515247
> 32 565772 554726
> 64 587716 497508
> 128 581297 415097
> 256 550296 334923
> 512 486207 243679
> 768 449673 192959
> 1024 410836 157734
> 2048 326224 82904
> 4096 250252 32007
>
> Not perfect with the patch, but not awful either.
Fairly good? Agreed. The performance peak is improved by 6% and
shifted to larger number of clients (32->128).
> I suspect this issue might actually explain quite a few odd performance
> behaviours we've seen at the larger end in the past. I think it has gotten a
> bit worse with the conversion of lwlock.c to proclists (I see lots of
> expensive multiplications to deal with sizeof(PGPROC)), but otherwise likely
> exists at least as far back as ab5194e6f61, in 9.5.
>
> I guess there's an argument for considering this a bug that we should
> backpatch a fix for? But given the vintage, probably not? The only thing that
> gives me pause is that this is quite hard to pinpoint as happening.
I don't think this is a bug but I think it might be back-patchable
since it doesn't change memory footprint (if adjusted), causes no
additional cost or interfarce breakage, thus it might be ok to
backpatch. Since this "bug" has the nature of positive-feedback so
reducing the coefficient is benetifical than the direct cause of the
change.
> I've attached my quick-and-dirty patch. Obviously it'd need a few defines etc,
> but I wanted to get this out to discuss before spending further time.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
