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

Hi,

Attached v2.12, with the following changes:

- Pushed the max_files_per_process change

  I plan to look at what parts of Jelte's change is worth doing ontop.

  Thanks for the review Noah.


- Rebased over Thomas' commit of the remaining read stream changes

  Yay!


- Addressed Noah's review comments


- Added another test to test_aio/, to test that changing io_workers while
  running works, and that workers are restarted if terminated

  Written by Bilal


- Made InvalidateLocalBuffer wait for IO if necessary

  As reported / suggested by Noah


- Added tests for dropping tables with ongoing IO

  This failed, as Noah predicted, without the InvalidateLocalBuffer() change.


- Added a commit to explicitly hold interrupts in workers after
  pgaio_io_reopen()

  As suggested by Noah.


- Added a commit to fix a logic error around what gets passed to
  ioh->report_return - this lead to temporary buffer validation errors not
  being reported

  Discovered while extending the tests, as noted in the next point.

  I could see a few different "formulations" of this change (e.g. the
  report_return stuff could be populated by pgaio_io_call_complete_local()
  instead), but I don't think it matters much.


- Add temporary table coverage to test_aio

  This required hanged test_aio.c to cope with temporary tables as well.


- io_uring tests don't run anymore when built with EXEC_BACKEND and liburing
  enabled


- Split the read stream patch into two

  Noah, quite rightly, pointed out that it's not safe to use batching if the
  next-block callback may block (or start its own batch). The best idea seems
  to be to make users of read stream opt-in to batching.  I've done that in a
  patch that uses where it seems safe without doing extra work. See also the
  commit message.


- Added a commit to add I/O, Asynchronous I/O glossary and acronym entries


- Docs for pg_aios


- Renamed pg_aios.offset to off, to avoid use of a keyword


- Updated the io_uring wait event name while waiting for IOs to complete to
  AIO_IO_URING_COMPLETION and updated the description of AIO_IO_COMPLETION to
  "Waiting for another process to complete IO."

  I think this is a mix of different suggestions by Noah.


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.

  Wonder if it's worth adding some coverage for when checksums are disabled?
  Probably not necessary?


Greetings,

Andres Freund