Обсуждение: pgsql: Consistently test for in-use shared memory.

Поиск
Список
Период
Сортировка

pgsql: Consistently test for in-use shared memory.

От
Noah Misch
Дата:
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

Branch
------
master

Details
-------
https://git.postgresql.org/pg/commitdiff/2f932f71d9f2963bbd201129d7b971c8f5f077fd

Modified Files
--------------
src/Makefile.global.in              |   4 +-
src/backend/port/sysv_shmem.c       | 255 ++++++++++++++++++++----------------
src/backend/port/win32_shmem.c      |   7 +-
src/backend/postmaster/postmaster.c |  12 +-
src/backend/storage/ipc/ipci.c      |  14 +-
src/backend/utils/init/postinit.c   |   6 +-
src/include/storage/ipc.h           |   2 +-
src/include/storage/pg_shmem.h      |   6 +-
src/test/perl/PostgresNode.pm       | 184 ++++++++++++++++++++------
src/test/recovery/t/017_shm.pl      | 154 ++++++++++++++++++++++
src/tools/msvc/vcregress.pl         |   1 +
11 files changed, 468 insertions(+), 177 deletions(-)


Re: pgsql: Consistently test for in-use shared memory.

От
Andres Freund
Дата:
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