Re: Fixing order of resowner cleanup in 12, for Windows

On Mon, May 6, 2019 at 3:44 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Thomas Munro <thomas(dot)munro(at)gmail(dot)com> writes:
> > On Mon, May 6, 2019 at 11:07 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> >> ... You're not doing future
> >> hackers any service by failing to include a comment that explains that
> >> DSM detach MUST BE LAST, and explaining why.
>
> > Ok, here's a version that provides a specific reason (the Windows file
> > handle thing) and also a more general reasoning: we don't really want
> > extension (or core) authors writing callbacks that depend on eg pins
> > or locks or whatever else being still held when they run, because
> > that's fragile, so calling them last is the best and most conservative
> > choice.
>
> LGTM.

Cool. I'll wait a bit to see if we can get confirmation from a
Windows hacker that it does what I claim. Or maybe I should try to
come up with a regression test that exercises it without having to
create a big table.

> > ... I think if someone does come with legitimate reasons to want
> > that, we should discuss it then, and perhaps consider something a bit
> > like the ResourceRelease_callbacks list: its callbacks are invoked for
> > each phase.
>
> Hmm, now that you mention it: this bit at the very end
>
> /* Let add-on modules get a chance too */
> for (item = ResourceRelease_callbacks; item; item = item->next)
> item->callback(phase, isCommit, isTopLevel, item->arg);
>
> seems kind of misplaced given this discussion. Should we not run that
> *first*, before we release core resources for the same phase? It's
> a lot more plausible that extension resources depend on core resources
> than vice versa.

Not sure. Changing the meaning of the existing callbacks from last
to first in each phase seems a bit unfriendly. If it's useful to be
able to run a callback before RESOURCE_RELEASE_BEFORE_LOCKS, perhaps
we need a new phase that comes before that?

--
Thomas Munro
https://enterprisedb.com