Horton

Re: AIO v2.5

Noah Misch <noah@leadboat.com>

View thread
From: Noah Misch <noah@leadboat.com>
To: Andres Freund <andres@anarazel.de>
Cc: Antonin Houska <ah@cybertec.at>, 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>
Date: 2025-03-24T00:29:39Z
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

commit 247ce06b wrote:
> +			pgaio_io_reopen(ioh);
> +
> +			/*
> +			 * To be able to exercise the reopen-fails path, allow injection
> +			 * points to trigger a failure at this point.
> +			 */
> +			pgaio_io_call_inj(ioh, "AIO_WORKER_AFTER_REOPEN");
> +
> +			error_errno = 0;
> +			error_ioh = NULL;
> +
> +			/*
> +			 * We don't expect this to ever fail with ERROR or FATAL, no need
> +			 * to keep error_ioh set to the IO.
> +			 * pgaio_io_perform_synchronously() contains a critical section to
> +			 * ensure we don't accidentally fail.
> +			 */
> +			pgaio_io_perform_synchronously(ioh);

A CHECK_FOR_INTERRUPTS() could close() the FD that pgaio_io_reopen() callee
smgr_aio_reopen() stores.  Hence, I think smgrfd() should assert that
interrupts are held instead of doing its own HOLD_INTERRUPTS(), and a
HOLD_INTERRUPTS() should surround the above region of code.  It's likely hard
to reproduce a problem, because pgaio_io_call_inj() does nothing in many
builds, and pgaio_io_perform_synchronously() starts by entering a critical
section.

On Thu, Mar 20, 2025 at 09:58:37PM -0400, Andres Freund wrote:
> Attached v2.11

> Subject: [PATCH v2.11 06/27] aio: Implement support for reads in smgr/md/fd

