Date: Mon, 24 Mar 2025 19:20:37 -0700
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>
Subject: Re: AIO v2.5
Message-ID: <20250325022037.91.nmisch@google.com>
References: <20250311194108.c5.nmisch@google.com>
 <5dzyoduxlvfg55oqtjyjehez5uoq6hnwgzor4kkybkfdgkj7ag@rbi4gsmzaczk>
 <20250312035743.f5.nmisch@google.com>
 <w6uiicyou7hzq47mbyejubtcyb2rngkkf45fk4q7inue5kfbeo@bbfad3qyubvs>
 <4b3f32ug3cayekysqlgspz2qjmeb7lca3gvazayglxr2m3d4dv@il33accgsji7>
 <17906.1741863183@localhost>
 <3yxd5r23zly5bytvgyktbxtxq2r3gbpi7xd4dugevh3h4w4q6c@lu6oatjjpltz>
 <oav52ytakcvfnk52w5o5wvxdjavxth2tgptxt25gnfmh2pdcpk@omp3xukdu5if>
 <zp7obmaqdd6b5u2vhpw27hovpjirtqvjakkbkaz4vju7tohyul@4kfobe7ipgxm>
 <6ak556uyqiptdwjaci4kbi5eykwkmzqgkbtkyaosjnopjhncrc@2v4ac2jwyz22>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <6ak556uyqiptdwjaci4kbi5eykwkmzqgkbtkyaosjnopjhncrc@2v4ac2jwyz22>
User-Agent: Mutt/2.2.12 (2023-09-09)
Archived-At: 
 <https://www.postgresql.org/message-id/20250325022037.91.nmisch%40google.com>
Precedence: bulk

On Thu, Mar 20, 2025 at 09:58:37PM -0400, Andres Freund wrote:
> Subject: [PATCH v2.11 09/27] bufmgr: Implement AIO read support

[I checked that v2.12 doesn't invalidate these review comments, but I didn't
technically rebase the review onto v2.12's line numbers.]

>  static void
>  TerminateBufferIO(BufferDesc *buf, bool clear_dirty, uint32 set_flag_bits,
> -				  bool forget_owner)
> +				  bool forget_owner, bool syncio)
>  {
>  	uint32		buf_state;
>  
> @@ -5586,6 +5636,14 @@ TerminateBufferIO(BufferDesc *buf, bool clear_dirty, uint32 set_flag_bits,
>  	if (clear_dirty && !(buf_state & BM_JUST_DIRTIED))
>  		buf_state &= ~(BM_DIRTY | BM_CHECKPOINT_NEEDED);
>  
> +	if (!syncio)
> +	{
> +		/* release ownership by the AIO subsystem */
> +		Assert(BUF_STATE_GET_REFCOUNT(buf_state) > 0);
> +		buf_state -= BUF_REFCOUNT_ONE;
> +		pgaio_wref_clear(&buf->io_wref);
> +	}

Looking at the callers:

ZeroAndLockBuffer[1083]        TerminateBufferIO(bufHdr, false, BM_VALID, true, true);
ExtendBufferedRelShared[2869]  TerminateBufferIO(buf_hdr, false, BM_VALID, true, true);
FlushBuffer[4827]              TerminateBufferIO(buf, true, 0, true, true);
AbortBufferIO[6637]            TerminateBufferIO(buf_hdr, false, BM_IO_ERROR, false, true);
buffer_readv_complete_one[7279] TerminateBufferIO(buf_hdr, false, set_flag_bits, false, false);
buffer_writev_complete_one[7427] TerminateBufferIO(buf_hdr, clear_dirty, set_flag_bits, false, false);

I think we can improve on the "syncio" arg name.  The first two aren't doing
IO, and AbortBufferIO() may be cleaning up what would have been an AIO if it
hadn't failed early.  Perhaps name the arg "release_aio" and pass
release_aio=true instead of syncio=false (release_aio = !syncio).

> +		 * about which buffers are target by IO can be hard to debug, making

s/target/targeted/

> +static pg_attribute_always_inline PgAioResult
> +buffer_readv_complete_one(uint8 buf_off, Buffer buffer, uint8 flags,
> +						  bool failed, bool is_temp)
> +{
...
> +		if ((flags & READ_BUFFERS_ZERO_ON_ERROR) || zero_damaged_pages)
> +		{
> +			ereport(WARNING,
> +					(errcode(ERRCODE_DATA_CORRUPTED),
> +					 errmsg("invalid page in block %u of relation %s; zeroing out page",

My earlier review requested s/LOG/WARNING/, but I wasn't thinking about this
in full depth.  In the !is_temp case, this runs in a complete_shared callback.
A process unrelated to the original IO may run this callback.  That's
unfortunate in two ways.  First, that other process's client gets an
unexpected WARNING.  The process getting the WARNING may not even have
zero_damaged_pages enabled.  Second, the client of the process that staged the
IO gets no message.

AIO ERROR-level messages handle this optimally.  We emit a LOG-level message
in the process that runs the complete_shared callback, and we arrange for the
ERROR-level message in the stager.  That would be ideal here: LOG in the
complete_shared runner, WARNING in the stager.

One could simplify things by forcing io_method=sync under ZERO_ON_ERROR ||
zero_damaged_pages, perhaps as a short-term approach.

Thoughts?