Re: ResourceOwner refactoring
Thomas Munro <thomas.munro@gmail.com>
From: Thomas Munro <thomas.munro@gmail.com>
To: Heikki Linnakangas <hlinnaka@iki.fi>
Cc: Alexander Lakhin <exclusion@gmail.com>,
Robert Haas <robertmhaas@gmail.com>, Aleksander Alekseev <aleksander@timescale.com>, "pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>,
Julien Rouhaud <rjuju123@gmail.com>, Peter Eisentraut <peter@eisentraut.org>, Andres Freund <andres@anarazel.de>
Date: 2023-11-12T23:08:57Z
Lists: pgsql-hackers
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.
> > 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().
I don't love the way this stuff is not very explicit, and if we're
going to keep doing this 'dynamic scope' stuff then I wish we could
find a scoping notation that looks more like the stuff one sees in
other languages that say something like
"with-resource-owner(area->resowner) { block of code }".
Commits
-
Make RelationFlushRelation() work without ResourceOwner during abort
- e6cd85772647 17.0 landed
-
Fix bug in bulk extending temp relation after failure
- d212957254de 17.0 landed
-
Add missing PGDLLIMPORT markings
- c6b86eaa55ff 17.0 landed
-
Add test_dsa module.
- 325f54033e59 17.0 landed
-
Clear CurrentResourceOwner earlier in CommitTransaction.
- c21e6e2fd48c 17.0 landed
-
Fix dsa.c with different resource owners.
- a8b330ffb6f7 17.0 landed
-
Fix bug in the new ResourceOwner implementation.
- 8f4a1ab471e6 17.0 landed
-
Change pgcrypto to use the new ResourceOwner mechanism.
- cd694f60dc97 17.0 landed
-
Use a faster hash function in resource owners.
- 954e43564d99 17.0 landed
-
Make ResourceOwners more easily extensible.
- b8bff07daa85 17.0 landed
-
Move a few ResourceOwnerEnlarge() calls for safety and clarity.
- b70c2143bbbe 17.0 landed