> +int
> +FileStartReadV(PgAioHandle *ioh, File file,
> +			   int iovcnt, off_t offset,
> +			   uint32 wait_event_info)
> +{
> +	int			returnCode;
> +	Vfd		   *vfdP;
> +
> +	Assert(FileIsValid(file));
> +
> +	DO_DB(elog(LOG, "FileStartReadV: %d (%s) " INT64_FORMAT " %d",
> +			   file, VfdCache[file].fileName,
> +			   (int64) offset,
> +			   iovcnt));
> +
> +	returnCode = FileAccess(file);
> +	if (returnCode < 0)
> +		return returnCode;
> +
> +	vfdP = &VfdCache[file];
> +
> +	pgaio_io_prep_readv(ioh, vfdP->fd, iovcnt, offset);

FileStartReadV() and pgaio_io_prep_readv() advance the IO to PGAIO_HS_STAGED
w/ batch mode, PGAIO_HS_SUBMITTED w/o batch mode.  I didn't expect that from
functions so named.  The "start" verb sounds to me like unconditional
PGAIO_HS_SUBMITTED, and the "prep" verb sounds like PGAIO_HS_DEFINED.  I like
the "stage" verb, because it matches PGAIO_HS_STAGED, and the comment at
PGAIO_HS_STAGED succinctly covers what to expect.  Hence, I recommend names
FileStageReadV, pgaio_io_stage_readv, mdstagereadv, and smgrstageread.  How do
you see it?

> +/*
> + * AIO error reporting callback for mdstartreadv().
> + *
> + * Errors are encoded as follows:
> + * - PgAioResult.error_data != 0 encodes IO that failed with errno != 0

I recommend replacing "errno != 0" with either "that errno" or "errno ==
error_data".


> Subject: [PATCH v2.11 07/27] aio: Add README.md explaining higher level design

Ready for commit apart from some trivia:

> +if (ioret.result.status == PGAIO_RS_ERROR)
> +    pgaio_result_report(aio_ret.result, &aio_ret.target_data, ERROR);

I think ioret and aio_ret are supposed to be the same object.  If that's
right, change one of the names.  Likewise elsewhere in this file.

> +The central API piece for postgres' AIO abstraction are AIO handles. To
> +execute an IO one first has to acquire an IO handle (`pgaio_io_acquire()`) and
> +then "defined", i.e. associate an IO operation with the handle.

s/"defined"/"define" it/ or similar

> +The "solution" to this the ability to associate multiple completion callbacks

s/this the/this is the/


> Subject: [PATCH v2.11 08/27] localbuf: Track pincount in BufferDesc as well

> @@ -5350,6 +5350,18 @@ ConditionalLockBufferForCleanup(Buffer buffer)
>  		Assert(refcount > 0);
>  		if (refcount != 1)
>  			return false;
> +
> +		/*
> +		 * Check that the AIO subsystem doesn't have a pin. Likely not
> +		 * possible today, but better safe than sorry.
> +		 */
> +		bufHdr = GetLocalBufferDescriptor(-buffer - 1);
> +		buf_state = pg_atomic_read_u32(&bufHdr->state);
> +		refcount = BUF_STATE_GET_REFCOUNT(buf_state);
> +		Assert(refcount > 0);
> +		if (refcount != 1)
> +			return false;
> +

LockBufferForCleanup() should get code like this
ConditionalLockBufferForCleanup() code, either now or when "not possible
today" ends.  Currently, it just assumes all local buffers are
cleanup-lockable:

	/* Nobody else to wait for */
	if (BufferIsLocal(buffer))
		return;

> @@ -570,7 +577,13 @@ InvalidateLocalBuffer(BufferDesc *bufHdr, bool check_unreferenced)
>  
>  	buf_state = pg_atomic_read_u32(&bufHdr->state);
>  
> -	if (check_unreferenced && LocalRefCount[bufid] != 0)
> +	/*
> +	 * We need to test not just LocalRefCount[bufid] but also the BufferDesc
> +	 * itself, as the latter is used to represent a pin by the AIO subsystem.
> +	 * This can happen if AIO is initiated and then the query errors out.
> +	 */
> +	if (check_unreferenced &&
> +		(LocalRefCount[bufid] != 0 || BUF_STATE_GET_REFCOUNT(buf_state) != 0))
>  		elog(ERROR, "block %u of %s is still referenced (local %u)",

I didn't write a test to prove it, but I'm suspecting we'll reach the above
ERROR with this sequence:

  CREATE TEMP TABLE foo ...;
  [some command that starts reading a block of foo into local buffers, then ERROR with IO ongoing]
  DROP TABLE foo;

DropRelationAllLocalBuffers() calls InvalidateLocalBuffer(bufHdr, true).  I
think we'd need to do like pgaio_shutdown() and finish all IOs (or all IOs for
the particular rel) before InvalidateLocalBuffer().  Or use something like the
logic near elog(ERROR, "buffer is pinned in InvalidateBuffer") in
corresponding bufmgr code.  I think that bufmgr ERROR is unreachable, since
only a private refcnt triggers that bufmgr ERROR.  Is there something
preventing the localbuf error from being a problem?  (This wouldn't require
changes to the current patch; responsibility would fall in a bufmgr AIO
patch.)


> Subject: [PATCH v2.11 09/27] bufmgr: Implement AIO read support

(Still reviewing this and later patches, but incidental observations follow.)

> +buffer_readv_complete_one(uint8 buf_off, Buffer buffer, uint8 flags,
> +						  bool failed, bool is_temp)
> +{
...
> +	PgAioResult result;
...
> +	result.status = PGAIO_RS_OK;
...
> +	return result;

gcc 14.2.0 -Werror gives me:

  bufmgr.c:7297:16: error: ‘result’ may be used uninitialized [-Werror=maybe-uninitialized]

Zeroing the unset fields silenced it:

--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -7221,3 +7221,3 @@ buffer_readv_complete_one(uint8 buf_off, Buffer buffer, uint8 flags,
 	char	   *bufdata = BufferGetBlock(buffer);
-	PgAioResult result;
+	PgAioResult result = { .status = PGAIO_RS_OK };
 	uint32		set_flag_bits;
@@ -7238,4 +7238,2 @@ buffer_readv_complete_one(uint8 buf_off, Buffer buffer, uint8 flags,
 
-	result.status = PGAIO_RS_OK;
-
 	/* check for garbage data */


> Subject: [PATCH v2.11 13/27] aio: Basic read_stream adjustments for real AIO

> @@ -416,6 +418,13 @@ read_stream_start_pending_read(ReadStream *stream)
>  static void
>  read_stream_look_ahead(ReadStream *stream)
>  {
> +	/*
> +	 * Allow amortizing the cost of submitting IO over multiple IOs. This
> +	 * requires that we don't do any operations that could lead to a deadlock
> +	 * with staged-but-unsubmitted IO.
> +	 */
> +	pgaio_enter_batchmode();

We call read_stream_get_block() while in batchmode, so the stream callback
needs to be ready for that.  A complicated case is
collect_corrupt_items_read_stream_next_block(), which may do its own buffer
I/O to read in a vmbuffer for VM_ALL_FROZEN().  That's feeling to me like a
recipe for corner cases reaching ERROR "starting batch while batch already in
progress".  Are there mitigating factors?


> Subject: [PATCH v2.11 17/27] aio: Add test_aio module

> +	# verify that page verification errors are detected even as part of a
> +	# shortened multi-block read (tbl_corr, block 1 is tbl_corred)

Is "tbl_corred" a typo of something?

> --- /dev/null
> +++ b/src/test/modules/test_aio/test_aio.c
> @@ -0,0 +1,657 @@
> +/*-------------------------------------------------------------------------
> + *
> + * delay_execution.c
> + *		Test module to allow delay between parsing and execution of a query.
> + *
> + * The delay is implemented by taking and immediately releasing a specified
> + * advisory lock.  If another process has previously taken that lock, the
> + * current process will be blocked until the lock is released; otherwise,
> + * there's no effect.  This allows an isolationtester script to reliably
> + * test behaviors where some specified action happens in another backend
> + * between parsing and execution of any desired query.
> + *
> + * Copyright (c) 2020-2025, PostgreSQL Global Development Group
> + *
> + * IDENTIFICATION
> + *	  src/test/modules/delay_execution/delay_execution.c

Header comment is surviving from copy-paste of delay_execution.c.

> +	 * Tor tests we don't want the resowner release preventing us from

s/Tor/For/