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

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);