Re: AIO v2.5

Hi,

On 2025-03-11 11:31:18 -0400, Melanie Plageman wrote:
> On Mon, Mar 10, 2025 at 2:23 PM Andres Freund <andres(at)anarazel(dot)de> wrote:
> >
> > - 0016 to 0020 - cleanups for temp buffers code - I just wrote these to clean
> > up the code before making larger changes, needs review
>
> This is a review of 0016-0020
>
> 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...

> 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...

> * the case, write it out before reusing it!
> */
> - if (buf_state & BM_DIRTY)
> + if (pg_atomic_read_u32(&bufHdr->state) & BM_DIRTY)
> {
> + uint32 buf_state = pg_atomic_read_u32(&bufHdr->state);
>
> I don't love that you fetch in the if statement and inside the if
> statement. You wouldn't normally do this, so it sticks out. I get that
> you want to avoid having the problem this commit fixes again, but
> maybe it is worth just fetching the buf_state above the if statement
> and adding a comment that it could have changed so you must do that.

It's seems way too easy to introduce new similar breakages if the scope of
buf_state is that wide - I yesterday did waste 90min because I did in another
similar place. The narrower scopes make that much less likely to be a problem.

> Anyway, I think your future patches make the local buf_state variable
> in this function obsolete, so perhaps it doesn't matter.

Leaving the defensive-programming aspect aside, it does seem like a better
intermediary state to me to have the local vars than to have to change more
lines when introducing FlushLocalBuffer() etc.

> Not related to this patch, but while reading this code, I noticed that
> this line of code is really weird:
> LocalBufHdrGetBlock(bufHdr) = GetLocalBufferStorage();
> I actually don't understand what it is doing ... setting the result of
> the macro to the result of GetLocalBufferStorage()? I haven't seen
> anything like that before.

Yes, that's what it's doing. LocalBufferBlockPointers() evaluates to a value
that can be used as an lvalue in an assignment.

Not exactly pretty...

> Otherwise, this patch LGTM.
>
> 0017:
>
> +++ 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?

> + * InvalidateLocalBuffer -- mark a local buffer invalid.
> + *
> + * If check_unreferenced is true, error out if the buffer is still
> + * used. Passing false is appropriate when redesignating the buffer instead
> + * dropping it.
> + *
> + * See also InvalidateBuffer().
> + */
> +static void
> +InvalidateLocalBuffer(BufferDesc *bufHdr, bool check_unreferenced)
> +{
>
> I was on the fence about the language "buffer is still used", since
> this is about the ref count and not the usage count. If this is the
> language used elsewhere perhaps it is fine.

I'll change it to "still pinned"
I guess I can make it "still pinned".

> I also was not sure what redesignate means here. If you mean to use
> this function in the future in other contexts than eviction and
> dropping buffers, fine. But otherwise, maybe just use a more obvious
> word (like eviction).

I was trying to reference changing the identity of the buffer as part of
buffer replacement, where we keep a pin to the buffer. Compared to the use of
InvalidateLocalBuffer() in DropRelationAllLocalBuffers() /
DropRelationLocalBuffers().

/*
* InvalidateLocalBuffer -- mark a local buffer invalid.
*
* If check_unreferenced is true, error out if the buffer is still
* pinned. Passing false is appropriate when calling InvalidateLocalBuffer()
* as part of changing the identity of a buffer, instead of just dropping the
* buffer.
*
* See also InvalidateBuffer().
*/

> 0018:
>
> Compiler now warns that buf_state is unused in GetLocalVictimBuffer().

Oops. Missed that because it was then removed in a later commit...

> @@ -4564,8 +4548,7 @@ FlushRelationBuffers(Relation rel)
> IOCONTEXT_NORMAL, IOOP_WRITE,
> io_start, 1, BLCKSZ);
>
> - buf_state &= ~(BM_DIRTY | BM_JUST_DIRTIED);
> - pg_atomic_unlocked_write_u32(&bufHdr->state, buf_state);
> + TerminateLocalBufferIO(bufHdr, true, 0);
>
> FlushRelationBuffers() used to clear BM_JUST_DIRTIED, which it seems
> like wouldn't have been applicable to local buffers before, but,
> actually with async IO could perhaps happen in the future? Anyway,
> TerminateLocalBuffers() doesn't clear that flag, so you should call
> that out if it was intentional.

I think it'd be good to start using BM_JUST_DIRTIED, even if just to make the
code between local and shared buffers more similar. But that that's better
done separately.

I don't know why FlushRelationBuffers cleared it, it's neer set at the moment.

I'll add a note to the commit message.

> @@ -5652,8 +5635,11 @@ TerminateBufferIO(BufferDesc *buf, bool
> clear_dirty, uint32 set_flag_bits,
> + buf_state &= ~BM_IO_IN_PROGRESS;
> + buf_state &= ~BM_IO_ERROR;
> - buf_state &= ~(BM_IO_IN_PROGRESS | BM_IO_ERROR);
>
> Is it worth mentioning in the commit message that you made a cosmetic
> change to TerminateBufferIO()?

Doesn't really seem worth calling out, but if you think it should, I will.

> 0020:
> This commit message is probably tooo thin. I think you need to at
> least say something about this being used by AIO in the future. Out of
> context of this patch set, it will be confusing.

Yep.

> +/*
> + * Like StartBufferIO, but for local buffers
> + */
> +bool
> +StartLocalBufferIO(BufferDesc *bufHdr, bool forInput, bool nowait)
> +{
>
> I think you could use a comment about why nowait might be useful for
> local buffers in the future. It wouldn't make sense with synchronous
> I/O, so it feels a bit weird without any comment.

Hm, fair point. Another approach would be to defer adding the argument to a
later patch, it doesn't need to be added here.

> + 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()...

> @@ -1450,13 +1450,11 @@ static inline bool
> WaitReadBuffersCanStartIO(Buffer buffer, bool nowait)
> {
> if (BufferIsLocal(buffer))
> else
> - return StartBufferIO(GetBufferDescriptor(buffer - 1), true, nowait);
> + return StartBufferIO(GetBufferDescriptor(buffer - 1),
> + true, nowait);
>
> I'm not sure it is worth the diff in non-local buffer case to reflow
> this. It is already confusing enough in this patch that you are adding
> some code that is mostly unneeded.

Heh, you're right. I had to add a line break in the StartLocalBufferIO() and
it looked wrong to have the two lines formatted differently :)

Thanks for the review!

Greetings,

Andres Freund