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

On Fri, Mar 5, 2021 at 12:12 PM Thomas Munro <thomas(dot)munro(at)gmail(dot)com> wrote:
> On Fri, Feb 26, 2021 at 7:08 PM Thomas Munro <thomas(dot)munro(at)gmail(dot)com> wrote:
> > Back in 2016, Robert Haas proposed to replace I/O locks with condition
> > variables[1]. Condition variables went in and have found lots of
> > uses, but this patch to replace a bunch of LWLocks and some busy
> > looping did not. Since then, it has been tested quite a lot as part
> > of the AIO project[2], which currently depends on it. That's why I'm
> > interested in following up now. I asked Robert if he planned to
> > re-propose it and he said I should go for it, so... here I go.
>
> I removed a redundant (Size) cast, fixed the wait event name and
> category (WAIT_EVENT_BUFFILE_XXX is for buffile.c stuff, not bufmgr.c
> stuff, and this is really an IPC wait, not an IO wait despite the
> name), updated documentation and pgindented.

More review and some proposed changes:

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?

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. It's also possible that some other project already had
designs on those BufferDesc bytes. This drops quite a few lines from
the tree, including the comment about how nice it'd be to be able to
put the lock in BufferDesc.

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?

Please see attached, which gets us to: 8 files changed, 30
insertions(+), 113 deletions(-)

PS: An idea I thought about while studying this patch is that we
should be able to make signaling an empty condition variable
free/cheap (no spinlock acquisition or other extra memory
barrier-containing operation); I'll write about that separately.