Re: Dynamic Shared Memory stuff

On Wed, Dec 18, 2013 at 12:21:08PM -0500, Robert Haas wrote:
> On Tue, Dec 10, 2013 at 6:26 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > The larger point is that such a shutdown process has never in the history
> > of Postgres been successful at removing shared-memory (or semaphore)
> > resources.  I do not feel a need to put a higher recoverability standard
> > onto the DSM code than what we've had for fifteen years with SysV shmem.
> >
> > But, by the same token, it shouldn't be any *less* recoverable.  In this
> > context that means that starting a new postmaster should recover the
> > resources, at least 90% of the time (100% if we still have the old
> > postmaster lockfile).  I think the idea of keeping enough info in the
> > SysV segment to permit recovery of DSM resources is a good one.  Then,
> > any case where the existing logic is able to free the old SysV segment
> > is guaranteed to also work for DSM.
> 
> So I'm taking a look at this.  There doesn't appear to be anything
> intrinsically intractable here, but it seems important to time the
> order of operations so as to minimize the chances that things fail in
> awkward places.  The point where we remove the old shared memory
> segment from the previous postmaster invocation is here:
> 
>         /*
>          * The segment appears to be from a dead Postgres process, or from a
>          * previous cycle of life in this same process.  Zap it, if possible.
>          * This probably shouldn't fail, but if it does, assume the segment
>          * belongs to someone else after all, and continue quietly.
>          */
>         shmdt(memAddress);
>         if (shmctl(shmid, IPC_RMID, NULL) < 0)
>             continue;
> 
> My first thought was to remember the control segment ID from the
> header just before the shmdt() there, and then later when the DSM
> module is starting, do cleanup.  But that doesn't seem like a good
> idea, because then if startup fails after we remove the old shared
> memory segment and before we get to the DSM initialization code, we'll
> have lost the information on what control segment needs to be cleaned
> up.  A subsequent startup attempt won't see the old shm again, because
> it's already gone.  I'm fairly sure that this would be a net reduction
> in reliability vs. the status quo.
> 
> So now what I'm thinking is that we ought to actually perform the DSM
> cleanup before detaching the old segment and trying to remove it.
> That shouldn't fail, but if it does, or if we get killed before
> completing it, the next run will hopefully find the same old shm and
> finish the cleanup.  But that kind of flies in the face of the comment
> above: if we perform DSM cleanup and then discover that the segment
> wasn't ours after all, that means we just stomped all over somebody
> else's stuff.  That's not too good.  But trying to remove the segment
> first and then perform the cleanup creates a window where, if we get
> killed, the next restart will have lost information about how to
> finish cleaning up.  So it seems that some kind of logic tweak is
> needed here, but I'm not sure exactly what.  As I see it, the options
> are:
> 
> 1. Make failure to remove the shared memory segment we thought was
> ours an error.  This will definitely show up any problems, but only
> after we've burned down some other processes's dynamic shared memory
> segments.  The most likely outcome is creating failure-to-start
> problems that don't exist today.
> 
> 2. Make it a warning.  We'll still burn down somebody else's DSMs, but
> at least we'll still start ourselves.  Sadly, "WARNING: You have just
> done a bad thing.  It's too late to fix it.  Sorry!" is not very
> appealing.

It has long been the responsibility of PGSharedMemoryCreate() to determine
that a segment is unimportant before calling IPC_RMID.  The success or failure
of IPC_RMID is an unreliable guide to the correctness of that determination.
IPC_RMID will succeed on an important segment owned by the same UID, and it
will fail if some other process removed the segment after our shmat().  Such a
failure does not impugn our having requested DSM cleanup on the basis of the
PGShmemHeader we did read, so an apologetic WARNING would be wrong.

> 3. Remove the old shared memory segment first, then perform the
> cleanup immediately afterwards.  If we get killed before completing
> the cleanup, we'll leak the un-cleaned-up stuff.  Decide that's OK and
> move on.

The period in which process exit would leak segments is narrow enough that
this seems fine.  I see no net advantage over performing the cleanup before
shmdt(), though.

> 4. Adopt some sort of belt-and-suspenders approach, keeping the state
> file we have now and backstopping it with this mechanism, so that we
> really only need this to work when $PGDATA has been blown away and
> recreated.  This seems pretty inelegant, and I'm not sure who it'll
> benefit other than those (few?) people who kill -9 the postmaster (or
> it segfaults or otherwise dies without running the code to remove
> shared memory segments) and then remove $PGDATA and then re-initdb and
> then expect cleanup to find the old DSMs.

Independent of the choice we make here, I would keep the state file.  POSIX
permits shm_open() segments to survive reboots, so the state file would come
into play after crashes on such systems.  Also, folks who use "ipcrm" after a
"kill -9" of the postmaster would benefit from the state file.  Nobody should
do that, but hey, catering to things nobody should do is what brought us here.

An option I had envisioned was to make PGSharedMemoryCreate() create a state
file based on the old sysv segment.  Then the later dsm_postmaster_startup()
would proceed normally, and a restart after a failure between shmdt() and
dsm_postmaster_startup() would also do the cleanup.  A wrinkle comes from the
fact that an existing state file could reference a different control segment;
this would happen if we picked a different sysv shm key compared to the last
postmaster start.  In that case, we should presumably clean both control
segments.  Performing the cleanup per #1/#2 achieves that.

> 5. Give up on this approach.  We could keep what we have now, or make
> the DSM control segment land at a well-known address as we do for the
> main segment.

How would having the DSM control segment at a well-known address affect the
problem at hand?  Did you mean a well-known dsm_handle?

> What do people prefer?

I recommend performing cleanup on the control segment named in PGShmemHeader
just before shmdt() in PGSharedMemoryCreate().  No new ERROR or WARNING sites
are necessary.  Have dsm_postmaster_startup() continue to perform a cleanup on
the control segment named in the state file.

> The other thing I'm a bit squidgy about is injecting more code that
> runs before we get the new shared memory segment up and running.
> During that time, we don't have effective mutual exclusion, so it's
> possible that someone else could be trying to do cleanup at the same
> time we are.  That should be OK as far as the DSM code itself is
> concerned, but there may be other downsides to lengthening the period
> of time that's required to get mutual exclusion in place that I should
> be worrying about.

I see no general need to avoid small increases to that time period.

Thanks,
nm

-- 
Noah Misch
EnterpriseDB                                 http://www.enterprisedb.com