Re: AIO v2.5

On Tue, Mar 11, 2025 at 1:56 PM Andres Freund <andres(at)anarazel(dot)de> wrote:
>
> Hi,
>
> On 2025-03-11 11:31:18 -0400, Melanie Plageman wrote:
> > Commit messages for 0017-0020 are thin. I assume you will beef them up
> > a bit before committing.
>
> Yea. I wanted to get some feedback on whether these refactorings are a good
> idea or not...

I'd say yes, they seem like a good idea.

> > Really, though, those matter much less than 0016 which is an actual bug (or
> > pre-bug) fix. I called out the ones where I think you should really consider
> > adding more detail to the commit message.
> >
> > 0016:
>
> Do you think we should backpatch that change? It's not really an active bug in
> 16+, but it's also not quite right. The other changes surely shouldn't be
> backpatched...

I don't feel strongly about it. PinLocalBuffer() is passed with
adjust_usagecount false and we have loads of other places where things
would just not work if we changed the boolean flag passed in to a
function called by it (bgwriter and SyncOneBuffer() with
skip_recently_used comes to mind).

On the other hand it's a straightforward fix that only needs to be
backpatched a couple versions, so it definitely doesn't hurt.

> > +++ b/src/backend/storage/buffer/localbuf.c
> > @@ -56,6 +56,7 @@ static int NLocalPinnedBuffers = 0;
> > static Buffer GetLocalVictimBuffer(void);
> > +static void InvalidateLocalBuffer(BufferDesc *bufHdr, bool check_unreferenced);
> >
> > Technically this line is too long
>
> Oh, do I love our line length limits. But, um, is it actually too long? It's
> 78 chars, which is exactly our limit, I think?

Teccchnically it's 79, which is why it showed up for me with this
handy line from the committing wiki page

git diff origin/master -- src/backend/storage/buffer/localbuf.c | grep
-E '^(\+|diff)' | sed 's/^+//' | expand -t4 | awk "length > 78 ||
/^diff/"

But anyway, it doesn't really matter. I only mentioned it because I
noticed it visually looked long.

> > + if (forInput ? (buf_state & BM_VALID) : !(buf_state & BM_DIRTY))
> > + {
> > + /* someone else already did the I/O */
> > + UnlockBufHdr(bufHdr, buf_state);
> > + return false;
> > + }
> >
> > UnlockBufHdr() explicitly says it should not be called for local
> > buffers. I know that code is unreachable right now, but it doesn't
> > feel quite right. I'm not sure what the architecture of AIO local
> > buffers will be like, but if other processes can't access these
> > buffers, I don't know why you would need BM_LOCKED. And if you will, I
> > think you need to edit the UnlockBufHdr() comment.
>
> You are right, this is a bug in my change. I started with a copy of
> StartBufferIO() and whittled it down insufficiently. Thanks for catching that!
>
> Wonder if we should add an assert against this to UnlockBufHdr()...

Yea, I think that makes sense.

- Melanie