On 13/11/2023 01:08, Thomas Munro wrote:
On Mon, Nov 13, 2023 at 11:16 AM Heikki Linnakangas <hlinn...@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)
