Re: Unexpected "shared memory block is still in use" - Mailing list pgsql-hackers

I wrote:
> Attached is a draft patch to change both shmem and sema key selection
> to be based on data directory inode rather than port.
> ...
> I'm not quite sure what's going on in src/test/recovery/t/017_shm.pl.
> As expected, the test for port number non-collision no longer sees
> a failure.  After fixing that, the test passes, but it takes a
> ridiculously long time (minutes); apparently each postmaster start/stop
> cycle takes much longer than it ought to.  I suppose this patch is
> breaking its assumptions, but I've not studied it.

After looking closer, the problem is pretty obvious: the initial
loop is trying to create a cluster whose shmem key matches its
port-number-based expectation.  With this code, that will never
happen except by unlikely accident, so it wastes time with repeated
initdb/start/stop attempts.  After 100 tries it gives up and presses
on with the test, resulting in the apparent pass with long runtime.

I now understand the point you made upthread that this test could
only be preserved if we invent some way to force the choice of shmem
key.  While it wouldn't be hard to do that (say, invent a magic
environment variable), I really don't want to do so.  In the field,
such a behavior would have no positive use, and it could destroy our
newly-improved guarantees about detecting conflicting old processes.

However, there's another way to skin this cat.  We can have the
Perl test script create a conflicting shmem segment directly,
as in the attached second-draft patch.  I simplified the test
script quite a bit, since I don't see any particular value in
creating more than one test postmaster with this approach.

This still isn't committable as-is, since the test will just curl up
and die on machines lacking IPC::SharedMem.  (It could be rewritten
to rely only on the lower-level IPC::SysV module, but I doubt that's
worth the trouble, since either way it'd fail on Windows.)  I'm not
sure whether we should just not bother to run the test at all, or
if we should run it but skip the IPC-related parts; and my Perl-fu
isn't really up to implementing either behavior.

Another thing that might be interesting is to do more than just create
the conflicting segment, ie, try to put some data into it that would
fool the postmaster.  I'm not excited about that at all, but maybe
someone else is?

The attached patch is identical to the previous one except for the
changes in src/test/recovery/t/017_shm.pl.

            regards, tom lane

diff --git a/src/backend/port/posix_sema.c b/src/backend/port/posix_sema.c
index 3370adf..bdd6552 100644
--- a/src/backend/port/posix_sema.c
+++ b/src/backend/port/posix_sema.c
@@ -29,6 +29,7 @@
 #include <semaphore.h>
 #include <signal.h>
 #include <unistd.h>
+#include <sys/stat.h>

 #include "miscadmin.h"
 #include "storage/ipc.h"
@@ -181,10 +182,6 @@ PGSemaphoreShmemSize(int maxSemas)
  * are acquired here or in PGSemaphoreCreate, register an on_shmem_exit
  * callback to release them.
  *
- * The port number is passed for possible use as a key (for Posix, we use
- * it to generate the starting semaphore name).  In a standalone backend,
- * zero will be passed.
- *
  * In the Posix implementation, we acquire semaphores on-demand; the
  * maxSemas parameter is just used to size the arrays.  For unnamed
  * semaphores, there is an array of PGSemaphoreData structs in shared memory.
@@ -196,8 +193,22 @@ PGSemaphoreShmemSize(int maxSemas)
  * we don't have to expose the counters to other processes.)
  */
 void
-PGReserveSemaphores(int maxSemas, int port)
+PGReserveSemaphores(int maxSemas)
 {
+    struct stat statbuf;
+
+    /*
+     * We use the data directory's inode number to seed the search for free
+     * semaphore keys.  This minimizes the odds of collision with other
+     * postmasters, while maximizing the odds that we will detect and clean up
+     * semaphores left over from a crashed postmaster in our own directory.
+     */
+    if (stat(DataDir, &statbuf) < 0)
+        ereport(FATAL,
+                (errcode_for_file_access(),
+                 errmsg("could not stat data directory \"%s\": %m",
+                        DataDir)));
+
 #ifdef USE_NAMED_POSIX_SEMAPHORES
     mySemPointers = (sem_t **) malloc(maxSemas * sizeof(sem_t *));
     if (mySemPointers == NULL)
@@ -215,7 +226,7 @@ PGReserveSemaphores(int maxSemas, int port)

     numSems = 0;
     maxSems = maxSemas;
-    nextSemKey = port * 1000;
+    nextSemKey = statbuf.st_ino;

     on_shmem_exit(ReleaseSemaphores, 0);
 }
diff --git a/src/backend/port/sysv_sema.c b/src/backend/port/sysv_sema.c
index ac5106f..a1652cc 100644
--- a/src/backend/port/sysv_sema.c
+++ b/src/backend/port/sysv_sema.c
@@ -17,6 +17,7 @@
 #include <signal.h>
 #include <unistd.h>
 #include <sys/file.h>
+#include <sys/stat.h>
 #ifdef HAVE_SYS_IPC_H
 #include <sys/ipc.h>
 #endif
@@ -301,10 +302,6 @@ PGSemaphoreShmemSize(int maxSemas)
  * are acquired here or in PGSemaphoreCreate, register an on_shmem_exit
  * callback to release them.
  *
- * The port number is passed for possible use as a key (for SysV, we use
- * it to generate the starting semaphore key).  In a standalone backend,
- * zero will be passed.
- *
  * In the SysV implementation, we acquire semaphore sets on-demand; the
  * maxSemas parameter is just used to size the arrays.  There is an array
  * of PGSemaphoreData structs in shared memory, and a postmaster-local array
@@ -314,8 +311,22 @@ PGSemaphoreShmemSize(int maxSemas)
  * have clobbered.)
  */
 void
