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

Hi,

Attached v2.11, with the following changes:


- Pushed the smgr interrupt change, as discussed on the dedicated thread


- Pushed "bufmgr: Improve stats when a buffer is read in concurrently"

  It was reviewed by Melanie and there didn't seem to be any reason to wait
  further.


- Addressed feedback from Melanie


- Addressed feedback from Noah


- Added a new commit: aio: Change prefix of PgAioResultStatus values to PGAIO_RS_

  As suggested/requested by Melanie. I think she's unfortunately right.


- Added a patch for some comment fixups for code that's either older or
  already pushed


- Added an error check for FileStartReadV() failing

  FileStartReadV() actually can fail, if the file can't be re-opened. I
  thought it'd be important for the error message to differ from the one
  that's issued for read actually failing, so I went with:

  "could not start reading blocks %u..%u in file \"%s\": %m"

  but I'm not sure how good that is.


- Added a new commit to redefine set_max_safe_fds() to not subtract
  already_open fds from max_files_per_process

  This prevents io_method=io_uring from failing when RLIMIT_NOFILE is high
  enough, but more than max_files_per_process io_uring instances need to be
  created.


- Improved error message if io_uring_queue_init() fails

  Added errhint()s for likely cases of failure.

  Added errcode().  I was tempted to use errcode_for_file_access(), but that
  doesn't support ENOSYS - perhaps I should add that instead?


- Disable io_uring method when using EXEC_BACKEND, they're not compatible

  I chose to do this with a define aio.h, but I guess we could also do it at
  configure time? That seems more complicated though - how would we even know
  that EXEC_BACKEND is used on non-windows?

  Not sure yet how to best disable testing io_uring in this case. We can't
  just query EXEC_BACKEND from pg_config.h unfortunately.  I guess making the
  initdb not fail and checking the error log would work, but that doesn't work
  nicely with Cluster.pm.


- Changed test_aio injection short-read injection point to zero out the rest
  of the IO, otherwise some tests fail to fail even if a bug in retries of
  partial reads is introduced


- Improved method_io_uring.c includes a bit (no pgstat.h)



Questions:


- We only "look" at BM_IO_ERROR for writes, isn't that somewhat weird?

  See AbortBufferIO(Buffer buffer)

  It doesn't really matter for the patchset, but it just strikes me as an oddity.



Greetings,

Andres Freund