Re: ResourceOwner refactoring

Hi,

On 2023-11-07 13:28:28 +0200, Heikki Linnakangas wrote:
> I feel pretty good about this overall. Barring objections or new cfbot
> failures, I will commit this in the next few days.

I am working on rebasing the AIO patch over this. I think I found a crash
that's unrelated to AIO.

#4 0x0000000000ea7631 in ExceptionalCondition (conditionName=0x4ba6f1 "owner->narr == 0",
fileName=0x4ba628 "../../../../../home/andres/src/postgresql/src/backend/utils/resowner/resowner.c", lineNumber=354)
at ../../../../../home/andres/src/postgresql/src/backend/utils/error/assert.c:66
#5 0x0000000000ef13b5 in ResourceOwnerReleaseAll (owner=0x3367d48, phase=RESOURCE_RELEASE_BEFORE_LOCKS, printLeakWarnings=false)
at ../../../../../home/andres/src/postgresql/src/backend/utils/resowner/resowner.c:354
#6 0x0000000000ef1d9c in ResourceOwnerReleaseInternal (owner=0x3367d48, phase=RESOURCE_RELEASE_BEFORE_LOCKS, isCommit=false, isTopLevel=true)
at ../../../../../home/andres/src/postgresql/src/backend/utils/resowner/resowner.c:717
#7 0x0000000000ef1c89 in ResourceOwnerRelease (owner=0x3367d48, phase=RESOURCE_RELEASE_BEFORE_LOCKS, isCommit=false, isTopLevel=true)
at ../../../../../home/andres/src/postgresql/src/backend/utils/resowner/resowner.c:644
#8 0x00000000008c1f87 in AbortTransaction () at ../../../../../home/andres/src/postgresql/src/backend/access/transam/xact.c:2851
#9 0x00000000008c4ae0 in AbortOutOfAnyTransaction () at ../../../../../home/andres/src/postgresql/src/backend/access/transam/xact.c:4761
#10 0x0000000000ec1502 in ShutdownPostgres (code=1, arg=0) at ../../../../../home/andres/src/postgresql/src/backend/utils/init/postinit.c:1357
#11 0x0000000000c942e5 in shmem_exit (code=1) at ../../../../../home/andres/src/postgresql/src/backend/storage/ipc/ipc.c:243

I think the problem is that ResourceOwnerSort() looks at owner->nhash == 0,
whereas ResourceOwnerReleaseAll() looks at !owner->hash. Therefore it's
possible to reach ResourceOwnerReleaseAll() with owner->hash != NULL while
also having owner->narr > 0.

I think both checks for !owner->hash in ResourceOwnerReleaseAll() need to
instead check owner->nhash == 0.

I'm somewhat surprised that this is only hit with the AIO branch. I guess one
needs to be somewhat unlucky to end up with a hashtable but 0 elements in it
at the time of resowner release. But still somewhat surprising.

Seperately, I see that resowner_private.h still exists in the repo, I think
that should be deleted, as many of the functions don't exist anymore?

Lastly, I think it'd be good to assert that we're not releasing the resowner
in ResourceOwnerRememberLock(). It's currently not strictly required, but that
seems like it's just leaking an implementation detail out?

Greetings,

Andres Freund