Re: ResourceOwner refactoring

Hi,

On 2023-11-17 12:44:41 -0800, Andres Freund wrote:
> 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.

Oops - I hadn't rebased far enough at this point... That was already fixed in
8f4a1ab471e.

Seems like it'd be good to cover that path in one of the tests?

- Andres