Re: AIO v2.5 - Mailing list pgsql-hackers

Hi,

On 2025-03-11 11:31:18 -0400, Melanie Plageman wrote:
> On Mon, Mar 10, 2025 at 2:23 PM Andres Freund <andres@anarazel.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