Don't treat EINVAL from semget() as a hard failure.

It turns out that on some platforms (at least current macOS, NetBSD,
OpenBSD) semget(2) will return EINVAL if there is a pre-existing
semaphore set with the same key and too few semaphores.  Our code
expects EEXIST in that case and treats EINVAL as a hard failure,
resulting in failure during initdb or postmaster start.

POSIX does document EINVAL for too-few-semaphores-in-set, and is
silent on its priority relative to EEXIST, so this behavior arguably
conforms to spec.  Nonetheless it's quite problematic because EINVAL
is also documented to mean that nsems is greater than the system's
limit on the number of semaphores per set (SEMMSL).  If that is
where the problem lies, retrying would just become an infinite loop.

To resolve this contradiction, retry after EINVAL, but also install a
loop limit that will make us give up regardless of the specific errno
after trying 1000 different keys.  (1000 is a pretty arbitrary number,
but it seems like it should be sufficient.)  I like this better than
the previous infinite-looping behavior, since it will also keep us out
of trouble if (say) we get EACCES due to a system-level permissions
problem rather than anything to do with a specific semaphore set.

This problem has only been observed in the field in PG 17, which uses
a higher nsems value than other branches (cf. 38da05346, 810a8b1c8).
That makes it possible to get the failure if a new v17 postmaster
has a key collision with an existing postmaster of another branch.
In principle though, we might see such a collision against a semaphore
set created by some other application, in which case all branches are
vulnerable on these platforms.  Hence, backpatch.

Reported-by: Gavin Panella <gavinpanella@gmail.com>
Author: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/CALL7chmzY3eXHA7zHnODUVGZLSvK3wYCSP0RmcDFHJY8f28Q3g@mail.gmail.com
Backpatch-through: 13

diff --git a/src/backend/port/sysv_sema.c b/src/backend/port/sysv_sema.c
index 0e9c35994ab159012f32c37632cab03c8d61cbcf..f12d8848e378c6e915012324f1aa60857ae28da9 100644 (file)
--- a/src/backend/port/sysv_sema.c
+++ b/src/backend/port/sysv_sema.c
@@ -69,7 +69,7 @@ static int    nextSemaNumber;     /* next free sem num in last sema set */
 
 
 static IpcSemaphoreId InternalIpcSemaphoreCreate(IpcSemaphoreKey semKey,
-                                                int numSems);
+                                                int numSems, bool retry_ok);
 static void IpcSemaphoreInitialize(IpcSemaphoreId semId, int semNum,
                                   int value);
 static void IpcSemaphoreKill(IpcSemaphoreId semId);
@@ -88,9 +88,13 @@ static void ReleaseSemaphores(int status, Datum arg);
  * If we fail with a failure code other than collision-with-existing-set,
  * print out an error and abort.  Other types of errors suggest nonrecoverable
  * problems.
+ *
+ * Unfortunately, it's sometimes hard to tell whether errors are
+ * nonrecoverable.  Our caller keeps track of whether continuing to retry
+ * is sane or not; if not, we abort on failure regardless of the errno.
  */
 static IpcSemaphoreId
-InternalIpcSemaphoreCreate(IpcSemaphoreKey semKey, int numSems)
+InternalIpcSemaphoreCreate(IpcSemaphoreKey semKey, int numSems, bool retry_ok)
 {
    int         semId;
 
@@ -101,16 +105,27 @@ InternalIpcSemaphoreCreate(IpcSemaphoreKey semKey, int numSems)
        int         saved_errno = errno;
 
        /*
-        * Fail quietly if error indicates a collision with existing set. One
-        * would expect EEXIST, given that we said IPC_EXCL, but perhaps we
-        * could get a permission violation instead?  Also, EIDRM might occur
-        * if an old set is slated for destruction but not gone yet.
+        * Fail quietly if error suggests a collision with an existing set and
+        * our caller has not lost patience.
+        *
+        * One would expect EEXIST, given that we said IPC_EXCL, but perhaps
+        * we could get a permission violation instead.  On some platforms
+        * EINVAL will be reported if the existing set has too few semaphores.
+        * Also, EIDRM might occur if an old set is slated for destruction but
+        * not gone yet.
+        *
+        * EINVAL is the key reason why we need the caller-level loop limit,
+        * as it can also mean that the platform's SEMMSL is less than
+        * numSems, and that condition can't be fixed by trying another key.
         */
-       if (saved_errno == EEXIST || saved_errno == EACCES
+       if (retry_ok &&
+           (saved_errno == EEXIST
+            || saved_errno == EACCES
+            || saved_errno == EINVAL
 #ifdef EIDRM
-           || saved_errno == EIDRM
+            || saved_errno == EIDRM
 #endif
-           )
+            ))
            return -1;
 
        /*
@@ -207,17 +222,22 @@ IpcSemaphoreGetLastPID(IpcSemaphoreId semId, int semNum)
 static IpcSemaphoreId
 IpcSemaphoreCreate(int numSems)
 {
+   int         num_tries = 0;
    IpcSemaphoreId semId;
    union semun semun;
    PGSemaphoreData mysema;
 
    /* Loop till we find a free IPC key */
-   for (nextSemaKey++;; nextSemaKey++)
+   for (nextSemaKey++;; nextSemaKey++, num_tries++)
    {
        pid_t       creatorPID;
 
-       /* Try to create new semaphore set */
-       semId = InternalIpcSemaphoreCreate(nextSemaKey, numSems + 1);
+       /*
+        * Try to create new semaphore set.  Give up after trying 1000
+        * distinct IPC keys.
+        */
+       semId = InternalIpcSemaphoreCreate(nextSemaKey, numSems + 1,
+                                          num_tries < 1000);
        if (semId >= 0)
            break;              /* successful create */
 
@@ -254,7 +274,7 @@ IpcSemaphoreCreate(int numSems)
        /*
         * Now try again to create the sema set.
         */
-       semId = InternalIpcSemaphoreCreate(nextSemaKey, numSems + 1);
+       semId = InternalIpcSemaphoreCreate(nextSemaKey, numSems + 1, true);
        if (semId >= 0)
            break;              /* successful create */