Re: AIO v2.0

On 01/09/2024 09:27, Andres Freund wrote:
> The main reason I had previously implemented WAL AIO etc was to know the
> design implications - but now that they're somewhat understood, I'm planning
> to keep the patchset much smaller, with the goal of making it upstreamable.

+1 on that approach.

> To solve the issue with an unbounded number of AIO references there are few
> changes compared to the prior approach:
>
> 1) Only one AIO handle can be "handed out" to a backend, without being
> defined. Previously the process of getting an AIO handle wasn't super
> lightweight, which made it appealing to cache AIO handles - which was one
> part of the problem for running out of AIO handles.
>
> 2) Nothing in a backend can force a "defined" AIO handle (i.e. one that is a
> valid operation) to stay around, it's always possible to execute the AIO
> operation and then reuse the handle. This provides a forward guarantee, by
> ensuring that completing AIOs can free up handles (previously they couldn't
> be reused until the backend local reference was released).
>
> 3) Callbacks on AIOs are not allowed to error out anymore, unless it's ok to
> take the server down.
>
> 4) Obviously some code needs to know the result of AIO operation and be able
> to error out. To allow for that the issuer of an AIO can provide a pointer
> to local memory that'll receive the result of an AIO, including details
> about what kind of errors occurred (possible errors are e.g. a read failing
> or a buffer's checksum validation failing).
>
>
> In the next few days I'll add a bunch more documentation and comments as well
> as some better perf numbers (assuming my workstation survived...).

Yeah, a high-level README would be nice. Without that, it's hard to
follow what "handed out" and "defined" above means for example.

A few quick comments the patches:

v2.0-0001-bufmgr-Return-early-in-ScheduleBufferTagForWrit.patch

+1, this seems ready to be committed right away.

v2.0-0002-Allow-lwlocks-to-be-unowned.patch

With LOCK_DEBUG, LWLock->owner will point to the backend that acquired
the lock, but it doesn't own it anymore. That's reasonable, but maybe
add a boolean to the LWLock to mark whether the lock is currently owned
or not.

The LWLockReleaseOwnership() name is a bit confusing together with
LWLockReleaseUnowned() and LWLockrelease(). From the names, you might
think that they all release the lock, but LWLockReleaseOwnership() just
disassociates it from the current process. Rename it to LWLockDisown()
perhaps.

v2.0-0003-Use-aux-process-resource-owner-in-walsender.patch

+1. The old comment "We don't currently need any ResourceOwner in a
walsender process" was a bit misleading, because the walsender did
create the short-lived "base backup" resource owner, so it's nice to get
that fixed.

v2.0-0008-aio-Skeleton-IO-worker-infrastructure.patch

My refactoring around postmaster.c child process handling will conflict
with this [1]. Not in any fundamental way, but can I ask you to review
those patch, please? After those patches, AIO workers should also have
PMChild slots (formerly known as Backend structs).

[1]
https://www.postgresql.org/message-id/a102f15f-eac4-4ff2-af02-f9ff209ec66f@iki.fi

--
Heikki Linnakangas
Neon (https://neon.tech)