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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
