From: Tom Lane Date: Tue, 13 Oct 2015 15:21:33 +0000 (-0400) Subject: On Windows, ensure shared memory handle gets closed if not being used. X-Git-Tag: REL9_1_20~74 X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=b0d8583593b5f778a1c79203f852a6d848294376;p=postgresql On Windows, ensure shared memory handle gets closed if not being used. Postmaster child processes that aren't supposed to be attached to shared memory were not bothering to close the shared memory mapping handle they inherit from the postmaster process. That's mostly harmless, since the handle vanishes anyway when the child process exits -- but the syslogger process, if used, doesn't get killed and restarted during recovery from a backend crash. That meant that Windows doesn't see the shared memory mapping as becoming free, so it doesn't delete it and the postmaster is unable to create a new one, resulting in failure to recover from crashes whenever logging_collector is turned on. Per report from Dmitry Vasilyev. It's a bit astonishing that we'd not figured this out long ago, since it's been broken from the very beginnings of out native Windows support; probably some previously-unexplained trouble reports trace to this. A secondary problem is that on Cygwin (perhaps only in older versions?), exec() may not detach from the shared memory segment after all, in which case these child processes did remain attached to shared memory, posing the risk of an unexpected shared memory clobber if they went off the rails somehow. That may be a long-gone bug, but we can deal with it now if it's still live, by detaching within the infrastructure introduced here to deal with closing the handle. Back-patch to all supported branches. Tom Lane and Amit Kapila --- diff --git a/src/backend/port/sysv_shmem.c b/src/backend/port/sysv_shmem.c index c91c86219b..b39ce8501e 100644 --- a/src/backend/port/sysv_shmem.c +++ b/src/backend/port/sysv_shmem.c @@ -214,7 +214,7 @@ InternalIpcMemoryCreate(IpcMemoryKey memKey, Size size) /****************************************************************************/ /* IpcMemoryDetach(status, shmaddr) removes a shared memory segment */ -/* from process' address spaceq */ +/* from process' address space */ /* (called as an on_shmem_exit callback, hence funny argument list) */ /****************************************************************************/ static void @@ -458,9 +458,10 @@ PGSharedMemoryCreate(Size size, bool makePrivate, int port) /* * PGSharedMemoryReAttach * - * Re-attach to an already existing shared memory segment. In the non - * EXEC_BACKEND case this is not used, because postmaster children inherit - * the shared memory segment attachment via fork(). + * This is called during startup of a postmaster child process to re-attach to + * an already existing shared memory segment. This is needed only in the + * EXEC_BACKEND case; otherwise postmaster children inherit the shared memory + * segment attachment via fork(). * * UsedShmemSegID and UsedShmemSegAddr are implicit parameters to this * routine. The caller must have already restored them to the postmaster's @@ -493,16 +494,52 @@ PGSharedMemoryReAttach(void) UsedShmemSegAddr = hdr; /* probably redundant */ } + +/* + * PGSharedMemoryNoReAttach + * + * This is called during startup of a postmaster child process when we choose + * *not* to re-attach to the existing shared memory segment. We must clean up + * to leave things in the appropriate state. This is not used in the non + * EXEC_BACKEND case, either. + * + * The child process startup logic might or might not call PGSharedMemoryDetach + * after this; make sure that it will be a no-op if called. + * + * UsedShmemSegID and UsedShmemSegAddr are implicit parameters to this + * routine. The caller must have already restored them to the postmaster's + * values. + */ +void +PGSharedMemoryNoReAttach(void) +{ + Assert(UsedShmemSegAddr != NULL); + Assert(IsUnderPostmaster); + +#ifdef __CYGWIN__ + /* cygipc (currently) appears to not detach on exec. */ + PGSharedMemoryDetach(); +#endif + + /* For cleanliness, reset UsedShmemSegAddr to show we're not attached. */ + UsedShmemSegAddr = NULL; + /* And the same for UsedShmemSegID. */ + UsedShmemSegID = 0; +} + #endif /* EXEC_BACKEND */ /* * PGSharedMemoryDetach * * Detach from the shared memory segment, if still attached. This is not - * intended for use by the process that originally created the segment - * (it will have an on_shmem_exit callback registered to do that). Rather, - * this is for subprocesses that have inherited an attachment and want to - * get rid of it. + * intended to be called explicitly by the process that originally created the + * segment (it will have an on_shmem_exit callback registered to do that). + * Rather, this is for subprocesses that have inherited an attachment and want + * to get rid of it. + * + * UsedShmemSegID and UsedShmemSegAddr are implicit parameters to this + * routine. */ void PGSharedMemoryDetach(void) diff --git a/src/backend/port/win32_shmem.c b/src/backend/port/win32_shmem.c index d697856639..5dd411c86f 100644 --- a/src/backend/port/win32_shmem.c +++ b/src/backend/port/win32_shmem.c @@ -16,7 +16,7 @@ #include "storage/ipc.h" #include "storage/pg_shmem.h" -HANDLE UsedShmemSegID = 0; +HANDLE UsedShmemSegID = INVALID_HANDLE_VALUE; void *UsedShmemSegAddr = NULL; static Size UsedShmemSegSize = 0; @@ -82,7 +82,6 @@ GetSharedMemName(void) * we only care about shmem segments that are associated with the intended * DataDir. This is an important consideration since accidental matches of * shmem segment IDs are reasonably common. - * */ bool PGSharedMemoryIsInUse(unsigned long id1, unsigned long id2) @@ -114,7 +113,6 @@ PGSharedMemoryIsInUse(unsigned long id1, unsigned long id2) * or recycle any existing segment. On win32, we always create a new segment, * since there is no need for recycling (segments go away automatically * when the last backend exits) - * */ PGShmemHeader * PGSharedMemoryCreate(Size size, bool makePrivate, int port) @@ -211,9 +209,6 @@ PGSharedMemoryCreate(Size size, bool makePrivate, int port) elog(LOG, "could not close handle to shared memory: %lu", GetLastError()); - /* Register on-exit routine to delete the new segment */ - on_shmem_exit(pgwin32_SharedMemoryDelete, PointerGetDatum(hmap2)); - /* * Get a pointer to the new shared memory segment. Map the whole segment * at once, and let the system decide on the initial address. @@ -246,14 +241,18 @@ PGSharedMemoryCreate(Size size, bool makePrivate, int port) UsedShmemSegSize = size; UsedShmemSegID = hmap2; + /* Register on-exit routine to delete the new segment */ + on_shmem_exit(pgwin32_SharedMemoryDelete, PointerGetDatum(hmap2)); + return hdr; } /* * PGSharedMemoryReAttach * - * Re-attach to an already existing shared memory segment. Use the - * handle inherited from the postmaster. + * This is called during startup of a postmaster child process to re-attach to + * an already existing shared memory segment, using the handle inherited from + * the postmaster. * * UsedShmemSegID and UsedShmemSegAddr are implicit parameters to this * routine. The caller must have already restored them to the postmaster's @@ -288,37 +287,88 @@ PGSharedMemoryReAttach(void) UsedShmemSegAddr = hdr; /* probably redundant */ } +/* + * PGSharedMemoryNoReAttach + * + * This is called during startup of a postmaster child process when we choose + * *not* to re-attach to the existing shared memory segment. We must clean up + * to leave things in the appropriate state. + * + * The child process startup logic might or might not call PGSharedMemoryDetach + * after this; make sure that it will be a no-op if called. + * + * UsedShmemSegID and UsedShmemSegAddr are implicit parameters to this + * routine. The caller must have already restored them to the postmaster's + * values. + */ +void +PGSharedMemoryNoReAttach(void) +{ + Assert(UsedShmemSegAddr != NULL); + Assert(IsUnderPostmaster); + + /* + * Under Windows we will not have mapped the segment, so we don't need to + * un-map it. Just reset UsedShmemSegAddr to show we're not attached. + */ + UsedShmemSegAddr = NULL; + + /* + * We *must* close the inherited shmem segment handle, else Windows will + * consider the existence of this process to mean it can't release the + * shmem segment yet. We can now use PGSharedMemoryDetach to do that. + */ + PGSharedMemoryDetach(); +} + /* * PGSharedMemoryDetach * * Detach from the shared memory segment, if still attached. This is not - * intended for use by the process that originally created the segment. Rather, - * this is for subprocesses that have inherited an attachment and want to - * get rid of it. + * intended to be called explicitly by the process that originally created the + * segment (it will have an on_shmem_exit callback registered to do that). + * Rather, this is for subprocesses that have inherited an attachment and want + * to get rid of it. + * + * UsedShmemSegID and UsedShmemSegAddr are implicit parameters to this + * routine. */ void PGSharedMemoryDetach(void) { + /* Unmap the view, if it's mapped */ if (UsedShmemSegAddr != NULL) { if (!UnmapViewOfFile(UsedShmemSegAddr)) - elog(LOG, "could not unmap view of shared memory: %lu", GetLastError()); + elog(LOG, "could not unmap view of shared memory: error code %lu", + GetLastError()); UsedShmemSegAddr = NULL; } + + /* And close the shmem handle, if we have one */ + if (UsedShmemSegID != INVALID_HANDLE_VALUE) + { + if (!CloseHandle(UsedShmemSegID)) + elog(LOG, "could not close handle to shared memory: error code %lu", + GetLastError()); + + UsedShmemSegID = INVALID_HANDLE_VALUE; + } } /* - * pgwin32_SharedMemoryDelete(status, shmId) deletes a shared memory segment - * (called as an on_shmem_exit callback, hence funny argument list) + * pgwin32_SharedMemoryDelete + * + * Detach from and delete the shared memory segment + * (called as an on_shmem_exit callback, hence funny argument list) */ static void pgwin32_SharedMemoryDelete(int status, Datum shmId) { + Assert(DatumGetPointer(shmId) == UsedShmemSegID); PGSharedMemoryDetach(); - if (!CloseHandle(DatumGetPointer(shmId))) - elog(LOG, "could not close handle to shared memory: %lu", GetLastError()); } /* diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c index bdc809dd8d..9b83065474 100644 --- a/src/backend/postmaster/postmaster.c +++ b/src/backend/postmaster/postmaster.c @@ -4173,13 +4173,16 @@ SubPostmasterMain(int argc, char *argv[]) /* * If appropriate, physically re-attach to shared memory segment. We want * to do this before going any further to ensure that we can attach at the - * same address the postmaster used. + * same address the postmaster used. On the other hand, if we choose not + * to re-attach, we may have other cleanup to do. */ if (strcmp(argv[1], "--forkbackend") == 0 || strcmp(argv[1], "--forkavlauncher") == 0 || strcmp(argv[1], "--forkavworker") == 0 || strcmp(argv[1], "--forkboot") == 0) PGSharedMemoryReAttach(); + else + PGSharedMemoryNoReAttach(); /* autovacuum needs this set before calling InitProcess */ if (strcmp(argv[1], "--forkavlauncher") == 0) diff --git a/src/include/storage/pg_shmem.h b/src/include/storage/pg_shmem.h index 5af1116645..428ff21fa3 100644 --- a/src/include/storage/pg_shmem.h +++ b/src/include/storage/pg_shmem.h @@ -48,6 +48,7 @@ extern HANDLE UsedShmemSegID; extern void *UsedShmemSegAddr; extern void PGSharedMemoryReAttach(void); +extern void PGSharedMemoryNoReAttach(void); #endif extern PGShmemHeader *PGSharedMemoryCreate(Size size, bool makePrivate,