Date: Tue, 25 Mar 2025 08:58:08 -0700
From: Noah Misch <noah@leadboat.com>
To: Andres Freund <andres@anarazel.de>
Cc: Thomas Munro <thomas.munro@gmail.com>, Antonin Houska <ah@cybertec.at>,
	pgsql-hackers@postgresql.org, 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: <20250325155808.f7.nmisch@google.com>
References: <zp7obmaqdd6b5u2vhpw27hovpjirtqvjakkbkaz4vju7tohyul@4kfobe7ipgxm>
 <6ak556uyqiptdwjaci4kbi5eykwkmzqgkbtkyaosjnopjhncrc@2v4ac2jwyz22>
 <20250324002939.5c.nmisch@google.com>
 <q2jujruxxksaz7k5m7wckkfz6qvmhiqy2wjwqcgqi3qaz25scb@n7hcd6qf47je>
 <havtpn6xuwpybrveymzvvym4cqi37jxhcsfququk4lioyasvsq@eitpmihs2akh>
 <CA+hUKGJPTf3pJXVLyQ_aEFFTRGbaJgANG2wpa94L0htEdDC4AQ@mail.gmail.com>
 <20250325004537.17.nmisch@google.com>
 <dbeeaize47y7esifdrinpa2l7cqqb67k72exvuf3appyxywjnc@7bt76mozhcy2>
 <20250325133321.54.nmisch@google.com>
 <nk5gyjsny6w222roebnj2f7oid3stqrbh4xj5ah3xkvz52borc@hedincoyhg43>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <nk5gyjsny6w222roebnj2f7oid3stqrbh4xj5ah3xkvz52borc@hedincoyhg43>
User-Agent: Mutt/2.2.12 (2023-09-09)
Archived-At: 
 <https://www.postgresql.org/message-id/20250325155808.f7.nmisch%40google.com>
Precedence: bulk

On Tue, Mar 25, 2025 at 11:26:14AM -0400, Andres Freund wrote:
> On 2025-03-25 06:33:21 -0700, Noah Misch wrote:
> > On Mon, Mar 24, 2025 at 10:30:27PM -0400, Andres Freund wrote:
> > > On 2025-03-24 17:45:37 -0700, Noah Misch wrote:
> > > > (We may be due for a test mode that does smgrreleaseall() at every
> > > > CHECK_FOR_INTERRUPTS()?)
> > >
> > > I suspect we are. I'm a bit afraid of even trying...
> > >
> > > ...
> > >
> > > It's extremely slow - but at least the main regression as well as the aio tests pass!
> >
> > One less thing!
> 
> Unfortunately I'm now doubting the thoroughness of my check - while I made
> every CFI() execute smgrreleaseall(), I didn't trigger CFI() in cases where we
> trigger it conditionally. E.g. elog(DEBUGN, ...) only executes a CFI if
> log_min_messages <= DEBUGN...
> 
> I'll try that in a bit.

While having nagging thoughts that we might be releasing FDs before io_uring
gets them into kernel custody, I tried this hack to maximize FD turnover:

static void
ReleaseLruFiles(void)
{
#if 0
	while (nfile + numAllocatedDescs + numExternalFDs >= max_safe_fds)
	{
		if (!ReleaseLruFile())
			break;
	}
#else
	while (ReleaseLruFile())
		;
#endif
}

"make check" with default settings (io_method=worker) passes, but
io_method=io_uring in the TEMP_CONFIG file got different diffs in each of two
runs.  s/#if 0/#if 1/ (restore normal FD turnover) removes the failures.
Here's the richer of the two diffs:

diff -U3 src/test/regress/expected/sanity_check.out src/test/regress/results/sanity_check.out
--- src/test/regress/expected/sanity_check.out  2024-10-24 12:43:25.741817594 -0700
+++ src/test/regress/results/sanity_check.out   2025-03-25 08:27:44.875151566 -0700
@@ -1,4 +1,7 @@
 VACUUM;
+ERROR:  index "pg_enum_oid_index" contains corrupted page at block 2
+HINT:  Please REINDEX it.
+CONTEXT:  while vacuuming index "pg_enum_oid_index" of relation "pg_catalog.pg_enum"
 --
 -- Sanity check: every system catalog that has OIDs should have
 -- a unique index on OID.  This ensures that the OIDs will be unique,
diff -U3 src/test/regress/expected/oidjoins.out src/test/regress/results/oidjoins.out
--- src/test/regress/expected/oidjoins.out      2023-07-06 19:58:07.686364439 -0700
+++ src/test/regress/results/oidjoins.out       2025-03-25 08:28:02.584335458 -0700
@@ -233,6 +233,8 @@
 NOTICE:  checking pg_policy {polrelid} => pg_class {oid}
 NOTICE:  checking pg_policy {polroles} => pg_authid {oid}
 NOTICE:  checking pg_default_acl {defaclrole} => pg_authid {oid}
