Re: dynamic shared memory

On Tue, Sep 24, 2013 at 12:19:51PM -0400, Robert Haas wrote:
> On Fri, Sep 20, 2013 at 7:44 AM, Andres Freund <andres@2ndquadrant.com> wrote:
> >> Actually, I was wondering if we ought
> >> to have a directory under pgdata whose explicit charter it was to
> >> contain files that shouldn't be copied as part of a base backup.
> >> pg_do_not_back_this_up.
> >
> > Wondered exactly about that as soon as you've mentioned
> > pg_basebackup. pg_local/?
> 
> That seems reasonable.  It's not totally transparent what that's
> supposed to mean, but it's fairly mnemonic once you know.  Other
> suggestions welcome, if anyone has ideas.

I like the concept and have no improvements on the name.

> Are there any other likely candidates for inclusion in that directory
> other than this stuff?

pg_xlog

> >> > Not sure whether it's sensible to only LOG in these cases. After all
> >> > there's something unexpected happening. The robustness argument doesn't
> >> > count since we're already shutting down.
> >
> >> I see no point in throwing an error.   The fact that we're having
> >> trouble cleaning up one dynamic shared memory segment doesn't mean we
> >> shouldn't try to clean up others, or that any remaining postmaster
> >> shutdown hooks shouldn't be executed.
> >
> > Well, it means we'll do a regular shutdown instead of PANICing
> > and *not* writing a checkpoint.
> > If something has corrupted our state to the point we cannot unregister
> > shared memory we registered, something has gone terribly wrong. Quite
> > possibly we've scribbled over our control structures or such. In that
> > case it's not proper to do a normal shutdown, we're quite possibly
> > writing bad data.
> 
> I have to admit I didn't consider the possibility of an
> otherwise-clean shutdown that hit only this problem.  I'm not sure how
> seriously to take that case.  I guess we could emit warnings for
> individual failures and then throw an error at the end if there were >
> 0, but that seems a little ugly.  Or we could go whole hog and treat
> any failure as a critical error.  Anyone else have an opinion on what
> to do here?

There's extensive precedent in our code for LOG, WARNING, or even ignoring the
return value of unlink().  (To my surprise, ignoring the return value is the
most popular choice.)  Of the dozens of backend callers, here is the mixed bag
that actually raises ERROR or better:

do_pg_stop_backup()
RestoreArchivedFile()
KeepFileRestoredFromArchive()
create_tablespace_directories() [remove old symlink during recovery]
destroy_tablespace_directories()
RelationCacheInitFilePreInvalidate()
CreateLockFile()

I think it's awfully unlikely that runaway code would corrupt shared_buffers
AND manage to make an unlink() fail.

> >> > Imo that's a PANIC or at the very least a FATAL.
> >>
> >> Sure, that's a tempting option, but it doesn't seem to serve any very
> >> necessary point.  There's no data corruption problem if we proceed
> >> here.  Most likely either (1) there's a bug in the code, which
> >> panicking won't fix or (2) the DBA hand-edited the state file, in
> >> which case maybe he shouldn't have done that, but if he thinks the
> >> best way to recover from that is a cluster-wide restart, he can do
> >> that himself.
> >
> > "There's no data corruption problem if we proceed" - but there likely
> > has been one leading to the current state.

+1 for making this one a PANIC, though.  With startup behind us, a valid dsm
state file pointed us to a control segment with bogus contents.  The
conditional probability of shared memory corruption seems higher than that of
a DBA editing the dsm state file of a running cluster to incorrectly name as
the dsm control segment some other existing shared memory segment.

Thanks,
nm

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