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-f9ff209ec...@iki.fi
--
Heikki Linnakangas
Neon (https://neon.tech)
