Horton

Re: AIO v2.2

Heikki Linnakangas <hlinnaka@iki.fi>

View thread
From: Heikki Linnakangas <hlinnaka@iki.fi>
To: Andres Freund <andres@anarazel.de>, pgsql-hackers@postgresql.org
Date: 2025-01-07T15:09:58Z
Lists: pgsql-hackers

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. aio: Fix assertion, clarify README

  2. aio: Fix reference to outdated name

  3. aio: Fix possible state confusions due to interrupt processing

  4. aio: Improve debug logging around waiting for IOs

  5. aio: Fix crash potential for pg_aios views due to late state update

  6. Increase BAS_BULKREAD based on effective_io_concurrency

  7. localbuf: Add Valgrind buffer access instrumentation

  8. aio: Make AIO more compatible with valgrind

  9. aio: Avoid spurious coverity warning

  10. tests: Fix incompatibility of test_aio with *_FORCE_RELEASE

  11. tests: Cope with WARNINGs during failed CREATE DB on windows

  12. aio: Add errcontext for processing I/Os for another backend

  13. aio: Add README.md explaining higher level design

  14. aio: Minor comment improvements

  15. aio: Add test_aio module

  16. aio: Add pg_aios view

  17. docs: Add acronym and glossary entries for I/O and AIO

  18. Enable IO concurrency on all systems

  19. read_stream: Introduce and use optional batchmode support

  20. docs: Reframe track_io_timing related docs as wait time

  21. bufmgr: Use AIO in StartReadBuffers()

  22. bufmgr: Implement AIO read support

  23. aio: Add WARNING result status

  24. Let caller of PageIsVerified() control ignore_checksum_failure

  25. pgstat: Allow checksum errors to be reported in critical sections

  26. Add errhint_internal()

  27. localbuf: Track pincount in BufferDesc as well

  28. aio, bufmgr: Comment fixes/improvements

  29. Fix mis-attribution of checksum failure stats to the wrong database

  30. aio: Implement support for reads in smgr/md/fd

  31. aio: Add io_method=io_uring

  32. aio: Add liburing dependency

  33. aio: Rename pgaio_io_prep_* to pgaio_io_start_*

  34. aio: Pass result of local callbacks to ->report_return

  35. aio: Be more paranoid about interrupts

  36. Redefine max_files_per_process to control additionally opened files

  37. aio: Change prefix of PgAioResultStatus values to PGAIO_RS_

  38. bufmgr: Improve stats when a buffer is read in concurrently

  39. aio: Add io_method=worker

  40. aio: Infrastructure for io_method=worker

  41. aio: Add core asynchronous I/O infrastructure

  42. aio: Basic subsystem initialization

  43. tests: Expand temp table tests to some pin related matters

  44. localbuf: Introduce FlushLocalBuffer()

  45. localbuf: Introduce TerminateLocalBufferIO()

  46. localbuf: Fix dangerous coding pattern in GetLocalVictimBuffer()

  47. localbuf: Introduce StartLocalBufferIO()

  48. localbuf: Introduce InvalidateLocalBuffer()

  49. Allow lwlocks to be disowned

  50. Make jsonb casts to scalar types translate JSON null to SQL NULL.

  51. bufmgr/smgr: Don't cross segment boundaries in StartReadBuffers()

  52. Use aux process resource owner in walsender

  53. bufmgr: Return early in ScheduleBufferTagForWriteback() if fsync=off

Attachments

On 01/01/2025 06:03, Andres Freund wrote:
> Hi,
> 
> Attached is a new version of the AIO patchset.

I haven't gone through it all yet, but some comments below.

> The biggest changes are:
> 
> - The README has been extended with an overview of the API. I think it gives a
>    good overview of how the API fits together.  I'd be very good to get
>    feedback from folks that aren't as familiar with AIO, I can't really see
>    what's easy/hard anymore.

Thanks, the README is super helpful! I was overwhelmed by all the new 
concepts before, now it all makes much more sense.

