Re: fork/exec patch

Claudio Natoli <claudio(dot)natoli(at)memetrics(dot)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