Re: [PATCH] Make ENOSPC not fatal in semaphore creation

On Sun, Oct 17, 2021 at 10:52:38AM -0400, Tom Lane wrote:
> Mikhail <mp39590(at)gmail(dot)com> writes:
> > On Sun, Oct 17, 2021 at 10:29:24AM -0400, Tom Lane wrote:
> >> AFAICS, this patch could be disastrous. What if the semaphore in
> >> question belongs to some other postmaster?
>
> > Does running more than one postmaster on the same PGDATA is supported at
> > all? Currently seed for the semaphore key is inode number of PGDATA.
>
> That hardly guarantees no collisions. If it did, we'd never have bothered
> with the PGSemaMagic business or the IpcSemaphoreGetLastPID check.

Got it, makes sense. Also, I was presented with examples that inode
number can be reused across mounting points for different clusters.

> >> Also, you haven't explained why the existing (and much safer) recycling
> >> logic in IpcSemaphoreCreate doesn't solve your problem.
>
> > semget() returns ENOSPC, so InternalIpcSemaphoreCreate doesn't return -1
> > so the whole logic of IpcSemaphoreCreate is not checked.
>
> Hmm. Maybe you could improve this by removing the first
> InternalIpcSemaphoreCreate call in IpcSemaphoreCreate, and
> rearranging the logic so that the first step consists of seeing
> whether a sema set is already there (and can safely be zapped),
> and only then proceed with creation.

I think, I can look into this on the next weekend. On first glance the
solution works for me.

> I am, however, concerned that this'll just trade off one hazard for
> another. Instead of a risk of failing with ENOSPC (which the DBA
> can fix), we'll have a risk of kneecapping some other process at
> random (which the DBA can do nothing to prevent).

Good argument, but I'll try to make second version of the patch with the
proposed logic change to see what we will get. I think it's "right"
behavior to recycle our own used semaphores, so the whole approach is
correct.

> I'm also fairly unclear on when the logic you propose would trigger
> at all. If the sema set is already there, I'd expect EEXIST or
> equivalent, not ENOSPC.

The logic works - the initial call to semget() in
InternalIpcSemaphoreCreate returns -1 and errno is set to ENOSPC - I
tested the patch on OpenBSD 7.0, it successfully recycles sem's after
previous "pkill -6 postgres". Verified it with 'ipcs -s'.