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-04-01T15:55:20Z
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

Hi,

On 2025-04-01 08:11:59 -0700, Noah Misch wrote:
> On Mon, Mar 31, 2025 at 08:41:39PM -0400, Andres Freund wrote:
> > updated version
>
> All non-write patches (1-7) are ready for commit, though I have some cosmetic
> recommendations below.  I've marked the commitfest entry Ready for Committer.

Thanks!

I haven't yet pushed the changes, but will work on that in the afternoon.

I plan to afterwards close the CF entry and will eventually create a new one
for write support, although probably only rebasing onto
https://postgr.es/m/stj36ea6yyhoxtqkhpieia2z4krnam7qyetc57rfezgk4zgapf%40gcnactj4z56m
and addressing some of the locking issues.

WRT the locking issues, I've been wondering whether we could make
LWLockWaitForVar() work that purpose, but I doubt it's the right approach.
Probably better to get rid of the LWLock*Var functions and go for the approach
I had in v1, namely a version of LWLockAcquire() with a callback that gets
called between LWLockQueueSelf() and PGSemaphoreLock(), which can cause the
lock acquisition to abort.


> This comment is a copy of the previous test's comment.  While the comment is
> not false, consider changing it to:
>
> 		# Check one read reporting multiple invalid blocks.

> > +		# Then test zeroing vio zero_damaged_pages
>
> s/vio/via/
>

These make sense.


> > +# Verify checksum handling when creating database from an invalid database.
> > +# This also serves as a minimal check that cross-database IO is handled
> > +# reasonably.
>
> To me, "invalid database" is a term of art from the message "cannot connect to
> invalid database".  Hence, I would change "invalid database" to "database w/
> invalid block" or similar, here and below.  (Alternatively, just delete "from
> an invalid database".  It's clear from the context.)

Yea, I agree, this is easy to misunderstand when stepping back. I went for "with
an invalid block".


> > +	if (corrupt_checksum)
> > +	{
> > +		bool		successfully_corrupted = 0;
> > +
> > +		/*
> > +		 * Any single modification of the checksum could just end up being
> > +		 * valid again. To be sure
> > +		 */
>
> Unfinished sentence.

Oops. See below.


> That said, I'm not following why we'd need this loop.  If this test code
> were changing the input to the checksum, it's true that an input bit flip
> might reach the same pd_checksum.  The test case is changing pd_checksum,
> not the input bits.

We might be changing the input, due to the zero/corrupt_header options. Or we
might be called on a page that is *already* corrupted. I did encounter that
situation once while writing tests, where the tests only passed if I made the
+ 1 a + 2. Which was, uh, rather confusing and left me feel like I was cursed
that day.


> I don't see how changing pd_checksum could leave the
> page still valid.  There's only one valid pd_checksum value for a given
> input page.

I updated the comment to:
		/*
		 * Any single modification of the checksum could just end up being
		 * valid again, due to e.g. corrupt_header changing the data in a way
		 * that'd result in the "corrupted" checksum, or the checksum already
		 * being invalid. Retry in that, unlikely, case.
		 */


> > +			/*
> > +			 * The underlying IO actually completed OK, and thus the "invalid"
> > +			 * portion of the IOV actually contains valid data. That can hide
> > +			 * a lot of problems, e.g. if we were to wrongly mark a buffer,
> > +			 * that wasn't read according to the shortened-read, IO as valid,
> > +			 * the contents would look valid and we might miss a bug.
>
> Minimally s/read, IO/read IO,/ but I'd edit a bit further:
>
> 			 * a lot of problems, e.g. if we were to wrongly mark-valid a
> 			 * buffer that wasn't read according to the shortened-read IO, the
> 			 * contents would look valid and we might miss a bug.

Adopted.


> > Subject: [PATCH v2.15 05/18] md: Add comment & assert to buffer-zeroing path
> >  in md[start]readv()
>
> > The zero_damaged_pages path is incomplete, as as missing segments are not
>
> s/as as/as/
>
> > For now, put an Assert(false) comments documenting this choice into mdreadv()
>
> s/comments/and comments/
>
> > +				 * For PG 18, we are putting an Assert(false) in into
> > +				 * mdreadv() (triggering failures in assertion-enabled builds,
>
> s/in into/in/

> > Subject: [PATCH v2.15 06/18] aio: comment polishing
>
> > + * - Partial reads need to be handle by the caller re-issuing IO for the
> > + *   unread blocks
>
> s/handle/handled/

All adopted.  I'm sorry that you had to see so much of tiredness-enhanced
dyslexia :(.

Greetings,

Andres Freund