+WARNING:  FK VIOLATION IN pg_default_acl({defaclrole}): ("(1,5)",0)
+WARNING:  FK VIOLATION IN pg_default_acl({defaclrole}): ("(1,7)",402654464)
 NOTICE:  checking pg_default_acl {defaclnamespace} => pg_namespace {oid}
 NOTICE:  checking pg_init_privs {classoid} => pg_class {oid}
 NOTICE:  checking pg_seclabel {classoid} => pg_class {oid}

> > > Because the end state varies, depending on the number of previously staged
> > > IOs, the IO method and whether batchmode is enabled, I think it's better if
> > > the "function naming pattern" (i.e. FileStartReadv, smgrstartreadv etc) is
> > > *not* aligned with an internal state name.  It will just mislead readers to
> > > think that there's a deterministic mapping when that does not exist.
> >
> > That's fair.  Could we provide the mapping in a comment, something like the
> > following?
> 
> Yes!
> 
> I wonder if it should also be duplicated or referenced elsewhere, although I
> am not sure where precisely.

I considered the README.md also, but adding that wasn't an obvious win.

> > --- a/src/include/storage/aio_internal.h
> > +++ b/src/include/storage/aio_internal.h
> > @@ -34,5 +34,10 @@
> >   * linearly through all states.
> >   *
> > - * State changes should all go through pgaio_io_update_state().
> > + * State changes should all go through pgaio_io_update_state().  Its callers
> > + * use these naming conventions:
> > + *
> > + * - A "start" function (e.g. FileStartReadV()) moves an IO from
> > + *   PGAIO_HS_HANDED_OUT to at least PGAIO_HS_STAGED and at most
> > + *   PGAIO_HS_COMPLETED_LOCAL.
> >   */
> >  typedef enum PgAioHandleState
> 
> One detail I'm not sure about: The above change is correct, but perhaps a bit
> misleading, because we can actually go "back" to IDLE. Not sure how to best
> phrase that though.

Not sure either.  Maybe the above could change to "to PGAIO_HS_STAGED or any
subsequent state" and the comment at PGAIO_HS_STAGED could say like "Once in
this state, concurrent activity could move the IO all the way to
PGAIO_HS_COMPLETED_LOCAL and recycle it back to IDLE."

> > > That's not an excuse for pgaio_io_prep* though, that's a pointlessly different
> > > naming that I just stopped seeing.
> 
> I assume you're on board with renaming _io_prep* to _io_start_*?

Yes.

> > > I'll try to think more about this, perhaps I can make myself see your POV
> > > more.

> > CheckBufferIsPinnedOnce(Buffer buffer)
> > {
> > 	if (BufferIsLocal(buffer))
> > 	{
> > 		if (LocalRefCount[-buffer - 1] != 1)
> > 			elog(ERROR, "incorrect local pin count: %d",
> > 				 LocalRefCount[-buffer - 1]);
> > 	}
> > 	else
> > 	{
> > 		if (GetPrivateRefCount(buffer) != 1)
> > 			elog(ERROR, "incorrect local pin count: %d",
> > 				 GetPrivateRefCount(buffer));
> > 	}
> > }
> 
> Pretty random orthogonal thought, that I was reminded of by the above code
> snippet:
> 
> It sure seems we should at some point get rid of LocalRefCount[] and just use
> the GetPrivateRefCount() infrastructure for both shared and local buffers.  I
> don't think the GetPrivateRefCount() infrastructure cares about
> local/non-local, leaving a few asserts aside.  If we do that, and start to use
> BM_IO_IN_PROGRESS, combined with ResourceOwnerRememberBufferIO(), the set of
> differences between shared and local buffers would be a lot smaller.

That sounds promising.

> > > > Like the comment, I expect it's academic today.  I expect it will stay
> > > > academic.  Anything that does a cleanup will start by reading the buffer,
> > > > which will resolve any refcnt the AIO subsystems holds for a read.  If there's
> > > > an AIO write, the LockBuffer(buffer, BUFFER_LOCK_EXCLUSIVE) will block on
> > > > that.  How about just removing the ConditionalLockBufferForCleanup() changes
> > > > or replacing them with a comment (like the present paragraph)?
> > >
> > > I think we'll need an expanded version of what I suggest once we have writes -
> > > but as you say, it shouldn't matter as long as we only have reads. So I think
> > > moving the relevant changes, with adjusted caveats, to the bufmgr: write
> > > change makes sense.
> >
> > Moving those changes works for me.  I'm not currently seeing the need under
> > writes, but that may get clearer upon reaching those patches.
> 
> FWIW, I don't think it's currently worth looking at the write side in detail,

Got it.  (I meant I didn't see a first-principles need, not that I had deduced
lack of need from a specific writes implementation.)

> > > Do you think it's worth mentioning the above workaround? I'm mildly inclined
> > > that not.
> >
> > Perhaps not in that detail, but perhaps we can rephrase (b) to not imply
> > exit+reenter is banned.  Maybe "(b) start another batch (without first exiting
> > one)".  It's also fine as-is, though.
> 
> I updated it to:
> 
>  * b) start another batch (without first exiting batchmode and re-entering
>  *    before returning)

That's good.