Re: fork/exec patch

Claudio Natoli <claudio.natoli@memetrics.com> writes:
>> You should probably check the return value of unlink().
>
> No. This isn't necessary (and what action would it take in any
> case?).

It should write a log message. I'm not sure why this /shouldn't/ be
done: if an operation fails, we should log that failure. This is
standard practise.

>> Doesn't this function still acquire ShmemIndexLock? (i.e. why was
>> this comment changed?)
>
> AFAICS this is just whitespace differences.

Read it again. Here's the whole diff hunk:

*** 320,340 ****
      strncpy(item.key, name, SHMEM_INDEX_KEYSIZE);
      item.location = BAD_LOCATION;

!     LWLockAcquire(ShmemIndexLock, LW_EXCLUSIVE);

      if (!ShmemIndex)
      {
          /*
           * If the shmem index doesn't exist, we are bootstrapping: we must
           * be trying to init the shmem index itself.
           *
!          * Notice that the ShmemIndexLock is held until the shmem index has
           * been completely initialized.
           */
          Assert(strcmp(name, "ShmemIndex") == 0);
          Assert(ShmemBootstrap);
          *foundPtr = FALSE;
!         return ShmemAlloc(size);
      }

      /* look it up in the shmem index */
--- 335,367 ----
      strncpy(item.key, name, SHMEM_INDEX_KEYSIZE);
      item.location = BAD_LOCATION;

!     SpinLockAcquire(ShmemIndexLock);

      if (!ShmemIndex)
      {
+         if (IsUnderPostmaster)
+         {
+             /* Must be initializing a (non-standalone) backend */
+             Assert(strcmp(name, "ShmemIndex") == 0);
+             Assert(ShmemBootstrap);
+             Assert(ShmemIndexAlloc);
+             *foundPtr = TRUE;
+         }
+         else
+         {
              /*
               * If the shmem index doesn't exist, we are bootstrapping: we must
               * be trying to init the shmem index itself.
               *
!              * Notice that the ShmemLock is held until the shmem index has
               * been completely initialized.
               */
              Assert(strcmp(name, "ShmemIndex") == 0);
              Assert(ShmemBootstrap);
              *foundPtr = FALSE;
!             ShmemIndexAlloc = ShmemAlloc(size);
!         }
!         return ShmemIndexAlloc;
      }

The code acquires ShmemIndexLock, performs some computations, and then
notes that "ShmemLock is held" in a comment before returning. ISTM
that is plainly wrong.

> [ the only other suggested changes are ] stylistic/cosmetic and
> effect the EXEC_BACKEND code only.

Yeah, my apologies for nitpicking...

-Neil