Hi, On 2023-11-10 16:26:51 +0200, Heikki Linnakangas wrote: > 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.
Yea - I wonder if we should have a version of dsm_attach() that pins at the same time. It's pretty silly to first attach (remembering the dsm) and then immediately pin (forgetting the dsm). > 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 there's a relationship from the dsa too, via on_dsm_detach(). > 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*. Yea, that doesn't seem great. It shouldn't affect the stats could though, due the dsa being pinned. > 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). Yea, that seems the right direction from here. Greetings, Andres Freund
