Re: dsm.c API tweak

Thomas Munro wrote:

> I'd word this slightly differently:
>
> + * If there is a CurrentResourceOwner, the new segment is born unpinned and the
> + * resource owner is in charge of destroying it (and will be blamed if it
> + * doesn't). If there's no current resource owner, then the segment starts in
> + * the pinned state, so it'll stay alive until explicitely unpinned.
>
> It's confusing that we've overloaded the term 'pin'. When we 'pin the
> mapping', we're disassociating it from the resource owner so that it
> will remain attached for longer than the current resource owner scope.
> When we 'pin the segment' we are making it survive even if all
> backends detach (= an extra secret refcount). What we're talking
> about here is not pinning the *segment*, but pinning the *mapping* in
> this backend.

I agree that it's unfortunate. I think we would be happier in the long
run if we changed one of those terms. Right now, the whole of dsm.c
appears confusing to me. Maybe one thing that would help is to explain
the distinction between mappings and segments in the comment at the top
of the file. This is a bigger change than I care to tackle right now,
though.

It took me a few minutes to realize that the memory allocated by
dsm_create_descriptor is backend-local, which is why dsm_attach creates
a new descriptor.

> How about: "If there is a non-NULL CurrentResourceOwner, the new
> segment is associated with it and will be automatically detached when
> the resource owner releases. Otherwise it will remain attached until
> explicitly detached. Creating or attaching with a NULL
> CurrentResourceOwner is equivalent to creating or attaching with a
> non-NULL CurrentResourceOwner and then calling dsm_pin_mapping()."

Sounds a lot better, but I don't think it explains the contract exactly
either, at least in the case where there is a resowner: what happens if
the resowner releases is that it logs a complaint (so what the resowner
does is act as cross-check that resources are properly handled
elsewhere, as well as ensuring sane behavior in case of error). I think
we should stress the point that the segment must be detached before the
resowner releases in normal cases. (Also, let's talk about segment
creation in the dsm_create comment, not attachment).

How about this:
If there is a non-NULL CurrentResourceOwner, the new segment is
associated with it and must be detached before the resource owner
releases, or a warning will be logged. If CurrentResourceOwner is
NULL, the segment remains attached until explicitely detached or the
session ends.
Creating with a NULL CurrentResourceOwner is equivalent to creating
with a non-NULL CurrentResourceOwner and then calling dsm_pin_mapping.

> Then dsm_attach() needs to say "See the note above dsm_create() about
> the CurrentResourceOwner.", since it's the same deal there.

I think trying to explain both in the comment for dsm_create() is more
confusing than helpful. I propose to spell out the rule in both places
instead:

If there is a non-NULL CurrentResourceOwner, the attached segment is
associated with it and must be detached before the resource owner
releases, or a warning will be logged. Otherwise the segment remains
attached until explicitely detached or the session ends.

--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services