Re: ResourceOwner refactoring

Thanks for the testing again!

On 10/11/2023 11:00, Alexander Lakhin wrote:
> I could see two failure modes:
> 2023-11-10 08:42:28.870 UTC [1163274] ERROR:  ResourceOwnerEnlarge called after release started
> 2023-11-10 08:42:28.870 UTC [1163274] STATEMENT:  drop table t;
> 2023-11-10 08:42:28.870 UTC [1163274] WARNING:  AbortTransaction while in COMMIT state
> 2023-11-10 08:42:28.870 UTC [1163274] PANIC:  cannot abort transaction 906, it was already committed
>
> 2023-11-10 08:43:27.897 UTC [1164148] ERROR:  ResourceOwnerEnlarge called after release started
> 2023-11-10 08:43:27.897 UTC [1164148] STATEMENT:  DROP DATABASE db69;
> 2023-11-10 08:43:27.897 UTC [1164148] WARNING:  AbortTransaction while in COMMIT state
> 2023-11-10 08:43:27.897 UTC [1164148] PANIC:  cannot abort transaction 1043, it was already committed
>
> The stack trace for the second ERROR (ResourceOwnerEnlarge called ...) is:
> ...
> #6  0x0000558af5b2f35c in ResourceOwnerEnlarge (owner=0x558af716f3c8) at resowner.c:455
> #7  0x0000558af5888f18 in dsm_create_descriptor () at dsm.c:1207
> #8  0x0000558af5889205 in dsm_attach (h=3172038420) at dsm.c:697
> #9  0x0000558af5b1ebed in get_segment_by_index (area=0x558af711da18, index=2) at dsa.c:1764
> #10 0x0000558af5b1ea4b in dsa_get_address (area=0x558af711da18, dp=2199023329568) at dsa.c:970
> #11 0x0000558af5669366 in dshash_seq_next (status=0x7ffdd5912fd0) at dshash.c:687
> #12 0x0000558af5901998 in pgstat_drop_database_and_contents (dboid=16444) at pgstat_shmem.c:830
> #13 0x0000558af59016f0 in pgstat_drop_entry (kind=PGSTAT_KIND_DATABASE, dboid=16444, objoid=0) at pgstat_shmem.c:888
> #14 0x0000558af59044eb in AtEOXact_PgStat_DroppedStats (xact_state=0x558af7111ee0, isCommit=true) at pgstat_xact.c:88
> #15 0x0000558af59043c7 in AtEOXact_PgStat (isCommit=true, parallel=false) at pgstat_xact.c:55
> #16 0x0000558af53c782e in CommitTransaction () at xact.c:2371
> #17 0x0000558af53c709e in CommitTransactionCommand () at xact.c:306
> ...

The quick, straightforward fix is to move the "CurrentResourceOwner =
NULL" line earlier in CommitTransaction, per attached
0003-Clear-CurrentResourceOwner-earlier-in-CommitTransact.patch. You're
not allowed to use the resource owner after you start to release it; it
was a bit iffy even before the ResourceOwner rewrite but now it's
explicitly forbidden. By clearing CurrentResourceOwner as soon as we
start releasing it, we can prevent any accidental use.

When CurrentResourceOwner == NULL, dsm_attach() returns a handle that's
not associated with any ResourceOwner. That's appropriate for the pgstat
case. The DSA is "pinned", so the handle is forgotten from the
ResourceOwner right after calling dsm_attach() anyway.

Looking closer at dsa.c, I think this is a wider problem though. The
comments don't make it very clear how it's supposed to interact with
ResourceOwners. There's just this brief comment in dsa_pin_mapping():

> * By default, areas are owned by the current resource owner, which means they
> * are detached automatically when that scope ends.

The dsa_area struct isn't directly owned by any ResourceOwner though.
The DSM segments created by dsa_create() or dsa_attach() are.

But the functions dsa_allocate() and dsa_get_address() can create or
attach more DSM segments to the area, and they will be owned by the by
the current resource owner *at the time of the call*. So if you call
dsa_get_address() while in a different resource owner, things get very
confusing. The attached new test module demonstrates that
(0001-Add-test_dsa-module.patch), here's a shortened version:

a = dsa_create(tranche_id);

/* Switch to new resource owner */
oldowner = CurrentResourceOwner;
childowner = ResourceOwnerCreate(oldowner, "temp owner");
CurrentResourceOwner = childowner;

/* make a bunch of allocations */
for (int i = 0; i < 10000; i++)
p[i] = dsa_allocate(a, 1000);

/* Release the child resource owner */
CurrentResourceOwner = oldowner;
ResourceOwnerRelease(childowner,
RESOURCE_RELEASE_BEFORE_LOCKS,
true, false);
ResourceOwnerRelease(childowner,
RESOURCE_RELEASE_LOCKS,
true, false);
ResourceOwnerRelease(childowner,
RESOURCE_RELEASE_AFTER_LOCKS,
true, false);
ResourceOwnerDelete(childowner);

dsa_detach(a);

This first prints warnings on The ResourceOwnerRelease() calls:

2023-11-10 13:57:21.475 EET [745346] WARNING: resource was not closed:
dynamic shared memory segment 2395813396
2023-11-10 13:57:21.475 EET [745346] WARNING: resource was not closed:
dynamic shared memory segment 3922992700
2023-11-10 13:57:21.475 EET [745346] WARNING: resource was not closed:
dynamic shared memory segment 1155452762
2023-11-10 13:57:21.475 EET [745346] WARNING: resource was not closed:
dynamic shared memory segment 4045183168
2023-11-10 13:57:21.476 EET [745346] WARNING: resource was not closed:
dynamic shared memory segment 1529990480

And a segfault at the dsm_detach() call:

2023-11-10 13:57:21.480 EET [745246] LOG: server process (PID 745346)
was terminated by signal 11: Segmentation fault

#0 0x000055a5148f64ee in slist_pop_head_node (head=0x55a516d06bf8) at
../../../../src/include/lib/ilist.h:1034
#1 0x000055a5148f5eba in dsm_detach (seg=0x55a516d06bc0) at dsm.c:822
#2 0x000055a514b86db0 in dsa_detach (area=0x55a516d64dc8) at dsa.c:1939
#3 0x00007fd5dcaee5e0 in test_dsa_resowners (fcinfo=0x55a516d56a28) at
test_dsa.c:112

I think that is surprising behavior from the DSA facility. When you make
allocations with dsa_allocate() or just call dsa_get_address() on an
existing dsa_pointer, you wouldn't expect the current resource owner to
matter. I think dsa_create/attach() should store the current resource
owner in the dsa_area, for use in subsequent operations on the DSA, per
attached patch (0002-Fix-dsa.c-with-different-resource-owners.patch).

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