From dfb5b10435b52d07cc0f294ee9919d67170b9601 Mon Sep 17 00:00:00 2001 From: Thomas Munro 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