Now that it's all laid out more clearly, I see how many different 
concepts and states there really are:

- For a single IO, there is an "IO handle", "IO references", and an "IO 
return". You first allocate an IO handle (PgAioHandle), and then you get 
a reference (PgAioHandleRef) and an "IO return" (PgAioReturn) struct for it.

- An IO handle has eight different states (PgAioHandleState).

I'm sure all those concepts exist for a reason. But still I wonder: can 
we simplify?

pgaio_io_get() and pgaio_io_release() are a bit asymmetric, I'd suggest 
pgaio_io_acquire() or similar. "get" also feels very innocent, even 
though it may wait for previous IO to finish. Especially when 
pgaio_io_get_ref() actually is innocent.

> typedef enum PgAioHandleState
> {
> 	/* not in use */
> 	AHS_IDLE = 0,
> 
> 	/* returned by pgaio_io_get() */
> 	AHS_HANDED_OUT,
> 
> 	/* pgaio_io_start_*() has been called, but IO hasn't been submitted yet */
> 	AHS_DEFINED,
> 
> 	/* subject's prepare() callback has been called */
> 	AHS_PREPARED,
> 
> 	/* IO has been submitted and is being executed */
> 	AHS_IN_FLIGHT,
> 
> 	/* IO finished, but result has not yet been processed */
> 	AHS_REAPED,
> 
> 	/* IO completed, shared completion has been called */
> 	AHS_COMPLETED_SHARED,
> 
> 	/* IO completed, local completion has been called */
> 	AHS_COMPLETED_LOCAL,
> } PgAioHandleState;

Do we need to distinguish between DEFINED and PREPARED? At quick glance, 
those states are treated the same. (The comment refers to 
pgaio_io_start_*() functions, but there's no such thing)

I didn't quite understand the point of the prepare callbacks. For 
example, when AsyncReadBuffers() calls smgrstartreadv(), the 
shared_buffer_readv_prepare() callback will be called. Why doesn't 
AsyncReadBuffers() do the "prepare" work itself directly; why does it 
need to be in a callback? I assume it's somehow related to error 
handling, but I didn't quite get it. Perhaps an "abort" callback that'd 
be called on error, instead of a "prepare" callback, would be better?

There are some synonyms used in the code: I think "in-flight" and 
"submitted" mean the same thing. And "prepared" and "staged". I'd 
suggest picking just one term for each concept.

I didn't understand the COMPLETED_SHARED and COMPLETED_LOCAL states. 
does a single IO go through both states, or are the mutually exclusive? 
At quick glance, I don't actually see any code that would set the 
COMPLETED_LOCAL state; is it dead code?

REAPED feels like a bad name. It sounds like a later stage than 
COMPLETED, but it's actually vice versa.

I'm a little surprised that the term "IO request" isn't used anywhere. I 
have no concrete suggestion, but perhaps that would be a useful term.

> - Retries for partial IOs (i.e. short reads) are now implemented. Turned out
>    to take all of three lines and adding one missing variable initialization.

:-)

> - There's no obvious way to tell "internal" function operating on an IO handle
>    apart from functions that are expected to be called by the issuer of an IO.
> 
>    One way to deal with this would be to introduce a distinct "issuer IO
>    reference" type.  I think that might be a good idea, it would also make it
>    clearer that a good number of the functions can only be called by the
>    issuer, before the IO is submitted.
> 
>    This would also make it easier to order functions more sensibly in aio.c, as
>    all the issuer functions would be together.
> 
>    The functions on AIO handles that everyone can call already have a distinct
>    type (PgAioHandleRef vs PgAioHandle*).

Hmm, yeah I think you might be onto something here.

Could pgaio_io_get() return an PgAioHandleRef directly, so that the 
issuer would never see a raw PgAioHandle ?


Finally, attached are a couple of typos and other trivial suggestions.

-- 
Heikki Linnakangas
Neon (https://neon.tech)