Horton

Re: AIO v2.5

Andres Freund <andres@anarazel.de>

View thread
From: Andres Freund <andres@anarazel.de>
To: Noah Misch <noah@leadboat.com>
Cc: pgsql-hackers@postgresql.org, Thomas Munro <thomas.munro@gmail.com>, Heikki Linnakangas <hlinnaka@iki.fi>, Robert Haas <robertmhaas@gmail.com>, Jakub Wartak <jakub.wartak@enterprisedb.com>, Jelte Fennema-Nio <postgres@jeltef.nl>, Antonin Houska <ah@cybertec.at>
Date: 2025-03-27T01:07:40Z
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

Hi,

Attached v2.13, with the following changes:
Changes:

- Pushed a fair number of commits

  A lot of thanks goes to Noah's detailed reviews!


- As Noah pointed out, the zero_damaged_pages warning could be emitted in an
  io worker or another backend, but omitted in the backend that started the IO

  To address that:

  1) I added a new commit "aio: Add WARNING result status"
     (itself trivial)

  2) I changed buffer_readv_complete() to encode the warning/error in a more
     detailed way than before (was_zeroed, first_invalid_off, count_invalid)

     As part of that I put the encoding/decoding into a static inline

  3) Tracking the number of invalid buffers was awkward with
     buffer_readv_complete_one() returning a PgAioResult. Now it just
     reports whether it found an invalid page with an out argument.

  4) As discussed there now is a different error messages for the case of
     multiple invalid pages

     The code is a bit awkward to avoid code duplication, curious whether
     that's seen as acceptable?  I could just duplicate the entire ereport()
     instead.

  5) The WARNING in the callback is now a LOG, as it will be sent to the
     client as a WARNING explicitly when the IO's results are processed

     I actually chose LOG_SERVER_ONLY - that seemed slightly better than just
     LOG? But not at all sure.

     There's a comment explaining this now too.


  Noah, I think this set of changes would benefit from another round of
  review. I left these changes in "squash-later: " commits, to make it easier
  to see / think about.


- Added a comment about the pgaio_result_report() in md_readv_complete(). I
  changed it to LOG_SERVER_ONLY as well , but I'm not at all sure about that.


- Previously the buffer completion callback checked zero_damaged_pages - but
  that's not right, the GUC hopefully is only set on a per-session basis

  I solved that by having AsyncReadBuffers() add ZERO_ON_ERROR to the flags if
  zero_damaged_pages is configured.

  Also added a comment explaining that we probably should eventually use a
  separate flag, so we can adjust the errcode etc differently.


- Explicit test for zero_damaged_pages and ZERO_ON_ERROR

  As part of that I made read_rel_block_ll() support reading multiple
  blocks. That makes it a lot easier to verify that we handle cases like a
  4-block read where 2,3 are invalid correctly.


- I removed the code that "localbuf: Track pincount in BufferDesc as well"
  added to ConditionalLockBufferForCleanup() and IsBufferCleanupOK() as discussed

  Right now the situations that the code was worried don't exist yet, as we
  only support reads.

  I added a comment about not needing to worry about that yet to "bufmgr:
  Implement AIO read support". And then changed that comment to a FIXME in the
  write patches.


- Squashed Thomas' change to make io_concurrency=0 really not use AIO


- Lots of other review comments by Noah addressed


- Merged typo fixes by Thom Brown



TODO:


- There are more tests in test_aio that should be expanded to run for temp
  tables as well, not just normal tables


- Add an explicit test for the checksum verification in the completion callback

  There is an existing test for testing an invalid page due to page header
  verification in test_aio, but not for checksum failures.

  I think it's indirectly covered (e.g. in amcheck), but seems better to test
  it explicitly.


- Add error context callbacks for io worker and "foreign" IO completion

Greetings,

Andres Freund