Re: AIO v2.5

Hi,

On 2025-03-24 19:20:37 -0700, Noah Misch wrote:
> On Thu, Mar 20, 2025 at 09:58:37PM -0400, Andres Freund wrote:
> > static void
> > TerminateBufferIO(BufferDesc *buf, bool clear_dirty, uint32 set_flag_bits,
> > - bool forget_owner)
> > + bool forget_owner, bool syncio)
> > ...
> Looking at the callers:
>
> ZeroAndLockBuffer[1083] TerminateBufferIO(bufHdr, false, BM_VALID, true, true);
> ExtendBufferedRelShared[2869] TerminateBufferIO(buf_hdr, false, BM_VALID, true, true);
> FlushBuffer[4827] TerminateBufferIO(buf, true, 0, true, true);
> AbortBufferIO[6637] TerminateBufferIO(buf_hdr, false, BM_IO_ERROR, false, true);
> buffer_readv_complete_one[7279] TerminateBufferIO(buf_hdr, false, set_flag_bits, false, false);
> buffer_writev_complete_one[7427] TerminateBufferIO(buf_hdr, clear_dirty, set_flag_bits, false, false);
>
> I think we can improve on the "syncio" arg name. The first two aren't doing
> IO, and AbortBufferIO() may be cleaning up what would have been an AIO if it
> hadn't failed early. Perhaps name the arg "release_aio" and pass
> release_aio=true instead of syncio=false (release_aio = !syncio).

Yes, I think that makes sense. Will do that tomorrow.

> > +static pg_attribute_always_inline PgAioResult
> > +buffer_readv_complete_one(uint8 buf_off, Buffer buffer, uint8 flags,
> > + bool failed, bool is_temp)
> > +{
> ...
> > + if ((flags & READ_BUFFERS_ZERO_ON_ERROR) || zero_damaged_pages)
> > + {
> > + ereport(WARNING,
> > + (errcode(ERRCODE_DATA_CORRUPTED),
> > + errmsg("invalid page in block %u of relation %s; zeroing out page",
>
> My earlier review requested s/LOG/WARNING/, but I wasn't thinking about this
> in full depth. In the !is_temp case, this runs in a complete_shared
> callback. A process unrelated to the original IO may run this callback.
> That's unfortunate in two ways. First, that other process's client gets an
> unexpected WARNING. The process getting the WARNING may not even have
> zero_damaged_pages enabled. Second, the client of the process that staged
> the IO gets no message.

Ah, right. That could be why I had flipped it. If so, shame on me for not
adding a comment...

> AIO ERROR-level messages handle this optimally. We emit a LOG-level message
> in the process that runs the complete_shared callback, and we arrange for the
> ERROR-level message in the stager. That would be ideal here: LOG in the
> complete_shared runner, WARNING in the stager.

We could obviously downgrade (crossgrade? A LOG is more severe than a LOG in
some ways, but not others) the message when run in a different backend fairly
easily. Still emitting a WARNING in the stager however is a bit more tricky.

Before thinking more deeply about how we could emit WARNING in the stage:

Is it actually sane to use WARNING here? At least for ZERO_ON_ERROR that could
trigger a rather massive flood of messages to the client in a *normal*
situation. I'm thinking of something like an insert extending a relation some
time after an immediate restart and encountering a lot of FSM corruption (due
to its non-crash-safe-ness) during the search for free space and the
subsequent FSM vacuum. It might be ok to LOG that, but sending a lot of
WARNINGs to the client seems not quite right.

If we want to implement it, I think we could introduce PGAIO_RS_WARN, which
then could tell the stager to issue the WARNING. It would add a bit of
distributed cost, both to callbacks and users of AIO, but it might not be too
bad.

> One could simplify things by forcing io_method=sync under ZERO_ON_ERROR ||
> zero_damaged_pages, perhaps as a short-term approach.

Yea, that could work. Perhaps even just for zero_damaged_pages, after
changing it so that ZERO_ON_ERROR always just LOGs.

Hm, it seems somewhat nasty to have rather different performance
characteristics when forced to use zero_damaged_pages to recover from a
problem. Imagine an instance that's configured to use DIO and then needs to
use zero_damaged_pages to recove from corruption...

/me adds writing a test for both ZERO_ON_ERROR and zero_damaged_pages to the
TODO.

Greetings,

Andres Freund