-PGReserveSemaphores(int maxSemas, int port)
+PGReserveSemaphores(int maxSemas)
 {
+    struct stat statbuf;
+
+    /*
+     * We use the data directory's inode number to seed the search for free
+     * semaphore keys.  This minimizes the odds of collision with other
+     * postmasters, while maximizing the odds that we will detect and clean up
+     * semaphores left over from a crashed postmaster in our own directory.
+     */
+    if (stat(DataDir, &statbuf) < 0)
+        ereport(FATAL,
+                (errcode_for_file_access(),
+                 errmsg("could not stat data directory \"%s\": %m",
+                        DataDir)));
+
     /*
      * We must use ShmemAllocUnlocked(), since the spinlock protecting
      * ShmemAlloc() won't be ready yet.  (This ordering is necessary when we
@@ -332,7 +343,7 @@ PGReserveSemaphores(int maxSemas, int port)
     if (mySemaSets == NULL)
         elog(PANIC, "out of memory");
     numSemaSets = 0;
-    nextSemaKey = port * 1000;
+    nextSemaKey = statbuf.st_ino;
     nextSemaNumber = SEMAS_PER_SET; /* force sema set alloc on 1st call */

     on_shmem_exit(ReleaseSemaphores, 0);
diff --git a/src/backend/port/sysv_shmem.c b/src/backend/port/sysv_shmem.c
index 968506d..dab2920 100644
--- a/src/backend/port/sysv_shmem.c
+++ b/src/backend/port/sysv_shmem.c
@@ -390,11 +390,12 @@ PGSharedMemoryAttach(IpcMemoryId shmId,

     /*
      * Try to attach to the segment and see if it matches our data directory.
-     * This avoids key-conflict problems on machines that are running several
-     * postmasters under the same userid and port number.  (That would not
-     * ordinarily happen in production, but it can happen during parallel
-     * testing.  Since our test setups don't open any TCP ports on Unix, such
-     * cases don't conflict otherwise.)
+     * This avoids any risk of duplicate-shmem-key conflicts on machines that
+     * are running several postmasters under the same userid.
+     *
+     * (When we're called from PGSharedMemoryCreate, this stat call is
+     * duplicative; but since this isn't a high-traffic case it's not worth
+     * trying to optimize.)
      */
     if (stat(DataDir, &statbuf) < 0)
         return SHMSTATE_ANALYSIS_FAILURE;    /* can't stat; be conservative */
@@ -617,12 +618,9 @@ AnonymousShmemDetach(int status, Datum arg)
  * 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.
- *
- * The port number is passed for possible use as a key (for SysV, we use
- * it to generate the starting shmem key).
  */
 PGShmemHeader *
-PGSharedMemoryCreate(Size size, int port,
+PGSharedMemoryCreate(Size size,
                      PGShmemHeader **shim)
 {
     IpcMemoryKey NextShmemSegID;
@@ -631,6 +629,17 @@ PGSharedMemoryCreate(Size size, int port,
     struct stat statbuf;
     Size        sysvsize;

+    /*
+     * We use the data directory's ID info (inode and device numbers) to
+     * positively identify shmem segments associated with this data dir, and
+     * also as seeds for searching for a free shmem key.
+     */
+    if (stat(DataDir, &statbuf) < 0)
+        ereport(FATAL,
+                (errcode_for_file_access(),
+                 errmsg("could not stat data directory \"%s\": %m",
+                        DataDir)));
+
     /* Complain if hugepages demanded but we can't possibly support them */
 #if !defined(MAP_HUGETLB)
     if (huge_pages == HUGE_PAGES_ON)
@@ -659,10 +668,10 @@ PGSharedMemoryCreate(Size size, int port,
     /*
      * 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.)
+     * loop simultaneously.  (CreateDataDirLockFile() does not entirely ensure
+     * that, but prefer fixing it over coping here.)
      */
-    NextShmemSegID = 1 + port * 1000;
+    NextShmemSegID = statbuf.st_ino;

     for (;;)
     {
@@ -748,11 +757,6 @@ PGSharedMemoryCreate(Size size, int port,
     hdr->dsm_control = 0;

     /* Fill in the data directory ID info, too */
-    if (stat(DataDir, &statbuf) < 0)
-        ereport(FATAL,
-                (errcode_for_file_access(),
-                 errmsg("could not stat data directory \"%s\": %m",
-                        DataDir)));
     hdr->device = statbuf.st_dev;
     hdr->inode = statbuf.st_ino;

diff --git a/src/backend/port/win32_sema.c b/src/backend/port/win32_sema.c
index 013c122..32cc697 100644
--- a/src/backend/port/win32_sema.c
+++ b/src/backend/port/win32_sema.c
@@ -44,7 +44,7 @@ PGSemaphoreShmemSize(int maxSemas)
  * process exits.
  */
 void
-PGReserveSemaphores(int maxSemas, int port)
+PGReserveSemaphores(int maxSemas)
 {
     mySemSet = (HANDLE *) malloc(maxSemas * sizeof(HANDLE));
     if (mySemSet == NULL)
diff --git a/src/backend/port/win32_shmem.c b/src/backend/port/win32_shmem.c
index ccd7b6b..6cb6328 100644
--- a/src/backend/port/win32_shmem.c
+++ b/src/backend/port/win32_shmem.c
@@ -194,7 +194,7 @@ EnableLockPagesPrivilege(int elevel)
  * standard header.
  */
 PGShmemHeader *
-PGSharedMemoryCreate(Size size, int port,
+PGSharedMemoryCreate(Size size,
                      PGShmemHeader **shim)
 {
     void       *memAddress;
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index 62dc93d..a5446d5 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -385,7 +385,7 @@ static void getInstallationPaths(const char *argv0);
 static void checkControlFile(void);
 static Port *ConnCreate(int serverFd);
 static void ConnFree(Port *port);
-static void reset_shared(int port);
+static void reset_shared(void);
 static void SIGHUP_handler(SIGNAL_ARGS);
 static void pmdie(SIGNAL_ARGS);
 static void reaper(SIGNAL_ARGS);
@@ -1175,7 +1175,7 @@ PostmasterMain(int argc, char *argv[])
     /*
      * Set up shared memory and semaphores.
      */
-    reset_shared(PostPortNumber);
+    reset_shared();

     /*
      * Estimate number of openable files.  This must happen after setting up
@@ -2599,17 +2599,16 @@ InitProcessGlobals(void)
  * reset_shared -- reset shared memory and semaphores
  */
 static void
-reset_shared(int port)
+reset_shared(void)
 {
     /*
      * Create or re-create shared memory and semaphores.
      *
      * Note: in each "cycle of life" we will normally assign the same IPC keys
-     * (if using SysV shmem and/or semas), since the port number is used to
-     * determine IPC keys.  This helps ensure that we will clean up dead IPC
-     * objects if the postmaster crashes and is restarted.
+     * (if using SysV shmem and/or semas).  This helps ensure that we will
+     * clean up dead IPC objects if the postmaster crashes and is restarted.
      */
-    CreateSharedMemoryAndSemaphores(port);
+    CreateSharedMemoryAndSemaphores();
 }


@@ -3934,7 +3933,7 @@ PostmasterStateMachine(void)
         /* re-read control file into local memory */
         LocalProcessControlFile(true);

-        reset_shared(PostPortNumber);
+        reset_shared();

         StartupPID = StartupDataBase();
         Assert(StartupPID != 0);
@@ -4962,7 +4961,7 @@ SubPostmasterMain(int argc, char *argv[])
         InitProcess();

         /* Attach process to shared data structures */
-        CreateSharedMemoryAndSemaphores(0);
+        CreateSharedMemoryAndSemaphores();

         /* And run the backend */
         BackendRun(&port);        /* does not return */
@@ -4976,7 +4975,7 @@ SubPostmasterMain(int argc, char *argv[])
         InitAuxiliaryProcess();

         /* Attach process to shared data structures */
-        CreateSharedMemoryAndSemaphores(0);
+        CreateSharedMemoryAndSemaphores();

         AuxiliaryProcessMain(argc - 2, argv + 2);    /* does not return */
     }
@@ -4989,7 +4988,7 @@ SubPostmasterMain(int argc, char *argv[])
         InitProcess();

         /* Attach process to shared data structures */
-        CreateSharedMemoryAndSemaphores(0);
+        CreateSharedMemoryAndSemaphores();

         AutoVacLauncherMain(argc - 2, argv + 2);    /* does not return */
     }
@@ -5002,7 +5001,7 @@ SubPostmasterMain(int argc, char *argv[])
         InitProcess();

         /* Attach process to shared data structures */
-        CreateSharedMemoryAndSemaphores(0);
+        CreateSharedMemoryAndSemaphores();

         AutoVacWorkerMain(argc - 2, argv + 2);    /* does not return */
     }
@@ -5020,7 +5019,7 @@ SubPostmasterMain(int argc, char *argv[])
         InitProcess();

         /* Attach process to shared data structures */
-        CreateSharedMemoryAndSemaphores(0);
+        CreateSharedMemoryAndSemaphores();

         /* 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 d7d7335..8853706 100644
--- a/src/backend/storage/ipc/ipci.c
+++ b/src/backend/storage/ipc/ipci.c
@@ -91,7 +91,7 @@ RequestAddinShmemSpace(Size size)
  * This is a bit code-wasteful and could be cleaned up.)
  */
 void
-CreateSharedMemoryAndSemaphores(int port)
+CreateSharedMemoryAndSemaphores(void)
 {
     PGShmemHeader *shim = NULL;

@@ -163,14 +163,14 @@ CreateSharedMemoryAndSemaphores(int port)
         /*
          * Create the shmem segment
          */
-        seghdr = PGSharedMemoryCreate(size, port, &shim);
+        seghdr = PGSharedMemoryCreate(size, &shim);

         InitShmemAccess(seghdr);

         /*
          * Create semaphores
          */
-        PGReserveSemaphores(numSemas, port);
+        PGReserveSemaphores(numSemas);

         /*
          * If spinlocks are disabled, initialize emulation layer (which
diff --git a/src/backend/utils/init/postinit.c b/src/backend/utils/init/postinit.c
index 43b9f17..29c5ec7 100644
--- a/src/backend/utils/init/postinit.c
+++ b/src/backend/utils/init/postinit.c
@@ -445,12 +445,10 @@ InitCommunication(void)
     if (!IsUnderPostmaster)        /* postmaster already did this */
     {
         /*
-         * 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.
+         * We're running a postgres bootstrap process or a standalone backend,
+         * so we need to set up shmem.
          */
-        CreateSharedMemoryAndSemaphores(PostPortNumber);
+        CreateSharedMemoryAndSemaphores();
     }
 }

diff --git a/src/include/storage/ipc.h b/src/include/storage/ipc.h
index e9b243f..d113813 100644
--- a/src/include/storage/ipc.h
+++ b/src/include/storage/ipc.h
@@ -76,6 +76,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(void);

 #endif                            /* IPC_H */
diff --git a/src/include/storage/pg_sema.h b/src/include/storage/pg_sema.h
index b95efa1..1ac4f9a 100644
--- a/src/include/storage/pg_sema.h
+++ b/src/include/storage/pg_sema.h
@@ -41,7 +41,7 @@ typedef HANDLE PGSemaphore;
 extern Size PGSemaphoreShmemSize(int maxSemas);

 /* Module initialization (called during postmaster start or shmem reinit) */
-extern void PGReserveSemaphores(int maxSemas, int port);
+extern void PGReserveSemaphores(int maxSemas);

 /* Allocate a PGSemaphore structure with initial count 1 */
 extern PGSemaphore PGSemaphoreCreate(void);
diff --git a/src/include/storage/pg_shmem.h b/src/include/storage/pg_shmem.h
index ac24878..6bd5664 100644
--- a/src/include/storage/pg_shmem.h
+++ b/src/include/storage/pg_shmem.h
@@ -82,7 +82,7 @@ extern void PGSharedMemoryReAttach(void);
 extern void PGSharedMemoryNoReAttach(void);
 #endif

-extern PGShmemHeader *PGSharedMemoryCreate(Size size, int port,
+extern PGShmemHeader *PGSharedMemoryCreate(Size size,
                                            PGShmemHeader **shim);
 extern bool PGSharedMemoryIsInUse(unsigned long id1, unsigned long id2);
 extern void PGSharedMemoryDetach(void);
diff --git a/src/test/recovery/t/017_shm.pl b/src/test/recovery/t/017_shm.pl
index 7f10ff5..be35f90 100644
--- a/src/test/recovery/t/017_shm.pl
+++ b/src/test/recovery/t/017_shm.pl
@@ -4,16 +4,18 @@
 use strict;
 use warnings;
 use Config;
+use File::stat qw(stat);
 use IPC::Run 'run';
+use IPC::SharedMem;
+use IPC::SysV qw(IPC_CREAT S_IRUSR S_IWUSR);
 use PostgresNode;
 use Test::More;
 use TestLib;
 use Time::HiRes qw(usleep);

-plan tests => 5;
+plan tests => 4;

 my $tempdir = TestLib::tempdir;
-my $port;

 # Log "ipcs" diffs on a best-effort basis, swallowing any error.
 my $ipcs_before = "$tempdir/ipcs_before";
@@ -25,71 +27,40 @@ sub log_ipcs
     return;
 }

-# These tests need a $port such that nothing creates or removes a segment in
-# $port's IpcMemoryKey range while this test script runs.  While there's no
-# way to ensure that in general, we do ensure that if PostgreSQL tests are the
-# only actors.  With TCP, the first get_new_node picks a port number.  With
-# Unix sockets, use a postmaster, $port_holder, to represent a key space
-# reservation.  $port_holder holds a reservation on the key space of port
-# 1+$port_holder->port if it created the first IpcMemoryKey of its own port's
-# key space.  If multiple copies of this test script run concurrently, they
-# will pick different ports.  $port_holder postmasters use odd-numbered ports,
-# and tests use even-numbered ports.  In the absence of collisions from other
-# shmget() activity, gnat starts with key 0x7d001 (512001), and flea starts
-# with key 0x7d002 (512002).
-my $port_holder;
-if (!$PostgresNode::use_tcp)
-{
-    my $lock_port;
-    for ($lock_port = 511; $lock_port < 711; $lock_port += 2)
-    {
-        $port_holder = PostgresNode->get_new_node(
-            "port${lock_port}_holder",
-            port     => $lock_port,
-            own_host => 1);
-        $port_holder->init;
-        $port_holder->append_conf('postgresql.conf', 'max_connections = 5');
-        $port_holder->start;
-        # Match the AddToDataDirLockFile() call in sysv_shmem.c.  Assume all
-        # systems not using sysv_shmem.c do use TCP.
-        my $shmem_key_line_prefix = sprintf("%9lu ", 1 + $lock_port * 1000);
-        last
-          if slurp_file($port_holder->data_dir . '/postmaster.pid') =~
-          /^$shmem_key_line_prefix/m;
-        $port_holder->stop;
-    }
-    $port = $lock_port + 1;
-}
-
 # Node setup.
-sub init_start
-{
-    my $name = shift;
-    my $ret = PostgresNode->get_new_node($name, port => $port, own_host => 1);
-    defined($port) or $port = $ret->port;    # same port for all nodes
-    $ret->init;
-    # Limit semaphore consumption, since we run several nodes concurrently.
-    $ret->append_conf('postgresql.conf', 'max_connections = 5');
-    $ret->start;
-    log_ipcs();
-    return $ret;
-}
-my $gnat = init_start 'gnat';
-my $flea = init_start 'flea';
+my $gnat = PostgresNode->get_new_node('gnat');
+$gnat->init;
+
+# Create a shmem segment that will conflict with gnat's first choice
+# of shmem key.
+my $gnat_dir_stat = stat($gnat->data_dir);
+defined($gnat_dir_stat) or die('unable to stat ' . $gnat->data_dir);
+my $gnat_inode = $gnat_dir_stat->ino;
+note "gnat's datadir inode = $gnat_inode";
+
+my $gnat_conflict_shm =
+  IPC::SharedMem->new($gnat_inode, 1024, IPC_CREAT | S_IRUSR | S_IWUSR);
+
+$gnat->start;
+log_ipcs();
+
+$gnat->restart;    # should keep same shmem key
+log_ipcs();

 # Upon postmaster death, postmaster children exit automatically.
 $gnat->kill9;
 log_ipcs();
-$flea->restart;       # flea ignores the shm key gnat abandoned.
-log_ipcs();
 poll_start($gnat);    # gnat recycles its former shm key.
 log_ipcs();

-# After clean shutdown, the nodes swap shm keys.
-$gnat->stop;
-$flea->restart;
+note "removing the conflicting shmem ...";
+$gnat_conflict_shm->remove;
 log_ipcs();
-$gnat->start;
+
+# Upon postmaster death, postmaster children exit automatically.
+$gnat->kill9;
+log_ipcs();
+poll_start($gnat);    # gnat will now use its normal shmem key.
 log_ipcs();

 # Scenarios involving no postmaster.pid, dead postmaster, and a live backend.
@@ -152,24 +123,18 @@ print STDERR $single_stderr;
 like($single_stderr, $pre_existing_msg,
     'single-user mode detected live backend via shared memory');
 log_ipcs();
-# Fail to reject startup if shm key N has become available and we crash while
-# using key N+1.  This is unwanted, but expected.  Windows is immune, because
-# its GetSharedMemName() use DataDir strings, not numeric keys.
-$flea->stop;    # release first key
-is( $gnat->start(fail_ok => 1),
-    $TestLib::windows_os ? 0 : 1,
-    'key turnover fools only sysv_shmem.c');
-$gnat->stop;     # release first key (no-op on $TestLib::windows_os)
-$flea->start;    # grab first key
-# cleanup
+
+# cleanup slow backend
 TestLib::system_log('pg_ctl', 'kill', 'QUIT', $slow_pid);
 $slow_client->finish;    # client has detected backend termination
 log_ipcs();
-poll_start($gnat);       # recycle second key

+# now startup should work
+poll_start($gnat);
+log_ipcs();
+
+# finish testing
 $gnat->stop;
-$flea->stop;
-$port_holder->stop if $port_holder;
 log_ipcs();