On Mon, May 6, 2019 at 11:07 AM Tom Lane <t...@sss.pgh.pa.us> wrote: > One thing I don't care for about this patch is that the original code > looked like it didn't matter what order we did the resource releases in, > and the patched code still looks like that. 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. Even with that, I'd only > rate it about a 75% chance that somebody won't try to add their new > resource type at the end --- but with no comment, the odds they'll > get it right are indistinguishable from zero.
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. 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. -- Thomas Munro https://enterprisedb.com
From dfb5b10435b52d07cc0f294ee9919d67170b9601 Mon Sep 17 00:00:00 2001 From: Thomas Munro <thomas.munro@enterprisedb.com> Date: Thu, 29 Nov 2018 12:24:07 +1300 Subject: [PATCH] Detach from DSM segments last in resowner.c. It turns out that Windows' FILE_SHARE_DELETE mode doesn't quite give POSIX semantics for unlink(). While it lets you unlink a file or a directory that has an open handle, it doesn't let you unlink a directory that previously contained a file that has an open handle. This has the consequence that an error raised while a SharedFileSet exists can cause us to fail to unlink a temporary directory, even though all the files within it have been unlinked. We write a LOG message and leak the empty directory until the next restart eventually cleans it up. Fix, by detaching from DSM segments only after we have closed all file handles. Since SharedFileSet uses a DSM detach hook to unlink files only when the last backend detaches, it's only our own file handles that we have to worry about. This was a secondary issue discovered while investigating bug #15460. Author: Thomas Munro, based on a suggestion from Robert Haas Discussion: https://postgr.es/m/CA%2BhUKGKVWbz_iniqvFujPZLioFPxGwuVV6PJeeCrQ8SVcdg7FQ%40mail.gmail.com Discussion: https://postgr.es/m/15460-b6db80de822fa0ad@postgresql.org Discussion: https://postgr.es/m/CAEepm=1Ugp7mNFX-YpSfWr0F_6QA4jzjtavauBcoAAZGd7_tPA@mail.gmail.com --- src/backend/utils/resowner/resowner.c | 32 ++++++++++++++++++--------- 1 file changed, 22 insertions(+), 10 deletions(-) diff --git a/src/backend/utils/resowner/resowner.c b/src/backend/utils/resowner/resowner.c index 64aafef311..839df34112 100644 --- a/src/backend/utils/resowner/resowner.c +++ b/src/backend/utils/resowner/resowner.c @@ -536,16 +536,6 @@ ResourceOwnerReleaseInternal(ResourceOwner owner, RelationClose(res); } - /* Ditto for dynamic shared memory segments */ - while (ResourceArrayGetAny(&(owner->dsmarr), &foundres)) - { - dsm_segment *res = (dsm_segment *) DatumGetPointer(foundres); - - if (isCommit) - PrintDSMLeakWarning(res); - dsm_detach(res); - } - /* Ditto for JIT contexts */ while (ResourceArrayGetAny(&(owner->jitarr), &foundres)) { @@ -669,6 +659,28 @@ ResourceOwnerReleaseInternal(ResourceOwner owner, PrintFileLeakWarning(res); FileClose(res); } + + /* + * Ditto for dynamic shared memory segments. + * + * Besides releasing shared memory, dsm_detach() also runs arbitrary + * registered callbacks. We do this after closing temporary files, + * because SharedFileSetOnDetach tries to unlink shared temporary + * directories when the last backend detaches. On Windows, that fails + * if file handles still exist for files inside that directory. + * + * More generally, doing this last prevents extension writers from + * developing DSM detach hooks that depend on other resources and thus + * on the exact release order. + */ + while (ResourceArrayGetAny(&(owner->dsmarr), &foundres)) + { + dsm_segment *res = (dsm_segment *) DatumGetPointer(foundres); + + if (isCommit) + PrintDSMLeakWarning(res); + dsm_detach(res); + } } /* Let add-on modules get a chance too */ -- 2.21.0
