Hi,
On Thu, May 1, 2025 at 4:26 AM Michael Paquier <mich...@paquier.xyz> wrote:
> On Wed, Apr 30, 2025 at 06:03:49PM -0400, Robert Haas wrote:
> > Sorry to turn up late here, but I strongly disagree with the notion
> > that this is a bug in the DSM or DSA code. It seems to me that it is
> > the caller's responsibility to provide a valid resource owner, not the
> > job of the called code to ignore the resource owner when it's
> > unusable. I suspect that there are many other parts of the system that
> > rely on the ResourceOwner machinery which likewise assume that the
> > ResourceOwner that they are passed is valid.
>
> Yeah, dshash would be one, I think. It feels to me that if you want
> to enforce this kind of policy to be checked, this is something that
> should be done in the shape of one or more assertion based the state
> of the resource owner expected in these low-level paths rather than
> tweaking the DSA and DSM code to do what you are expecting here, and
> only enforce such new policies on HEAD to avoid disruption with
> existing systems.
>
I believe it would be particularly useful to add an assertion in
dsm_unpin_mapping().
This function relies on CurrentResourceOwner being non-NULL and not
releasing to
successfully unpin a mapping.
>
> I'm actually rather scared of the patch, isn't there a risk of
> breaking existing patterns that worked out of the box by forcing the
> resowner to not be set?
In this context, resowner not being set means that the dsm segment
would remain attached until the session ends or until it is explicitly
detached. IIUC, the key difference is that a segment will stay mapped
for longer than expected in cases where the mapping was created
when a transaction was aborting.
Thank you for the review comments, it makes sense to include this
check in the callers of the DSA API.
Wrapping the dsa_create/dsa_attach call in the following snippet helps
resolve the issue in case of ProcessGetMemoryContextInterrupt.
+ ResourceOwner current_owner;
+ bool res_releasing = false;
+
+ if (CurrentResourceOwner &&
IsResourceOwnerReleasing(CurrentResourceOwner))
+ {
+ current_owner = CurrentResourceOwner;
+ CurrentResourceOwner = NULL;
+ res_releasing = true;
+ }
+
MemoryStatsDsaArea =
dsa_create(memCxtArea->lw_lock.tranche);
+ if (res_releasing)
+ CurrentResourceOwner = current_owner;
Kindly let me know your views.
Thank you,
Rahila Syed
