Re: Replace buffer I/O locks with condition variables (reviving an old patch)

On Tue, Mar 9, 2021 at 6:24 PM Julien Rouhaud <rjuju123@gmail.com> wrote:
> > The old I/O lock array was the only user of struct
> > LWLockMinimallyPadded, added in commit 6150a1b08a9, and it seems kinda
> > strange to leave it in the tree with no user.  Of course it's remotely
> > possible there are extensions using it (know of any?).  In the
> > attached, I've ripped that + associated commentary out, because it's
> > fun to delete dead code.  Objections?
>
> None from me.  I don't know of any extension relying on it, and neither does
> codesearch.debian.net.  I would be surprised to see any extension actually
> relying on that anyway.

Thanks for checking!

> > Since the whole reason for that out-of-line array in the first place
> > was to keep BufferDesc inside one cache line, and since it is in fact
> > possible to put a new condition variable into BufferDesc without
> > exceeding 64 bytes on a 64 bit x86 box, perhaps we should just do that
> > instead?  I haven't yet considered other architectures or potential
> > member orders.
>
> +1 for adding the cv into BufferDesc.  That brings the struct size to exactly
> 64 bytes on x86 64 bits architecture.  This won't add any extra overhead to
> LOCK_DEBUG cases, as it was already exceeding the 64B threshold, if that even
> was a concern.

I also checked that it's 64B on an Arm box.  Not sure about POWER.
But... despite the fact that it looks like a good change in isolation,
I decided to go back to the separate array in this initial commit,
because the AIO branch also wants to add a new BufferDesc member[1].
I may come back to that change, if we can make some more space (seems
entirely doable, but I'd like to look into that separately).

> > I wonder if we should try to preserve user experience a little harder,
> > for the benefit of people who have monitoring queries that look for
> > this condition.  Instead of inventing a new wait_event value, let's
> > just keep showing "BufferIO" in that column.  In other words, the
> > change is that wait_event_type changes from "LWLock" to "IPC", which
> > is a pretty good summary of this patch.  Done in the attached.  Does
> > this make sense?
>
> I think it does make sense, and it's good to preserve this value.
>
> Looking at the patch itself, I don't have much to add it all looks sensible and
> I agree with the arguments in the first mail.  All regression tests pass and
> documentation builds.

I found one more thing to tweak: a reference in the README.

> I'm marking this patch as RFC.

Thanks for the review.  And of course to Robert for writing the patch.  Pushed.

[1] https://github.com/anarazel/postgres/blob/aio/src/include/storage/buf_internals.h#L190