On Tue, Mar 9, 2021 at 6:24 PM Julien Rouhaud <rjuju...@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
