From 9ec582b64d69fcd103e8a3bc13a621ca376db921 Mon Sep 17 00:00:00 2001
From: Noah Misch <noah@leadboat.com>
Date: Fri, 5 Apr 2019 00:00:52 -0700
Subject: [PATCH] Revert "Consistently test for in-use shared memory."
This reverts commits 2f932f71d9f2963bbd201129d7b971c8f5f077fd,
16ee6eaf80a40007a138b60bb5661660058d0422 and
6f0e190056fe441f7cf788ff19b62b13c94f68f3. The buildfarm has revealed
several bugs. Back-patch like the original commits.
Discussion: https://postgr.es/m/20190404145319.GA1720877@rfd.leadboat.com
---
src/backend/port/sysv_shmem.c | 257 ++++++++++++----------------
src/backend/port/win32_shmem.c | 7 +-
src/backend/postmaster/postmaster.c | 12 +-
src/backend/storage/ipc/ipci.c | 14 +-
src/backend/utils/init/postinit.c | 6 +-
src/include/storage/ipc.h | 2 +-
src/include/storage/pg_shmem.h | 6 +-
7 files changed, 138 insertions(+), 166 deletions(-)
diff --git a/src/backend/port/sysv_shmem.c b/src/backend/port/sysv_shmem.c
index d9f20ed195e..fed5b79fa4c 100644
--- a/src/backend/port/sysv_shmem.c
+++ b/src/backend/port/sysv_shmem.c
@@ -70,26 +70,6 @@
typedef key_t IpcMemoryKey; /* shared memory key passed to shmget(2) */
typedef int IpcMemoryId; /* shared memory ID returned by shmget(2) */
-/*
- * How does a given IpcMemoryId relate to this PostgreSQL process?
- *
- * One could recycle unattached segments of different data directories if we
- * distinguished that case from other SHMSTATE_FOREIGN cases. Doing so would
- * cause us to visit less of the key space, making us less likely to detect a
- * SHMSTATE_ATTACHED key. It would also complicate the concurrency analysis,
- * in that postmasters of different data directories could simultaneously
- * attempt to recycle a given key. We'll waste keys longer in some cases, but
- * avoiding the problems of the alternative justifies that loss.
- */
-typedef enum
-{
- SHMSTATE_ANALYSIS_FAILURE, /* unexpected failure to analyze the ID */
- SHMSTATE_ATTACHED, /* pertinent to DataDir, has attached PIDs */
- SHMSTATE_ENOENT, /* no segment of that ID */
- SHMSTATE_FOREIGN, /* exists, but not pertinent to DataDir */
- SHMSTATE_UNATTACHED /* pertinent to DataDir, no attached PIDs */
-} IpcMemoryState;
-
unsigned long UsedShmemSegID = 0;
void *UsedShmemSegAddr = NULL;
@@ -102,8 +82,8 @@ static void *AnonymousShmem = NULL;
static void *InternalIpcMemoryCreate(IpcMemoryKey memKey, Size size);
static void IpcMemoryDetach(int status, Datum shmaddr);
static void IpcMemoryDelete(int status, Datum shmId);
-static IpcMemoryState PGSharedMemoryAttach(IpcMemoryId shmId,
- PGShmemHeader **addr);
+static PGShmemHeader *PGSharedMemoryAttach(IpcMemoryKey key,
+ IpcMemoryId *shmid);
/*
@@ -307,36 +287,11 @@ IpcMemoryDelete(int status, Datum shmId)
bool
PGSharedMemoryIsInUse(unsigned long id1, unsigned long id2)
{
- PGShmemHeader *memAddress;
- IpcMemoryState state;
-
- state = PGSharedMemoryAttach((IpcMemoryId) id2, &memAddress);
- if (memAddress && shmdt(memAddress) < 0)
- elog(LOG, "shmdt(%p) failed: %m", memAddress);
- switch (state)
- {
- case SHMSTATE_ENOENT:
- case SHMSTATE_FOREIGN:
- case SHMSTATE_UNATTACHED:
- return false;
- case SHMSTATE_ANALYSIS_FAILURE:
- case SHMSTATE_ATTACHED:
- return true;
- }
- return true;
-}
-
-/* See comment at IpcMemoryState. */
-static IpcMemoryState
-PGSharedMemoryAttach(IpcMemoryId shmId,
- PGShmemHeader **addr)
-{
+ IpcMemoryId shmId = (IpcMemoryId) id2;
struct shmid_ds shmStat;
struct stat statbuf;
PGShmemHeader *hdr;
- *addr = NULL;
-
/*
* We detect whether a shared memory segment is in use by seeing whether
* it (a) exists and (b) has any processes attached to it.
@@ -349,7 +304,7 @@ PGSharedMemoryAttach(IpcMemoryId shmId,
* exists.
*/
if (errno == EINVAL)
- return SHMSTATE_ENOENT;
+ return false;
/*
* EACCES implies that the segment belongs to some other userid, which
@@ -357,7 +312,7 @@ PGSharedMemoryAttach(IpcMemoryId shmId,
* is relevant to our data directory).
*/
if (errno == EACCES)
- return SHMSTATE_FOREIGN;
+ return false;
/*
* Some Linux kernel versions (in fact, all of them as of July 2007)
@@ -368,7 +323,7 @@ PGSharedMemoryAttach(IpcMemoryId shmId,
*/
#ifdef HAVE_LINUX_EIDRM_BUG
if (errno == EIDRM)
- return SHMSTATE_ENOENT;
+ return false;
#endif
/*
@@ -376,26 +331,25 @@ PGSharedMemoryAttach(IpcMemoryId shmId,
* only likely case is EIDRM, which implies that the segment has been
* IPC_RMID'd but there are still processes attached to it.
*/
- return SHMSTATE_ANALYSIS_FAILURE;
+ return true;
}
+ /* If it has no attached processes, it's not in use */
+ if (shmStat.shm_nattch == 0)
+ return false;
+
/*
* Try to attach to the segment and see if it matches our data directory.
* This avoids shmid-conflict problems on machines that are running
* several postmasters under the same userid.
*/
if (stat(DataDir, &statbuf) < 0)
- return SHMSTATE_ANALYSIS_FAILURE; /* can't stat; be conservative */
+ return true; /* if can't stat, be conservative */
+
+ hdr = (PGShmemHeader *) shmat(shmId, NULL, PG_SHMAT_FLAGS);
- /*
- * If we can't attach, be conservative. This may fail if postmaster.pid
- * furnished the shmId and another user created a world-readable segment
- * of the same shmId.
- */
- hdr = (PGShmemHeader *) shmat(shmId, UsedShmemSegAddr, PG_SHMAT_FLAGS);
if (hdr == (PGShmemHeader *) -1)
- return SHMSTATE_ANALYSIS_FAILURE;
- *addr = hdr;
+ return true; /* if can't attach, be conservative */
if (hdr->magic != PGShmemMagic ||
hdr->device != statbuf.st_dev ||
@@ -403,12 +357,16 @@ PGSharedMemoryAttach(IpcMemoryId shmId,
{
/*
* It's either not a Postgres segment, or not one for my data
- * directory.
+ * directory. In either case it poses no threat.
*/
- return SHMSTATE_FOREIGN;
+ shmdt((void *) hdr);
+ return false;
}
- return shmStat.shm_nattch == 0 ? SHMSTATE_UNATTACHED : SHMSTATE_ATTACHED;
+ /* Trouble --- looks a lot like there's still live backends */
+ shmdt((void *) hdr);
+
+ return true;
}
#ifdef USE_ANONYMOUS_SHMEM
@@ -584,21 +542,25 @@ AnonymousShmemDetach(int status, Datum arg)
* standard header. Also, register an on_shmem_exit callback to release
* the storage.
*
- * Dead Postgres segments pertinent to this DataDir are recycled if found, but
- * we do not fail upon collision with foreign shmem segments. The idea here
- * is to detect and re-use keys that may have been assigned by a crashed
- * postmaster or backend.
+ * Dead Postgres segments are recycled if found, but we do not fail upon
+ * collision with non-Postgres shmem segments. The idea here is to detect and
+ * re-use keys that may have been assigned by a crashed postmaster or backend.
+ *
+ * makePrivate means to always create a new segment, rather than attach to
+ * or recycle any existing segment.
*
* The port number is passed for possible use as a key (for SysV, we use
- * it to generate the starting shmem key).
+ * it to generate the starting shmem key). In a standalone backend,
+ * zero will be passed.
*/
PGShmemHeader *
-PGSharedMemoryCreate(Size size, int port,
+PGSharedMemoryCreate(Size size, bool makePrivate, int port,
PGShmemHeader **shim)
{
IpcMemoryKey NextShmemSegID;
void *memAddress;
PGShmemHeader *hdr;
+ IpcMemoryId shmid;
struct stat statbuf;
Size sysvsize;
@@ -629,20 +591,11 @@ PGSharedMemoryCreate(Size size, int port,
/* Make sure PGSharedMemoryAttach doesn't fail without need */
UsedShmemSegAddr = NULL;
- /*
- * Loop till we find a free IPC key. Trust CreateDataDirLockFile() to
- * ensure no more than one postmaster per data directory can enter this
- * loop simultaneously. (CreateDataDirLockFile() does not ensure that,
- * but prefer fixing it over coping here.)
- */
- NextShmemSegID = 1 + port * 1000;
+ /* Loop till we find a free IPC key */
+ NextShmemSegID = port * 1000;
- for (;;)
+ for (NextShmemSegID++;; NextShmemSegID++)
{
- IpcMemoryId shmid;
- PGShmemHeader *oldhdr;
- IpcMemoryState state;
-
/* Try to create new segment */
memAddress = InternalIpcMemoryCreate(NextShmemSegID, sysvsize);
if (memAddress)
@@ -650,71 +603,58 @@ PGSharedMemoryCreate(Size size, int port,
/* Check shared memory and possibly remove and recreate */
+ if (makePrivate) /* a standalone backend shouldn't do this */
+ continue;
+
+ if ((memAddress = PGSharedMemoryAttach(NextShmemSegID, &shmid)) == NULL)
+ continue; /* can't attach, not one of mine */
+
/*
- * shmget() failure is typically EACCES, hence SHMSTATE_FOREIGN.
- * ENOENT, a narrow possibility, implies SHMSTATE_ENOENT, but one can
- * safely treat SHMSTATE_ENOENT like SHMSTATE_FOREIGN.
+ * If I am not the creator and it belongs to an extant process,
+ * continue.
*/
- shmid = shmget(NextShmemSegID, sizeof(PGShmemHeader), 0);
- if (shmid < 0)
+ hdr = (PGShmemHeader *) memAddress;
+ if (hdr->creatorPID != getpid())
{
- oldhdr = NULL;
- state = SHMSTATE_FOREIGN;
+ if (kill(hdr->creatorPID, 0) == 0 || errno != ESRCH)
+ {
+ shmdt(memAddress);
+ continue; /* segment belongs to a live process */
+ }
}
- else
- state = PGSharedMemoryAttach(shmid, &oldhdr);
-
- switch (state)
- {
- case SHMSTATE_ANALYSIS_FAILURE:
- case SHMSTATE_ATTACHED:
- ereport(FATAL,
- (errcode(ERRCODE_LOCK_FILE_EXISTS),
- errmsg("pre-existing shared memory block (key %lu, ID %lu) is still in use",
- (unsigned long) NextShmemSegID,
- (unsigned long) shmid),
- errhint("Terminate any old server processes associated with data directory \"%s\".",
- DataDir)));
- break;
- case SHMSTATE_ENOENT:
- /*
- * To our surprise, some other process deleted since our last
- * InternalIpcMemoryCreate(). Moments earlier, we would have
- * seen SHMSTATE_FOREIGN. Try that same ID again.
- */
- elog(LOG,
- "shared memory block (key %lu, ID %lu) deleted during startup",
- (unsigned long) NextShmemSegID,
- (unsigned long) shmid);
- break;
- case SHMSTATE_FOREIGN:
- NextShmemSegID++;
- break;
- case SHMSTATE_UNATTACHED:
+ /*
+ * The segment appears to be from a dead Postgres process, or from a
+ * previous cycle of life in this same process. Zap it, if possible,
+ * and any associated dynamic shared memory segments, as well. This
+ * probably shouldn't fail, but if it does, assume the segment belongs
+ * to someone else after all, and continue quietly.
+ */
+ if (hdr->dsm_control != 0)
+ dsm_cleanup_using_control_segment(hdr->dsm_control);
+ shmdt(memAddress);
+ if (shmctl(shmid, IPC_RMID, NULL) < 0)
+ continue;
- /*
- * The segment pertains to DataDir, and every process that had
- * used it has died or detached. Zap it, if possible, and any
- * associated dynamic shared memory segments, as well. This
- * shouldn't fail, but if it does, assume the segment belongs
- * to someone else after all, and try the next candidate.
- * Otherwise, try again to create the segment. That may fail
- * if some other process creates the same shmem key before we
- * do, in which case we'll try the next key.
- */
- if (oldhdr->dsm_control != 0)
- dsm_cleanup_using_control_segment(oldhdr->dsm_control);
- if (shmctl(shmid, IPC_RMID, NULL) < 0)
- NextShmemSegID++;
- break;
- }
+ /*
+ * Now try again to create the segment.
+ */
+ memAddress = InternalIpcMemoryCreate(NextShmemSegID, sysvsize);
+ if (memAddress)
+ break; /* successful create and attach */
- if (oldhdr && shmdt(oldhdr) < 0)
- elog(LOG, "shmdt(%p) failed: %m", oldhdr);
+ /*
+ * Can only get here if some other process managed to create the same
+ * shmem key before we did. Let him have that one, loop around to try
+ * next key.
+ */
}
- /* Initialize new segment. */
+ /*
+ * OK, we created a new segment. Mark it as created by this process. The
+ * order of assignments here is critical so that another Postgres process
+ * can't see the header as valid but belonging to an invalid PID!
+ */
hdr = (PGShmemHeader *) memAddress;
hdr->creatorPID = getpid();
hdr->magic = PGShmemMagic;
@@ -774,8 +714,7 @@ void
PGSharedMemoryReAttach(void)
{
IpcMemoryId shmid;
- PGShmemHeader *hdr;
- IpcMemoryState state;
+ void *hdr;
void *origUsedShmemSegAddr = UsedShmemSegAddr;
Assert(UsedShmemSegAddr != NULL);
@@ -788,18 +727,14 @@ PGSharedMemoryReAttach(void)
#endif
elog(DEBUG3, "attaching to %p", UsedShmemSegAddr);
- shmid = shmget(UsedShmemSegID, sizeof(PGShmemHeader), 0);
- if (shmid < 0)
- state = SHMSTATE_FOREIGN;
- else
- state = PGSharedMemoryAttach(shmid, &hdr);
- if (state != SHMSTATE_ATTACHED)
+ hdr = (void *) PGSharedMemoryAttach((IpcMemoryKey) UsedShmemSegID, &shmid);
+ if (hdr == NULL)
elog(FATAL, "could not reattach to shared memory (key=%d, addr=%p): %m",
(int) UsedShmemSegID, UsedShmemSegAddr);
if (hdr != origUsedShmemSegAddr)
elog(FATAL, "reattaching to shared memory returned unexpected address (got %p, expected %p)",
hdr, origUsedShmemSegAddr);
- dsm_set_control_handle(hdr->dsm_control);
+ dsm_set_control_handle(((PGShmemHeader *) hdr)->dsm_control);
UsedShmemSegAddr = hdr; /* probably redundant */
}
@@ -875,3 +810,31 @@ PGSharedMemoryDetach(void)
}
#endif
}
+
+
+/*
+ * Attach to shared memory and make sure it has a Postgres header
+ *
+ * Returns attach address if OK, else NULL
+ */
+static PGShmemHeader *
+PGSharedMemoryAttach(IpcMemoryKey key, IpcMemoryId *shmid)
+{
+ PGShmemHeader *hdr;
+
+ if ((*shmid = shmget(key, sizeof(PGShmemHeader), 0)) < 0)
+ return NULL;
+
+ hdr = (PGShmemHeader *) shmat(*shmid, UsedShmemSegAddr, PG_SHMAT_FLAGS);
+
+ if (hdr == (PGShmemHeader *) -1)
+ return NULL; /* failed: must be some other app's */
+
+ if (hdr->magic != PGShmemMagic)
+ {
+ shmdt((void *) hdr);
+ return NULL; /* segment belongs to a non-Postgres app */
+ }
+
+ return hdr;
+}
diff --git a/src/backend/port/win32_shmem.c b/src/backend/port/win32_shmem.c
index ac3192c4c2d..110bdcc703d 100644
--- a/src/backend/port/win32_shmem.c
+++ b/src/backend/port/win32_shmem.c
@@ -109,9 +109,14 @@ PGSharedMemoryIsInUse(unsigned long id1, unsigned long id2)
*
* Create a shared memory segment of the given size and initialize its
* standard header.
+ *
+ * makePrivate means to always create a new segment, rather than attach to
+ * 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, int port,
+PGSharedMemoryCreate(Size size, bool makePrivate, int port,
PGShmemHeader **shim)
{
void *memAddress;
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index 6011282666f..644b4c6b2c8 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -2517,7 +2517,7 @@ reset_shared(int port)
* determine IPC keys. This helps ensure that we will clean up dead IPC
* objects if the postmaster crashes and is restarted.
*/
- CreateSharedMemoryAndSemaphores(port);
+ CreateSharedMemoryAndSemaphores(false, port);
}
@@ -4804,7 +4804,7 @@ SubPostmasterMain(int argc, char *argv[])
InitProcess();
/* Attach process to shared data structures */
- CreateSharedMemoryAndSemaphores(0);
+ CreateSharedMemoryAndSemaphores(false, 0);
/* And run the backend */
BackendRun(&port); /* does not return */
@@ -4818,7 +4818,7 @@ SubPostmasterMain(int argc, char *argv[])
InitAuxiliaryProcess();
/* Attach process to shared data structures */
- CreateSharedMemoryAndSemaphores(0);
+ CreateSharedMemoryAndSemaphores(false, 0);
AuxiliaryProcessMain(argc - 2, argv + 2); /* does not return */
}
@@ -4831,7 +4831,7 @@ SubPostmasterMain(int argc, char *argv[])
InitProcess();
/* Attach process to shared data structures */
- CreateSharedMemoryAndSemaphores(0);
+ CreateSharedMemoryAndSemaphores(false, 0);
AutoVacLauncherMain(argc - 2, argv + 2); /* does not return */
}
@@ -4844,7 +4844,7 @@ SubPostmasterMain(int argc, char *argv[])
InitProcess();
/* Attach process to shared data structures */
- CreateSharedMemoryAndSemaphores(0);
+ CreateSharedMemoryAndSemaphores(false, 0);
AutoVacWorkerMain(argc - 2, argv + 2); /* does not return */
}
@@ -4862,7 +4862,7 @@ SubPostmasterMain(int argc, char *argv[])
InitProcess();
/* Attach process to shared data structures */
- CreateSharedMemoryAndSemaphores(0);
+ CreateSharedMemoryAndSemaphores(false, 0);
/* Fetch MyBgworkerEntry from shared memory */
shmem_slot = atoi(argv[1] + 15);
diff --git a/src/backend/storage/ipc/ipci.c b/src/backend/storage/ipc/ipci.c
index d949bed9d51..32ac58f7d1a 100644
--- a/src/backend/storage/ipc/ipci.c
+++ b/src/backend/storage/ipc/ipci.c
@@ -85,9 +85,12 @@ RequestAddinShmemSpace(Size size)
* through the same code as before. (Note that the called routines mostly
* check IsUnderPostmaster, rather than EXEC_BACKEND, to detect this case.
* This is a bit code-wasteful and could be cleaned up.)
+ *
+ * If "makePrivate" is true then we only need private memory, not shared
+ * memory. This is true for a standalone backend, false for a postmaster.
*/
void
-CreateSharedMemoryAndSemaphores(int port)
+CreateSharedMemoryAndSemaphores(bool makePrivate, int port)
{
PGShmemHeader *shim = NULL;
@@ -152,7 +155,7 @@ CreateSharedMemoryAndSemaphores(int port)
/*
* Create the shmem segment
*/
- seghdr = PGSharedMemoryCreate(size, port, &shim);
+ seghdr = PGSharedMemoryCreate(size, makePrivate, port, &shim);
InitShmemAccess(seghdr);
@@ -167,9 +170,12 @@ CreateSharedMemoryAndSemaphores(int port)
{
/*
* We are reattaching to an existing shared memory segment. This
- * should only be reached in the EXEC_BACKEND case.
+ * should only be reached in the EXEC_BACKEND case, and even then only
+ * with makePrivate == false.
*/
-#ifndef EXEC_BACKEND
+#ifdef EXEC_BACKEND
+ Assert(!makePrivate);
+#else
elog(PANIC, "should be attached to shared memory already");
#endif
}
diff --git a/src/backend/utils/init/postinit.c b/src/backend/utils/init/postinit.c
index 5dd43dfea2a..4a7ceb53ea4 100644
--- a/src/backend/utils/init/postinit.c
+++ b/src/backend/utils/init/postinit.c
@@ -402,11 +402,9 @@ InitCommunication(void)
{
/*
* We're running a postgres bootstrap process or a standalone backend.
- * Though we won't listen on PostPortNumber, use it to select a shmem
- * key. This increases the chance of detecting a leftover live
- * backend of this DataDir.
+ * Create private "shmem" and semaphores.
*/
- CreateSharedMemoryAndSemaphores(PostPortNumber);
+ CreateSharedMemoryAndSemaphores(true, 0);
}
}
diff --git a/src/include/storage/ipc.h b/src/include/storage/ipc.h
index 836285ec09f..c6283c2af34 100644
--- a/src/include/storage/ipc.h
+++ b/src/include/storage/ipc.h
@@ -75,6 +75,6 @@ extern void on_exit_reset(void);
/* ipci.c */
extern PGDLLIMPORT shmem_startup_hook_type shmem_startup_hook;
-extern void CreateSharedMemoryAndSemaphores(int port);
+extern void CreateSharedMemoryAndSemaphores(bool makePrivate, int port);
#endif /* IPC_H */
diff --git a/src/include/storage/pg_shmem.h b/src/include/storage/pg_shmem.h
index 49fda225eb6..9dbcbce0692 100644
--- a/src/include/storage/pg_shmem.h
+++ b/src/include/storage/pg_shmem.h
@@ -30,7 +30,7 @@ typedef struct PGShmemHeader /* standard header for all Postgres shmem */
{
int32 magic; /* magic # to identify Postgres segments */
#define PGShmemMagic 679834894
- pid_t creatorPID; /* PID of creating process (set but unread) */
+ pid_t creatorPID; /* PID of creating process */
Size totalsize; /* total size of segment */
Size freeoffset; /* offset to first free space */
dsm_handle dsm_control; /* ID of dynamic shared memory control seg */
@@ -64,8 +64,8 @@ extern void PGSharedMemoryReAttach(void);
extern void PGSharedMemoryNoReAttach(void);
#endif
-extern PGShmemHeader *PGSharedMemoryCreate(Size size, int port,
- PGShmemHeader **shim);
+extern PGShmemHeader *PGSharedMemoryCreate(Size size, bool makePrivate,
+ int port, PGShmemHeader **shim);
extern bool PGSharedMemoryIsInUse(unsigned long id1, unsigned long id2);
extern void PGSharedMemoryDetach(void);
--
GitLab