Date: Wed, 3 Apr 2019 19:00:57 -0700
From: Andres Freund <andres@anarazel.de>
To: Noah Misch <noah@leadboat.com>
Cc: pgsql-committers@lists.postgresql.org
Subject: Re: pgsql: Consistently test for in-use shared memory.
Message-ID: <20190404020057.galelv7by75ekqrh@alap3.anarazel.de>
References: <E1hBq3r-0002Dz-CX@gemulon.postgresql.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <E1hBq3r-0002Dz-CX@gemulon.postgresql.org>
Precedence: bulk

Hi,

On 2019-04-04 00:16:55 +0000, Noah Misch wrote:
> Consistently test for in-use shared memory.
> 
> postmaster startup scrutinizes any shared memory segment recorded in
> postmaster.pid, exiting if that segment matches the current data
> directory and has an attached process.  When the postmaster.pid file was
> missing, a starting postmaster used weaker checks.  Change to use the
> same checks in both scenarios.  This increases the chance of a startup
> failure, in lieu of data corruption, if the DBA does "kill -9 `head -n1
> postmaster.pid` && rm postmaster.pid && pg_ctl -w start".  A postmaster
> will no longer recycle segments pertaining to other data directories.
> That's good for production, but it's bad for integration tests that
> crash a postmaster and immediately delete its data directory.  Such a
> test now leaks a segment indefinitely.  No "make check-world" test does
> that.  win32_shmem.c already avoided all these problems.  In 9.6 and
> later, enhance PostgresNode to facilitate testing.  Back-patch to 9.4
> (all supported versions).
> 
> Reviewed by Daniel Gustafsson and Kyotaro HORIGUCHI.
> 
> Discussion: https://postgr.es/m/20130911033341.GD225735@tornado.leadboat.com

My compiler now nitpicks:

In file included from /home/andres/src/postgresql/src/include/postgres.h:47,
                 from pg_shmem.c:20:
pg_shmem.c: In function ‘PGSharedMemoryCreate’:
/home/andres/src/postgresql/src/include/utils/elog.h:122:5: warning: this statement may fall through [-Wimplicit-fallthrough=]
  do { \
     ^
/home/andres/src/postgresql/src/include/utils/elog.h:142:2: note: in expansion of macro ‘ereport_domain’
  ereport_domain(elevel, TEXTDOMAIN, rest)
  ^~~~~~~~~~~~~~
pg_shmem.c:668:5: note: in expansion of macro ‘ereport’
     ereport(FATAL,
     ^~~~~~~
pg_shmem.c:675:4: note: here
    case SHMSTATE_ENOENT:
    ^~~~
All of PostgreSQL successfully made. Ready to install.

I think the SHMSTATE_ATTACHED case simply should grow a (unreachable)
break. But I'd also strongly suggest adding one to SHMSTATE_UNATTACHED -
because that's actually reachable, and just seems like a trap for person
adding another case:.

Greetings,

Andres Freund