On Mon, May 6, 2019 at 3:44 PM Tom Lane <t...@sss.pgh.pa.us> wrote: > Thomas Munro <thomas.mu...@gmail.com> writes: > > On Mon, May 6, 2019 at 11:07 AM Tom Lane <t...@sss.pgh.pa.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
