Re: ResourceOwner refactoring

On 13/11/2023 01:08, Thomas Munro wrote:
> On Mon, Nov 13, 2023 at 11:16 AM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
>> On 11/11/2023 14:00, Alexander Lakhin wrote:
>>> 10.11.2023 17:26, Heikki Linnakangas wrote:
>>>> 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).
> 
> Yeah, I agree that it is surprising and dangerous that the DSM
> segments can have different owners if you're not careful, and it's not
> clear that you have to be.  Interesting that no one ever reported
> breakage in parallel query due to this thinko, presumably because the
> current resource owner always turns out to be either the same one or
> something with longer life.

Yep. I ran check-world with an extra assertion that 
"CurrentResourceOwner == area->resowner || area->resowner == NULL" to 
verify that. No failures.

>>> With the patch 0002 applied, I'm observing another anomaly:
>>
>> Ok, so the ownership of a dsa was still muddled :-(. Attached is a new
>> version that goes a little further. It replaces the whole
>> 'mapping_pinned' flag in dsa_area with the 'resowner'. When a mapping is
>> pinned, it means that 'resowner == NULL'. This is now similar to how the
>> resowner field and pinning works in dsm.c.
> 
> This patch makes sense to me.
> 
> It might be tempting to delete the dsa_pin_mapping() interface
> completely and say that if CurrentResourceOwner == NULL, then it's
> effectively (what we used to call) pinned, but I think it's useful for
> exception-safe construction of multiple objects to be able to start
> out with a resource owner and then later 'commit' by clearing it.  As
> seen in GetSessionDsmHandle().

Yeah that's useful. I don't like the "pinned mapping" term here. It's 
confusing because we also have dsa_pin(), which means something 
different: the area will continue to exist even after all backends have 
detached from it. I think we should rename 'dsa_pin_mapping()' to 
'dsa_forget_resowner()' or something like that.

I pushed these fixes, without that renaming. Let's do that as a separate 
patch if we like that.

I didn't backpatch this for now, because we don't seem to have a live 
bug in back branches. You could argue for backpatching because this 
could bite us in the future if we backpatch something else that uses 
dsa's, or if there are extensions using it. We can do that later after 
we've had this in 'master' for a while.

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