Horton

Thread

Commits

  1. Probe only 127.0.0.1 when looking for ports on Unix.

  2. Don't write to stdin of a test process that could have already exited.

  3. Test both 0.0.0.0 and 127.0.0.x addresses to find a usable port.

  4. MSYS: Translate REGRESS_SHLIB to a Windows file name.

  5. MSYS: Skip src/test/recovery/t/017_shm.pl.

  6. When Perl "kill(9, ...)" fails, try "pg_ctl kill".

  7. Consistently test for in-use shared memory.

  8. Revert "Consistently test for in-use shared memory."

  9. Silence -Wimplicit-fallthrough in sysv_shmem.c.

  10. Make src/test/recovery/t/017_shm.pl safe for concurrent execution.

  11. Update HINT for pre-existing shared memory block.

  12. Add WL_EXIT_ON_PM_DEATH pseudo-event.

  13. The default values for shared_buffers and max_connections are now 1000

  14. XLOG (and related) changes:

  15. Significant cleanups in SysV IPC handling (shared mem and semaphores).

  1. Weaker shmem interlock w/o postmaster.pid

    Noah Misch <noah@leadboat.com> — 2013-09-11T03:33:41Z

    If a starting postmaster's CreateLockFile() finds an existing postmaster.pid,
it subjects the shared memory segment named therein to the careful scrutiny of
PGSharedMemoryIsInUse().  If that segment matches the current data directory
and has any attached processes, we bail with the "pre-existing shared memory
block ... is still in use" error.  When the postmaster.pid file is missing,
there's inherently less we can do to reliably detect this situation; in
particular, an old postmaster could have chosen an unusual key due to the
usual 1+(port*1000) key being in use.  That being said, PGSharedMemoryCreate()
typically will stumble upon the old segment, and it (its sysv variant, anyway)
applies checks much weaker than those of PGSharedMemoryIsInUse().  If the
segment has a PGShmemHeader and the postmaster PID named in that header is not
alive, PGSharedMemoryCreate() will delete the segment and proceed.  Shouldn't
it instead check the same things as PGSharedMemoryIsInUse()?

The concrete situation in which I encountered this involved PostgreSQL 9.2 and
an immediate shutdown with a backend that had blocked SIGQUIT.  The backend
survived the immediate shutdown as one would expect.  The postmaster
nonetheless removed postmaster.pid before exiting, and I could immediately
restart PostgreSQL despite the survival of the SIGQUIT-blocked backend.  If I
instead SIGKILL the postmaster, postmaster.pid remains, and I must kill stray
backends before restarting.  The postmaster should not remove postmaster.pid
unless it has verified that its children have exited.  Concretely, that means
not removing postmaster.pid on immediate shutdown in 9.3 and earlier.  That's
consistent with the rough nature of an immediate shutdown, anyway.

I'm thinking to preserve postmaster.pid at immediate shutdown in all released
versions, but I'm less sure about back-patching a change to make
PGSharedMemoryCreate() pickier.  On the one hand, allowing startup to proceed
with backends still active in the same data directory is a corruption hazard.
On the other hand, it could break weird shutdown/restart patterns that permit
trivial lifespan overlap between backends of different postmasters.  Opinions?

Thanks,
nm

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

  2. Re: Weaker shmem interlock w/o postmaster.pid

    Stephen Frost <sfrost@snowman.net> — 2013-09-11T15:32:01Z

    * Noah Misch (noah@leadboat.com) wrote:
> Shouldn't it instead check the same things as PGSharedMemoryIsInUse()?

Offhand, I tend to agree that we should really be doing a very careful
job of looking at if an existing segment is still in use.

> The concrete situation in which I encountered this involved PostgreSQL 9.2 and
> an immediate shutdown with a backend that had blocked SIGQUIT.  The backend
> survived the immediate shutdown as one would expect.  

Well..  We expect this now because of the analysis you did in the
adjacent thread showing how it can happen.

> The postmaster
> nonetheless removed postmaster.pid before exiting, and I could immediately
> restart PostgreSQL despite the survival of the SIGQUIT-blocked backend.  If I
> instead SIGKILL the postmaster, postmaster.pid remains, and I must kill stray
> backends before restarting.  The postmaster should not remove postmaster.pid
> unless it has verified that its children have exited.

This makes sense, however..

> Concretely, that means
> not removing postmaster.pid on immediate shutdown in 9.3 and earlier.  That's
> consistent with the rough nature of an immediate shutdown, anyway.

I don't like leaving the postmaster.pid file around, even on an
immediate shutdown.  I don't have any great suggestions regarding what
to do, given what we try to do wrt 'immediate', so perhaps it's
acceptable for future releases.

> I'm thinking to preserve postmaster.pid at immediate shutdown in all released
> versions, but I'm less sure about back-patching a change to make
> PGSharedMemoryCreate() pickier.  On the one hand, allowing startup to proceed
> with backends still active in the same data directory is a corruption hazard.

The corruption risk, imv anyway, is sufficient to backpatch the change
and overrides the concerns around very fast shutdown/restarts.

	Thanks,

		Stephen

  3. Re: Weaker shmem interlock w/o postmaster.pid

    Robert Haas <robertmhaas@gmail.com> — 2013-09-11T18:10:45Z

    On Tue, Sep 10, 2013 at 11:33 PM, Noah Misch <noah@leadboat.com> wrote:
> I'm thinking to preserve postmaster.pid at immediate shutdown in all released
> versions, but I'm less sure about back-patching a change to make
> PGSharedMemoryCreate() pickier.  On the one hand, allowing startup to proceed
> with backends still active in the same data directory is a corruption hazard.
> On the other hand, it could break weird shutdown/restart patterns that permit
> trivial lifespan overlap between backends of different postmasters.  Opinions?

I'm more sanguine about the second change than the first.  Leaving
postmaster.pid around seems like a clear user-visible behavior change
that could break user scripts or have other consequences that we don't
foresee; thus, I would vote against back-patching it.  Indeed, I'm not
sure it's a good idea to do that even in master.  On the other hand,
tightening the checks in PGSharedMemoryCreate() seems very much worth
doing, and I think it might also be safe enough to back-patch.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  4. Re: Weaker shmem interlock w/o postmaster.pid

    Noah Misch <noah@leadboat.com> — 2013-09-12T02:28:00Z

    On Wed, Sep 11, 2013 at 11:32:01AM -0400, Stephen Frost wrote:
> * Noah Misch (noah@leadboat.com) wrote:
> > The concrete situation in which I encountered this involved PostgreSQL 9.2 and
> > an immediate shutdown with a backend that had blocked SIGQUIT.  The backend
> > survived the immediate shutdown as one would expect.  
> 
> Well..  We expect this now because of the analysis you did in the
> adjacent thread showing how it can happen.

That was a surprising way for it to happen, but there have long been known
vectors like a SIGSTOP'd backend or a backend that has blocked SIGQUIT.

> > Concretely, that means
> > not removing postmaster.pid on immediate shutdown in 9.3 and earlier.  That's
> > consistent with the rough nature of an immediate shutdown, anyway.
> 
> I don't like leaving the postmaster.pid file around, even on an
> immediate shutdown.  I don't have any great suggestions regarding what
> to do, given what we try to do wrt 'immediate', so perhaps it's
> acceptable for future releases.

Fair enough.

> > I'm thinking to preserve postmaster.pid at immediate shutdown in all released
> > versions, but I'm less sure about back-patching a change to make
> > PGSharedMemoryCreate() pickier.  On the one hand, allowing startup to proceed
> > with backends still active in the same data directory is a corruption hazard.
> 
> The corruption risk, imv anyway, is sufficient to backpatch the change
> and overrides the concerns around very fast shutdown/restarts.

Making PGSharedMemoryCreate() pickier in all branches will greatly diminish
the marginal value of preserving postmaster.pid, so I'm fine with dropping the
postmaster.pid side of the proposal.

Thanks,
nm

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

  5. Re: Weaker shmem interlock w/o postmaster.pid

    David Johnston <polobo@yahoo.com> — 2013-09-12T02:57:22Z

    Noah Misch-2 wrote
>> > I'm thinking to preserve postmaster.pid at immediate shutdown in all
>> released
>> > versions, but I'm less sure about back-patching a change to make
>> > PGSharedMemoryCreate() pickier.  On the one hand, allowing startup to
>> proceed
>> > with backends still active in the same data directory is a corruption
>> hazard.
>> 
>> The corruption risk, imv anyway, is sufficient to backpatch the change
>> and overrides the concerns around very fast shutdown/restarts.
> 
> Making PGSharedMemoryCreate() pickier in all branches will greatly
> diminish
> the marginal value of preserving postmaster.pid, so I'm fine with dropping
> the
> postmaster.pid side of the proposal.

Its probably still worth a fresh look at the immediate shutdown process to
see whether the current location where postmaster.pid is removed is
acceptable.  It may not be necessary to leave it in place always but:

1) if there is a section of shared memory that can only be reached/found if
one knows the pid, and
2) postmaster.pid is removed before that area is "secured from future
clobbering"

then there may be a risk that can still be mitigated by moving its removal
without having to go to the extreme.  

David J.




--
View this message in context: http://postgresql.1045698.n5.nabble.com/Weaker-shmem-interlock-w-o-postmaster-pid-tp5770399p5770559.html
Sent from the PostgreSQL - hackers mailing list archive at Nabble.com.

  6. Re: Weaker shmem interlock w/o postmaster.pid

    Noah Misch <noah@leadboat.com> — 2013-09-12T03:25:20Z

    On Wed, Sep 11, 2013 at 07:57:22PM -0700, David Johnston wrote:
> Noah Misch-2 wrote
> > Making PGSharedMemoryCreate() pickier in all branches will greatly
> > diminish
> > the marginal value of preserving postmaster.pid, so I'm fine with dropping
> > the
> > postmaster.pid side of the proposal.
> 
> Its probably still worth a fresh look at the immediate shutdown process to
> see whether the current location where postmaster.pid is removed is
> acceptable.  It may not be necessary to leave it in place always but:
> 
> 1) if there is a section of shared memory that can only be reached/found if
> one knows the pid, and

Similar: one needs a sysv shared memory key to find the segment, and
postmaster.pid records that key.  The chosen key is almost always the same
from run to run, so a new postmaster typically does find the old segment even
without postmaster.pid.

> 2) postmaster.pid is removed before that area is "secured from future
> clobbering"

Clobbering shared memory is not the actual threat here.  We use the shared
memory segment as a witness to the fact that PostgreSQL processes are active
in a data directory.  If we let children of different postmasters operate in
the same directory simultaneously, they can corrupt data.

> then there may be a risk that can still be mitigated by moving its removal
> without having to go to the extreme.  

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

  7. Re: Weaker shmem interlock w/o postmaster.pid

    Bruce Momjian <bruce@momjian.us> — 2014-02-12T23:06:40Z

    On Wed, Sep 11, 2013 at 02:10:45PM -0400, Robert Haas wrote:
> On Tue, Sep 10, 2013 at 11:33 PM, Noah Misch <noah@leadboat.com> wrote:
> > I'm thinking to preserve postmaster.pid at immediate shutdown in all released
> > versions, but I'm less sure about back-patching a change to make
> > PGSharedMemoryCreate() pickier.  On the one hand, allowing startup to proceed
> > with backends still active in the same data directory is a corruption hazard.
> > On the other hand, it could break weird shutdown/restart patterns that permit
> > trivial lifespan overlap between backends of different postmasters.  Opinions?
> 
> I'm more sanguine about the second change than the first.  Leaving
> postmaster.pid around seems like a clear user-visible behavior change
> that could break user scripts or have other consequences that we don't
> foresee; thus, I would vote against back-patching it.  Indeed, I'm not
> sure it's a good idea to do that even in master.  On the other hand,
> tightening the checks in PGSharedMemoryCreate() seems very much worth
> doing, and I think it might also be safe enough to back-patch.

Were these changes every applied?  I don't see them.

-- 
  Bruce Momjian  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

  + Everyone has their own god. +

  8. Re: Weaker shmem interlock w/o postmaster.pid

    Noah Misch <noah@leadboat.com> — 2014-02-13T14:02:24Z

    On Wed, Feb 12, 2014 at 06:06:40PM -0500, Bruce Momjian wrote:
> On Wed, Sep 11, 2013 at 02:10:45PM -0400, Robert Haas wrote:
> > On Tue, Sep 10, 2013 at 11:33 PM, Noah Misch <noah@leadboat.com> wrote:
> > > I'm thinking to preserve postmaster.pid at immediate shutdown in all released
> > > versions, but I'm less sure about back-patching a change to make
> > > PGSharedMemoryCreate() pickier.  On the one hand, allowing startup to proceed
> > > with backends still active in the same data directory is a corruption hazard.
> > > On the other hand, it could break weird shutdown/restart patterns that permit
> > > trivial lifespan overlap between backends of different postmasters.  Opinions?
> > 
> > I'm more sanguine about the second change than the first.  Leaving
> > postmaster.pid around seems like a clear user-visible behavior change
> > that could break user scripts or have other consequences that we don't
> > foresee; thus, I would vote against back-patching it.  Indeed, I'm not
> > sure it's a good idea to do that even in master.  On the other hand,
> > tightening the checks in PGSharedMemoryCreate() seems very much worth
> > doing, and I think it might also be safe enough to back-patch.
> 
> Were these changes every applied?  I don't see them.

No, I haven't gotten around to writing them.

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

  9. Re: [HACKERS] Weaker shmem interlock w/o postmaster.pid

    Noah Misch <noah@leadboat.com> — 2018-08-12T06:48:15Z

    On Wed, Sep 11, 2013 at 10:28:00PM -0400, Noah Misch wrote:
> On Wed, Sep 11, 2013 at 11:32:01AM -0400, Stephen Frost wrote:
> > * Noah Misch (noah@leadboat.com) wrote:
> > > Concretely, that means
> > > not removing postmaster.pid on immediate shutdown in 9.3 and earlier.  That's
> > > consistent with the rough nature of an immediate shutdown, anyway.

With 9.3 having a few months left, that's less interesting, but ...

> > I don't like leaving the postmaster.pid file around, even on an
> > immediate shutdown.  I don't have any great suggestions regarding what
> > to do, given what we try to do wrt 'immediate', so perhaps it's
> > acceptable for future releases.
> 
> Fair enough.

... we could still worry about folks doing "kill -9 `head -n1 postmaster.pid`
&& rm postmaster.pid".  A possible defense is to record a copy of the shm
identifiers in a second file ("sysv_shmem_key"?) within the data directory.
Since users would have no expectations about a new file's lifecycle, we could
remove it when and only when we verify a segment doesn't exist.  However, a
DBA determined to remove postmaster.pid would likely remove both files.

> > > I'm thinking to preserve postmaster.pid at immediate shutdown in all released
> > > versions, but I'm less sure about back-patching a change to make
> > > PGSharedMemoryCreate() pickier.  On the one hand, allowing startup to proceed
> > > with backends still active in the same data directory is a corruption hazard.
> > 
> > The corruption risk, imv anyway, is sufficient to backpatch the change
> > and overrides the concerns around very fast shutdown/restarts.
> 
> Making PGSharedMemoryCreate() pickier in all branches will greatly diminish
> the marginal value of preserving postmaster.pid, so I'm fine with dropping the
> postmaster.pid side of the proposal.

Here's an implementation.  The first, small patch replaces an obsolete
errhint() about manually removing shared memory blocks.  In its place,
recommend killing processes.  Today's text, added in commit 4d14fe0, is too
rarely pertinent to justify a HINT.  (You might use ipcrm if a PostgreSQL
process is stuck in D state and has SIGKILL pending, so it will never become
runnable again.)  Removal of the ipcclean script ten years ago (39627b1) and
its non-replacement corroborate that manual shm removal is now a niche goal.

In the main patch, one of the tests covers an unsafe case that sysv_shmem.c
still doesn't detect.  I could pursue a fix via the aforementioned
sysv_shmem_key file, modulo the possibility of a DBA removing it.  I could
also, when postmaster.pid is missing, make sysv_shmem.c check the first N
(N=100?) keys applicable to the selected port.  My gut feeling is that neither
thing is worthwhile, but I'm interested to hear other opinions.

This removes the creatorPID test, which commit c715fde added before commit
4d14fe0 added the shm_nattch test.  The pair of tests is not materially
stronger than the shm_nattch test by itself.  For reasons explained in the
comment at "enum IpcMemoryState", this patch has us cease recycling segments
pertaining to data directories other than our own.  This isn't ideal for the
buildfarm, where a postmaster crash bug would permanently leak a shm segment.
I think the benefits for production use cases eclipse this drawback.  Did I
miss a reason to like the old behavior?

Single-user mode has been protected even less than normal execution, because
it selected a shm key as though using port zero.  Thus, starting single-user
mode after an incomplete shutdown would never detect the pre-shutdown shm.
The patch makes single-user mode work like normal execution.  Until commit
de98a7e, single-user mode used malloc(), not a shm segment.  That commit also
restricted single-user mode to not recycle old segments.  I didn't find a
rationale for that restriction, but a contributing factor may have been the
fact that every single-user process uses the same port-0 shm key space.  Also,
initdb starts various single-user backends, and reaping stale segments would
be an odd side effect for initdb.  With single-user mode using a real port
number and PostgreSQL no longer recycling segments of other data directories,
I removed that restriction.  By the way, as of this writing, my regular
development system has 41 stale segments, all in the single-user key space
(0x00000001 to 0x0000002d with gaps).  It's clear how they persisted once
leaked, but I don't know how I leaked them in the first place.

I ran 9327 iterations of the test file, and it did not leak a shm segment in
that time.  I tested on Red Hat and on Windows Server 2016; I won't be shocked
if the test (not the code under test) breaks on other Windows configurations.
The patch adds PostgresNode features to enable that test coverage.

The comment referring to a CreateDataDirLockFile() bug refers to this one:
https://postgr.es/m/flat/20120803145635.GE9683%40tornado.leadboat.com

Thanks,
nm

  10. Re: [HACKERS] Weaker shmem interlock w/o postmaster.pid

    Dmitry Dolgov <9erthalion6@gmail.com> — 2018-11-29T12:10:57Z

    > On Sun, Aug 12, 2018 at 8:48 AM Noah Misch <noah@leadboat.com> wrote:
>
> On Wed, Sep 11, 2013 at 10:28:00PM -0400, Noah Misch wrote:
> > On Wed, Sep 11, 2013 at 11:32:01AM -0400, Stephen Frost wrote:
> > > * Noah Misch (noah@leadboat.com) wrote:
> > > > Concretely, that means
> > > > not removing postmaster.pid on immediate shutdown in 9.3 and earlier.  That's
> > > > consistent with the rough nature of an immediate shutdown, anyway.
>
> With 9.3 having a few months left, that's less interesting, but ...

Thanks for the patch!

I'm a bit out of context, but taking into account that 9.3 is already beyond
EOL, is it still interesting? I'm just thinking about whether it's worth to
move it to the next CF, or mark as rejected, since no one reviewed the patch so
far?

As a side note, with this patch recovery tests are failing now on 016_shm.pl

#   Failed test 'detected live backend via shared memory'
#   at t/016_shm.pl line 87.
#                   '2018-11-28 13:08:08.504 UTC [21924] LOG:
listening on Unix socket "/tmp/yV2oDNcG8e/gnat/.s.PGSQL.512"
# 2018-11-28 13:08:08.512 UTC [21925] LOG:  database system was
interrupted; last known up at 2018-11-28 13:08:08 UTC
# 2018-11-28 13:08:08.512 UTC [21925] LOG:  database system was not
properly shut down; automatic recovery in progress
# 2018-11-28 13:08:08.512 UTC [21925] LOG:  invalid record length at
0/165FEF8: wanted 24, got 0
# 2018-11-28 13:08:08.512 UTC [21925] LOG:  redo is not required
# 2018-11-28 13:08:08.516 UTC [21924] LOG:  database system is ready
to accept connections
# '
#     doesn't match '(?^:pre-existing shared memory block)'

  11. Re: [HACKERS] Weaker shmem interlock w/o postmaster.pid

    Noah Misch <noah@leadboat.com> — 2018-11-30T06:51:40Z

    On Thu, Nov 29, 2018 at 01:10:57PM +0100, Dmitry Dolgov wrote:
> On Sun, Aug 12, 2018 at 8:48 AM Noah Misch <noah@leadboat.com> wrote:
> > With 9.3 having a few months left, that's less interesting, but ...

> I'm a bit out of context, but taking into account that 9.3 is already beyond
> EOL, is it still interesting?

Yes.

> As a side note, with this patch recovery tests are failing now on 016_shm.pl
> 
> #   Failed test 'detected live backend via shared memory'
> #   at t/016_shm.pl line 87.
> #                   '2018-11-28 13:08:08.504 UTC [21924] LOG:
> listening on Unix socket "/tmp/yV2oDNcG8e/gnat/.s.PGSQL.512"
> # 2018-11-28 13:08:08.512 UTC [21925] LOG:  database system was
> interrupted; last known up at 2018-11-28 13:08:08 UTC
> # 2018-11-28 13:08:08.512 UTC [21925] LOG:  database system was not
> properly shut down; automatic recovery in progress
> # 2018-11-28 13:08:08.512 UTC [21925] LOG:  invalid record length at
> 0/165FEF8: wanted 24, got 0
> # 2018-11-28 13:08:08.512 UTC [21925] LOG:  redo is not required
> # 2018-11-28 13:08:08.516 UTC [21924] LOG:  database system is ready
> to accept connections
> # '
> #     doesn't match '(?^:pre-existing shared memory block)'

Thanks for the report.  Since commit cfdf4dc made pg_sleep() react to
postmaster death, the test will need a different way to stall a backend.  This
doesn't affect non-test code, and the test still passes against cfdf4dc^ and
against REL_11_STABLE.  I've queued a task to update the test code, but review
can proceed in parallel.

  12. Re: [HACKERS] Weaker shmem interlock w/o postmaster.pid

    Noah Misch <noah@leadboat.com> — 2018-12-03T00:41:06Z

    On Thu, Nov 29, 2018 at 10:51:40PM -0800, Noah Misch wrote:
> On Thu, Nov 29, 2018 at 01:10:57PM +0100, Dmitry Dolgov wrote:
> > As a side note, with this patch recovery tests are failing now on 016_shm.pl
> > 
> > #   Failed test 'detected live backend via shared memory'
> > #   at t/016_shm.pl line 87.
> > #                   '2018-11-28 13:08:08.504 UTC [21924] LOG:
> > listening on Unix socket "/tmp/yV2oDNcG8e/gnat/.s.PGSQL.512"
> > # 2018-11-28 13:08:08.512 UTC [21925] LOG:  database system was
> > interrupted; last known up at 2018-11-28 13:08:08 UTC
> > # 2018-11-28 13:08:08.512 UTC [21925] LOG:  database system was not
> > properly shut down; automatic recovery in progress
> > # 2018-11-28 13:08:08.512 UTC [21925] LOG:  invalid record length at
> > 0/165FEF8: wanted 24, got 0
> > # 2018-11-28 13:08:08.512 UTC [21925] LOG:  redo is not required
> > # 2018-11-28 13:08:08.516 UTC [21924] LOG:  database system is ready
> > to accept connections
> > # '
> > #     doesn't match '(?^:pre-existing shared memory block)'
> 
> Thanks for the report.  Since commit cfdf4dc made pg_sleep() react to
> postmaster death, the test will need a different way to stall a backend.  This
> doesn't affect non-test code, and the test still passes against cfdf4dc^ and
> against REL_11_STABLE.  I've queued a task to update the test code, but review
> can proceed in parallel.

Here's a version fixing that test for post-cfdf4dc backends.

  13. Re: [HACKERS] Weaker shmem interlock w/o postmaster.pid

    Michael Paquier <michael@paquier.xyz> — 2019-02-02T01:18:34Z

    On Sun, Dec 02, 2018 at 04:41:06PM -0800, Noah Misch wrote:
> Here's a version fixing that test for post-cfdf4dc backends.

This patch set still applies and needs review, so moved to next CF.
--
Michael

  14. Re: [HACKERS] Weaker shmem interlock w/o postmaster.pid

    Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> — 2019-03-04T09:04:20Z

    Hello.

At Sun, 2 Dec 2018 16:41:06 -0800, Noah Misch <noah@leadboat.com> wrote in <20181203004106.GA2860387@rfd.leadboat.com>
> On Thu, Nov 29, 2018 at 10:51:40PM -0800, Noah Misch wrote:
> > On Thu, Nov 29, 2018 at 01:10:57PM +0100, Dmitry Dolgov wrote:
> > > As a side note, with this patch recovery tests are failing now on 016_shm.pl
> > > 
> > > #   Failed test 'detected live backend via shared memory'
> > > #   at t/016_shm.pl line 87.
> > > #                   '2018-11-28 13:08:08.504 UTC [21924] LOG:
> > > listening on Unix socket "/tmp/yV2oDNcG8e/gnat/.s.PGSQL.512"
> > > # 2018-11-28 13:08:08.512 UTC [21925] LOG:  database system was
> > > interrupted; last known up at 2018-11-28 13:08:08 UTC
> > > # 2018-11-28 13:08:08.512 UTC [21925] LOG:  database system was not
> > > properly shut down; automatic recovery in progress
> > > # 2018-11-28 13:08:08.512 UTC [21925] LOG:  invalid record length at
> > > 0/165FEF8: wanted 24, got 0
> > > # 2018-11-28 13:08:08.512 UTC [21925] LOG:  redo is not required
> > > # 2018-11-28 13:08:08.516 UTC [21924] LOG:  database system is ready
> > > to accept connections
> > > # '
> > > #     doesn't match '(?^:pre-existing shared memory block)'
> > 
> > Thanks for the report.  Since commit cfdf4dc made pg_sleep() react to
> > postmaster death, the test will need a different way to stall a backend.  This
> > doesn't affect non-test code, and the test still passes against cfdf4dc^ and
> > against REL_11_STABLE.  I've queued a task to update the test code, but review
> > can proceed in parallel.

I found that I have 65(h) segments left alone on my environment:p

At Sat, 11 Aug 2018 23:48:15 -0700, Noah Misch <noah@leadboat.com> wrote in <20180812064815.GB2301738@rfd.leadboat.com>
> still doesn't detect.  I could pursue a fix via the aforementioned
> sysv_shmem_key file, modulo the possibility of a DBA removing it.  I could
> also, when postmaster.pid is missing, make sysv_shmem.c check the first N
> (N=100?) keys applicable to the selected port.  My gut feeling is that neither
> thing is worthwhile, but I'm interested to hear other opinions.

# Though I don't get the meaning of the "modulo" there..

I think the only thing we must avoid here is sharing the same
shmem segment with a living-dead server. If we can do that
without the pid file, it would be better than relying on it. We
could remove orphaned segments automatically, but I don't think
we should do that going so far as relying on a dedicated
file. Also, I don't think it's worth stopping shmem id scanning
at a certain number since I don't come up with an appropriate
number for it. But looking "port * 1000", it might be expected
that a usable segment id will found while scanning that number of
ids (1000).

> Here's a version fixing that test for post-cfdf4dc backends.

This moves what were in PGSharedMmoeryIsInUse into a new function
IpcMemoryAnalyze for reusing then adds distinction among
EEXISTS/FOREIGN in not-in-use cases and ATTACHED/ANALYSIS_FAILURE
in an in-use case. UNATTACHED is changed to a not-in-use case by
this patch.  As the result PGSharedMemoryIsInUse() is changed so
that it reutrns "not-in-use" for the UNATTACHED case. It looks
fine.

PGSharedMemoryCreate changed to choose a usable shmem id using
the IpcMemoryAnalyze().  But some of the statuses from
IpcMemoryAnalyze() is concealed by failure of
PGSharedMemoryAttach() and ignored silently opposed to what the
code is intending to do. (By the way SHMSTATE_EEXISTS seems
suggesting oppsite thing from EEXIST, which would be confusing.)

PGSharedMemoryCreate() repeats shmat/shmdt twice in every
iteration. It won't harm so much but it would be better if we
could get rid of that.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  15. Re: [HACKERS] Weaker shmem interlock w/o postmaster.pid

    Noah Misch <noah@leadboat.com> — 2019-03-07T03:24:22Z

    On Mon, Mar 04, 2019 at 06:04:20PM +0900, Kyotaro HORIGUCHI wrote:
> I found that I have 65(h) segments left alone on my environment:p

Did patched PostgreSQL create those, or did unpatched PostgreSQL create them?

> At Sat, 11 Aug 2018 23:48:15 -0700, Noah Misch <noah@leadboat.com> wrote in <20180812064815.GB2301738@rfd.leadboat.com>
> > still doesn't detect.  I could pursue a fix via the aforementioned
> > sysv_shmem_key file, modulo the possibility of a DBA removing it.  I could
> > also, when postmaster.pid is missing, make sysv_shmem.c check the first N
> > (N=100?) keys applicable to the selected port.  My gut feeling is that neither
> > thing is worthwhile, but I'm interested to hear other opinions.
> 
> # Though I don't get the meaning of the "modulo" there..

It wasn't clear.  Adding a separate sysv_shmem_key file would not protect a
cluster if the DBA does "rm -f postmaster.pid sysv_shmem_key".  It would,
however, fix this test case:

+# Fail to reject startup if shm key N has become available and we crash while
+# using key N+1.  This is unwanted, but expected.  Windows is immune, because
+# its GetSharedMemName() use DataDir strings, not numeric keys.
+$flea->stop;    # release first key
+is( $gnat->start(fail_ok => 1),
+       $TestLib::windows_os ? 0 : 1,
+       'key turnover fools only sysv_shmem.c');

> I think the only thing we must avoid here is sharing the same
> shmem segment with a living-dead server. If we can do that
> without the pid file, it would be better than relying on it. We
> could remove orphaned segments automatically, but I don't think
> we should do that going so far as relying on a dedicated
> file.

There are two things to avoid.  First, during postmaster startup, avoid
sharing a segment with any other process.  Second, avoid sharing a data
directory with a running PostgreSQL process that was a child of some other
postmaster.  I think that's what you mean by "living-dead server".  The first
case is already prevented thoroughly, because we only proceed with startup
after creating a new segment with IPC_CREAT | IPC_EXCL.  The second case is
what this patch seeks to improve.

> Also, I don't think it's worth stopping shmem id scanning
> at a certain number since I don't come up with an appropriate
> number for it. But looking "port * 1000", it might be expected
> that a usable segment id will found while scanning that number of
> ids (1000).

If we find thousands of unusable segments, we do keep going until we find a
usable segment.  I was referring to a different situation.  Today, if there's
no postmaster.pid file and we're using port 5432, we first try segment ID
5432000.  If segment ID 5432000 is usable, we use it without examining any
other segment ID.  If segment ID 5432010 were in use by a "living-dead
server", we won't discover that fact.  We could increase the chance of
discovering that fact by checking every segment ID from 5432000 to 5432999.
(Once finished with that scan, we'd still create and use 5432000.)  I didn't
implement that idea in the patch, but I shared it to see if anyone thought it
was worth adding.

> PGSharedMemoryCreate changed to choose a usable shmem id using
> the IpcMemoryAnalyze().  But some of the statuses from
> IpcMemoryAnalyze() is concealed by failure of
> PGSharedMemoryAttach() and ignored silently opposed to what the
> code is intending to do.

SHMSTATE_FOREIGN is ignored silently.  The patch modifies the
PGSharedMemoryAttach() header comment to direct callers to treat
PGSharedMemoryAttach() failure like SHMSTATE_FOREIGN.  I don't think the code
had an unintentional outcome due to the situation you describe.  Perhaps I
could have made the situation clearer.

> (By the way SHMSTATE_EEXISTS seems
> suggesting oppsite thing from EEXIST, which would be confusing.)

Good catch.  Is SHMSTATE_ENOENT better?

> PGSharedMemoryCreate() repeats shmat/shmdt twice in every
> iteration. It won't harm so much but it would be better if we
> could get rid of that.

I'll try to achieve that and see if the code turns out cleaner.

Thanks,
nm

  16. Re: [HACKERS] Weaker shmem interlock w/o postmaster.pid

    Noah Misch <noah@leadboat.com> — 2019-03-09T07:16:10Z

    On Wed, Mar 06, 2019 at 07:24:22PM -0800, Noah Misch wrote:
> On Mon, Mar 04, 2019 at 06:04:20PM +0900, Kyotaro HORIGUCHI wrote:
> > PGSharedMemoryCreate changed to choose a usable shmem id using
> > the IpcMemoryAnalyze().  But some of the statuses from
> > IpcMemoryAnalyze() is concealed by failure of
> > PGSharedMemoryAttach() and ignored silently opposed to what the
> > code is intending to do.
> 
> SHMSTATE_FOREIGN is ignored silently.  The patch modifies the
> PGSharedMemoryAttach() header comment to direct callers to treat
> PGSharedMemoryAttach() failure like SHMSTATE_FOREIGN.  I don't think the code
> had an unintentional outcome due to the situation you describe.  Perhaps I
> could have made the situation clearer.

I think v3, attached, avoids this appearance of unintended behavior.

> > (By the way SHMSTATE_EEXISTS seems
> > suggesting oppsite thing from EEXIST, which would be confusing.)
> 
> Good catch.  Is SHMSTATE_ENOENT better?

I did s/SHMSTATE_EEXISTS/SHMSTATE_ENOENT/.

> > PGSharedMemoryCreate() repeats shmat/shmdt twice in every
> > iteration. It won't harm so much but it would be better if we
> > could get rid of that.
> 
> I'll try to achieve that and see if the code turns out cleaner.

I renamed IpcMemoryAnalyze() to PGSharedMemoryAttach() and deleted the old
function of that name.  Now, this function never calls shmdt(); the caller is
responsible for that.  I do like things better this way.  What do you think?

  17. Re: [HACKERS] Weaker shmem interlock w/o postmaster.pid

    Daniel Gustafsson <daniel@yesql.se> — 2019-03-29T09:53:51Z

    On Saturday, March 9, 2019 8:16 AM, Noah Misch <noah@leadboat.com> wrote:

This patch is not really in my wheelhouse, so I might very well be testing it
in the wrong way, but whats the fun in staying in shallow end. Below is my
attempt at reviewing this patchset.

Both patches applies with a little bit of fuzz, and passes check-world. No new
perlcritic warnings are introduced, but 016_shm.pl needs to be renamed to 017
since commit b0825d28ea83e44139bd319e6d1db2c499c claimed 016. They absolutely
seem to do what they on the tin, and to my understanding this seems like an
improvement we definitely want.

> I renamed IpcMemoryAnalyze() to PGSharedMemoryAttach() and deleted the old
> function of that name. Now, this function never calls shmdt(); the caller is
> responsible for that. I do like things better this way. What do you think?

I think it makes for a good API for the caller to be responsible, but it does
warrant a comment on the function to explicitly state that.

A few other small comments:

+   state = PGSharedMemoryAttach((IpcMemoryId) id2, &memAddress);
+   if (memAddress)
+       shmdt(memAddress);

This seems like a case where it would be useful to log a shmdt() error or do
an Assert() around the success of the operation perhaps?

+    * Loop till we find a free IPC key.  Trust CreateDataDirLockFile() to
+    * ensure no more than one postmaster per data directory can enter this
+    * loop simultaneously.  (CreateDataDirLockFile() does not ensure that,
+    * but prefer fixing it over coping here.)

This comment make it seem like there is a fix to CreateLockFile() missing to
his patch, is that correct? If so, do you have an idea for that patch?

> I tested on Red Hat and on Windows Server 2016; I won't be shocked
> if the test (not the code under test) breaks on other Windows configurations.

IIRC there are Windows versions where Win32::Process::KillProcess is required
for this, but thats admittedly somewhat dated knowledge. If the buildfarm goes
red on older Windows animals it might be something to look at perhaps.

Switching this to Ready for Committer since I can't see anything but tiny things.

cheers ./daniel

  18. Re: [HACKERS] Weaker shmem interlock w/o postmaster.pid

    Noah Misch <noah@leadboat.com> — 2019-03-31T22:42:33Z

    On Fri, Mar 29, 2019 at 09:53:51AM +0000, Daniel Gustafsson wrote:
> On Saturday, March 9, 2019 8:16 AM, Noah Misch <noah@leadboat.com> wrote:
> > I renamed IpcMemoryAnalyze() to PGSharedMemoryAttach() and deleted the old
> > function of that name. Now, this function never calls shmdt(); the caller is
> > responsible for that. I do like things better this way. What do you think?
> 
> I think it makes for a good API for the caller to be responsible, but it does
> warrant a comment on the function to explicitly state that.

The name "PGSharedMemoryAttach" makes that fact sufficiently obvious, I think.

> A few other small comments:
> 
> +   state = PGSharedMemoryAttach((IpcMemoryId) id2, &memAddress);
> +   if (memAddress)
> +       shmdt(memAddress);
> 
> This seems like a case where it would be useful to log a shmdt() error or do
> an Assert() around the success of the operation perhaps?

I'll add the same elog(LOG) we have at other shmdt() sites.  I can't think of
a site where we Assert() about the results of a system call.  While shmdt()
might be a justified exception, elog(LOG) seems reasonable.

> +    * Loop till we find a free IPC key.  Trust CreateDataDirLockFile() to
> +    * ensure no more than one postmaster per data directory can enter this
> +    * loop simultaneously.  (CreateDataDirLockFile() does not ensure that,
> +    * but prefer fixing it over coping here.)
> 
> This comment make it seem like there is a fix to CreateLockFile() missing to
> his patch, is that correct? If so, do you have an idea for that patch?

That comment refers to
https://postgr.es/m/flat/20120803145635.GE9683%40tornado.leadboat.com

> Switching this to Ready for Committer since I can't see anything but tiny things.

Thanks.

  19. Re: [HACKERS] Weaker shmem interlock w/o postmaster.pid

    Daniel Gustafsson <daniel@yesql.se> — 2019-04-01T08:19:56Z

    On Monday, April 1, 2019 12:42 AM, Noah Misch <noah@leadboat.com> wrote:
> On Fri, Mar 29, 2019 at 09:53:51AM +0000, Daniel Gustafsson wrote:

> > This seems like a case where it would be useful to log a shmdt() error or do
> > an Assert() around the success of the operation perhaps?
>
> I'll add the same elog(LOG) we have at other shmdt() sites. I can't think of
> a site where we Assert() about the results of a system call. While shmdt()
> might be a justified exception, elog(LOG) seems reasonable.

Agreed, seems reasonable.

> > -   -   Loop till we find a free IPC key. Trust CreateDataDirLockFile() to
> > -   -   ensure no more than one postmaster per data directory can enter this
> > -   -   loop simultaneously. (CreateDataDirLockFile() does not ensure that,
> > -   -   but prefer fixing it over coping here.)
> >
> > This comment make it seem like there is a fix to CreateLockFile() missing to
> > his patch, is that correct? If so, do you have an idea for that patch?
>
> That comment refers to
> https://postgr.es/m/flat/20120803145635.GE9683%40tornado.leadboat.com

Gotcha, thanks for the clarification.

cheers ./daniel

  20. Re: [HACKERS] Weaker shmem interlock w/o postmaster.pid

    Noah Misch <noah@leadboat.com> — 2019-04-04T02:05:43Z

    On Mon, Apr 01, 2019 at 08:19:56AM +0000, Daniel Gustafsson wrote:
> On Monday, April 1, 2019 12:42 AM, Noah Misch <noah@leadboat.com> wrote:
> > On Fri, Mar 29, 2019 at 09:53:51AM +0000, Daniel Gustafsson wrote:
> 
> > > This seems like a case where it would be useful to log a shmdt() error or do
> > > an Assert() around the success of the operation perhaps?
> >
> > I'll add the same elog(LOG) we have at other shmdt() sites. I can't think of
> > a site where we Assert() about the results of a system call. While shmdt()
> > might be a justified exception, elog(LOG) seems reasonable.
> 
> Agreed, seems reasonable.

Pushed, but that broke two buildfarm members:
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=idiacanthus&dt=2019-04-04%2000%3A33%3A14
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=komodoensis&dt=2019-04-04%2000%3A33%3A13

I think the problem arose because these animals run on the same machine, and
their test execution was synchronized to the second.  Two copies of the new
test ran concurrently.  It doesn't tolerate that, owing to expectations about
which shared memory keys are in use.  My initial thought is to fix this by
having a third postmaster that runs throughout the test and represents
ownership of a given port.  If that postmaster gets something other than the
first shm key pertaining to its port, switch ports and try again.

I'll also include fixes for the warnings Andres reported on the
pgsql-committers thread.

  21. Re: [HACKERS] Weaker shmem interlock w/o postmaster.pid

    Noah Misch <noah@leadboat.com> — 2019-04-04T14:53:19Z

    On Wed, Apr 03, 2019 at 07:05:43PM -0700, Noah Misch wrote:
> On Mon, Apr 01, 2019 at 08:19:56AM +0000, Daniel Gustafsson wrote:
> > On Monday, April 1, 2019 12:42 AM, Noah Misch <noah@leadboat.com> wrote:
> > > On Fri, Mar 29, 2019 at 09:53:51AM +0000, Daniel Gustafsson wrote:
> > 
> > > > This seems like a case where it would be useful to log a shmdt() error or do
> > > > an Assert() around the success of the operation perhaps?
> > >
> > > I'll add the same elog(LOG) we have at other shmdt() sites. I can't think of
> > > a site where we Assert() about the results of a system call. While shmdt()
> > > might be a justified exception, elog(LOG) seems reasonable.
> > 
> > Agreed, seems reasonable.
> 
> Pushed, but that broke two buildfarm members:
> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=idiacanthus&dt=2019-04-04%2000%3A33%3A14
> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=komodoensis&dt=2019-04-04%2000%3A33%3A13
> 
> I think the problem arose because these animals run on the same machine, and
> their test execution was synchronized to the second.  Two copies of the new
> test ran concurrently.  It doesn't tolerate that, owing to expectations about
> which shared memory keys are in use.  My initial thought is to fix this by
> having a third postmaster that runs throughout the test and represents
> ownership of a given port.  If that postmaster gets something other than the
> first shm key pertaining to its port, switch ports and try again.
> 
> I'll also include fixes for the warnings Andres reported on the
> pgsql-committers thread.

This thread's 2019-04-03 patches still break buildfarm members in multiple
ways.  I plan to revert them.  I'll wait a day or two before doing that, in
case more failure types show up.

  22. Re: [HACKERS] Weaker shmem interlock w/o postmaster.pid

    Noah Misch <noah@leadboat.com> — 2019-04-08T06:41:41Z

    On Thu, Apr 04, 2019 at 07:53:19AM -0700, Noah Misch wrote:
> On Wed, Apr 03, 2019 at 07:05:43PM -0700, Noah Misch wrote:
> > Pushed, but that broke two buildfarm members:
> > https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=idiacanthus&dt=2019-04-04%2000%3A33%3A14
> > https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=komodoensis&dt=2019-04-04%2000%3A33%3A13
> > 
> > I think the problem arose because these animals run on the same machine, and
> > their test execution was synchronized to the second.  Two copies of the new
> > test ran concurrently.  It doesn't tolerate that, owing to expectations about
> > which shared memory keys are in use.  My initial thought is to fix this by
> > having a third postmaster that runs throughout the test and represents
> > ownership of a given port.  If that postmaster gets something other than the
> > first shm key pertaining to its port, switch ports and try again.
> > 
> > I'll also include fixes for the warnings Andres reported on the
> > pgsql-committers thread.
> 
> This thread's 2019-04-03 patches still break buildfarm members in multiple
> ways.  I plan to revert them.  I'll wait a day or two before doing that, in
> case more failure types show up.

Notable classes of buildfarm failure:

- AIX animals failed two ways.  First, I missed a "use" statement such that
  poll_start() would fail if it needed more than one attempt.  Second, I
  assumed $pid would be gone as soon as kill(9, $pid) returned[1].
- komodoensis and idiacanthus failed due to 16ee6ea not fully resolving the
  problems with concurrent execution.  I reproduced the various concurrency
  bugs by setting up four vpath build trees and looping the one test in each:
    for dir in 0 1 2 3; do (until [ -f /tmp/stopprove ]; do make -C $dir/src/test/recovery installcheck PROVE_TESTS=t/017_shm.pl; done) & done; wait; rm /tmp/stopprove
- elver failed due to semaphore exhaustion.  I'm reducing max_connections.
- lorikeet's FailedAssertion("!(vmq->mq_sender == ((void *)0))" looked
  suspicious, but this happened six other times in the past year[2], always on
  v10 lorikeet.
- Commit 0aa0ccf, a wrong back-patch, saw 100% failure of the new test.

While it didn't cause a buildfarm failure, I'm changing the non-test code to
treat shmat() EACCESS as SHMSTATE_FOREIGN, so we ignore that key and move to
another.  In the previous version, I treated it as SHMSTATE_ANALYSIS_FAILURE
and blocked startup.  In HEAD today, shmat() failure blocks startup if and
only if we got the shmid from postmaster.pid; there's no distinction between
EACCES and other causes.

Attached v4 fixes all the above.  I've also attached the incremental diff
versus the code I reverted.


[1] POSIX says "sig or at least one pending unblocked signal shall be
delivered to the sending thread before kill() returns."  I doubt the
postmaster had another signal pending often enough to explain the failures, so
AIX probably doesn't follow POSIX in this respect.

[2] All examples in the last year:
 sysname  │     stage      │    branch     │      snapshot       │                                            url
──────────┼────────────────┼───────────────┼─────────────────────┼───────────────────────────────────────────────────────────────────────────────────────────
 lorikeet │ InstallCheck-C │ REL_10_STABLE │ 2018-05-04 09:49:55 │ https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=lorikeet&dt=2018-05-04%2009:49:55
 lorikeet │ InstallCheck-C │ REL_10_STABLE │ 2018-05-05 13:15:24 │ https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=lorikeet&dt=2018-05-05%2013:15:24
 lorikeet │ InstallCheck-C │ REL_10_STABLE │ 2018-05-06 09:33:35 │ https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=lorikeet&dt=2018-05-06%2009:33:35
 lorikeet │ InstallCheck-C │ REL_10_STABLE │ 2018-05-15 20:52:36 │ https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=lorikeet&dt=2018-05-15%2020:52:36
 lorikeet │ Check          │ REL_10_STABLE │ 2019-02-20 10:40:40 │ https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=lorikeet&dt=2019-02-20%2010:40:40
 lorikeet │ InstallCheck-C │ REL_10_STABLE │ 2019-03-06 09:31:24 │ https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=lorikeet&dt=2019-03-06%2009:31:24
 lorikeet │ Check          │ REL_10_STABLE │ 2019-04-04 09:47:02 │ https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=lorikeet&dt=2019-04-04%2009:47:02

  23. Re: [HACKERS] Weaker shmem interlock w/o postmaster.pid

    Thomas Munro <thomas.munro@gmail.com> — 2019-04-08T08:07:28Z

    On Mon, Apr 8, 2019 at 6:42 PM Noah Misch <noah@leadboat.com> wrote:
> - lorikeet's FailedAssertion("!(vmq->mq_sender == ((void *)0))" looked
>   suspicious, but this happened six other times in the past year[2], always on
>   v10 lorikeet.

It happens on v11 too:

https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=lorikeet&dt=2018-09-25%2010%3A06%3A31

The text changed slightly because we dropped an unnecessary extra
pointer-to-volatile:

FailedAssertion("!(mq->mq_sender == ((void *)0))"

So either two workers started with the same parallel worker number, or
something unexpectedly overwrote the shm_mq struct?

-- 
Thomas Munro
https://enterprisedb.com

  24. Re: [HACKERS] Weaker shmem interlock w/o postmaster.pid

    Noah Misch <noah@leadboat.com> — 2019-04-08T14:48:04Z

    On Mon, Apr 08, 2019 at 08:07:28PM +1200, Thomas Munro wrote:
> On Mon, Apr 8, 2019 at 6:42 PM Noah Misch <noah@leadboat.com> wrote:
> > - lorikeet's FailedAssertion("!(vmq->mq_sender == ((void *)0))" looked
> >   suspicious, but this happened six other times in the past year[2], always on
> >   v10 lorikeet.
> 
> It happens on v11 too:
> 
> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=lorikeet&dt=2018-09-25%2010%3A06%3A31
> 
> The text changed slightly because we dropped an unnecessary extra
> pointer-to-volatile:
> 
> FailedAssertion("!(mq->mq_sender == ((void *)0))"
> 
> So either two workers started with the same parallel worker number, or
> something unexpectedly overwrote the shm_mq struct?

Ahh.  In that case, it's a duplicate of
https://postgr.es/m/flat/136712b0-0619-5619-4634-0f0286acaef7%402ndQuadrant.com

  25. Re: [HACKERS] Weaker shmem interlock w/o postmaster.pid

    Thomas Munro <thomas.munro@gmail.com> — 2019-04-11T00:48:35Z

    On Mon, Apr 8, 2019 at 6:42 PM Noah Misch <noah@leadboat.com> wrote:
> - AIX animals failed two ways.  First, I missed a "use" statement such that
>   poll_start() would fail if it needed more than one attempt.  Second, I
>   assumed $pid would be gone as soon as kill(9, $pid) returned[1].

> [1] POSIX says "sig or at least one pending unblocked signal shall be
> delivered to the sending thread before kill() returns."  I doubt the
> postmaster had another signal pending often enough to explain the failures, so
> AIX probably doesn't follow POSIX in this respect.

It looks like you fixed this, but I was curious about this obversation
as someone interested in learning more about kernel stuff and
portability... Maybe I misunderstood, but isn't POSIX referring to
kill(sig, $YOUR_OWN_PID) there?  That is, if you signal *yourself*,
and no other thread exists that could handle the signal, it will be
handled by the sending thread, and in the case of SIGKILL it will
therefore never return.  But here, you were talking about a perl
script that kills the postmaster, no?  If so, that passage doesn't
seem to apply.  In any case, regardless of whether the signal handler
has run to completion when kill() returns, doesn't the pid have to
continue to exist in the process table until it is reaped by its
parent (possibly in response to SIGCHLD), with one of the wait*()
family of system calls?

-- 
Thomas Munro
https://enterprisedb.com

  26. Re: [HACKERS] Weaker shmem interlock w/o postmaster.pid

    Noah Misch <noah@leadboat.com> — 2019-04-11T06:22:28Z

    On Thu, Apr 11, 2019 at 12:48:35PM +1200, Thomas Munro wrote:
> On Mon, Apr 8, 2019 at 6:42 PM Noah Misch <noah@leadboat.com> wrote:
> > - AIX animals failed two ways.  First, I missed a "use" statement such that
> >   poll_start() would fail if it needed more than one attempt.  Second, I
> >   assumed $pid would be gone as soon as kill(9, $pid) returned[1].
> 
> > [1] POSIX says "sig or at least one pending unblocked signal shall be
> > delivered to the sending thread before kill() returns."  I doubt the
> > postmaster had another signal pending often enough to explain the failures, so
> > AIX probably doesn't follow POSIX in this respect.
> 
> It looks like you fixed this, but I was curious about this obversation
> as someone interested in learning more about kernel stuff and
> portability... Maybe I misunderstood, but isn't POSIX referring to
> kill(sig, $YOUR_OWN_PID) there?  That is, if you signal *yourself*,
> and no other thread exists that could handle the signal, it will be
> handled by the sending thread, and in the case of SIGKILL it will
> therefore never return.  But here, you were talking about a perl
> script that kills the postmaster, no?  If so, that passage doesn't
> seem to apply.

You're right.  I revoke the footnote.

> In any case, regardless of whether the signal handler
> has run to completion when kill() returns, doesn't the pid have to
> continue to exist in the process table until it is reaped by its
> parent (possibly in response to SIGCHLD), with one of the wait*()
> family of system calls?

True.  I'll add that to the code comment.

  27. Re: [HACKERS] Weaker shmem interlock w/o postmaster.pid

    Thomas Munro <thomas.munro@gmail.com> — 2019-04-18T04:30:46Z

    On Thu, Apr 11, 2019 at 6:22 PM Noah Misch <noah@leadboat.com> wrote:
>  -                       my $iaddr = inet_aton($test_localhost);
> +                       my $iaddr = inet_aton('0.0.0.0');

This causes make check-world to deliver a flurry of pop-ups from
macOS's built-in Firewall asking if perl should be allowed to listen
to all interfaces (well I didn't catch the exact message, but that's
probably the drift).  Not sure if they'd go away permanently if I
managed to click OK before they disappear, but it's fun trying.  The
silly firewall facility is not actually enabled by default on this OS,
but unfortunately this company-issued machine has it forced to on.
This isn't really an objection to the code, it's more of a bemused
anecdote about a computer that can't decide whether it's a Unix
workstation or a Fisher Price My First Computer.

-- 
Thomas Munro
https://enterprisedb.com

  28. Re: [HACKERS] Weaker shmem interlock w/o postmaster.pid

    Noah Misch <noah@leadboat.com> — 2019-04-21T16:59:03Z

    On Thu, Apr 18, 2019 at 04:30:46PM +1200, Thomas Munro wrote:
> On Thu, Apr 11, 2019 at 6:22 PM Noah Misch <noah@leadboat.com> wrote:
> >  -                       my $iaddr = inet_aton($test_localhost);
> > +                       my $iaddr = inet_aton('0.0.0.0');
> 
> This causes make check-world to deliver a flurry of pop-ups from
> macOS's built-in Firewall asking if perl should be allowed to listen
> to all interfaces (well I didn't catch the exact message, but that's
> probably the drift).  Not sure if they'd go away permanently if I
> managed to click OK before they disappear, but it's fun trying.  The
> silly firewall facility is not actually enabled by default on this OS,
> but unfortunately this company-issued machine has it forced to on.
> This isn't really an objection to the code, it's more of a bemused
> anecdote about a computer that can't decide whether it's a Unix
> workstation or a Fisher Price My First Computer.

That is unfortunate.  The "Allowing specific applications" section of
https://support.apple.com/en-us/HT201642 appears to offer a way to allow perl
permanently.  Separately, it wouldn't cost much for us to abandon that check
on !$use_tcp (non-Windows) configurations.

  29. Re: [HACKERS] Weaker shmem interlock w/o postmaster.pid

    Noah Misch <noah@leadboat.com> — 2019-04-28T17:00:11Z

    On Fri, Mar 29, 2019 at 09:53:51AM +0000, Daniel Gustafsson wrote:
> On Saturday, March 9, 2019 8:16 AM, Noah Misch <noah@leadboat.com> wrote:
> > I tested on Red Hat and on Windows Server 2016; I won't be shocked
> > if the test (not the code under test) breaks on other Windows configurations.
> 
> IIRC there are Windows versions where Win32::Process::KillProcess is required
> for this, but thats admittedly somewhat dated knowledge. If the buildfarm goes
> red on older Windows animals it might be something to look at perhaps.

Since my second attempt at committing this (commit c098509), I fixed these
bugs in the new test file:

- MSYS-orchestrated mingw32 (buildfarm member jacana) failed the Perl kill(9,
  ...) calls[1].  For HEAD and v11, using "pg_ctl kill KILL <pid>" fixed that.
  For v10 and v9.6, I disabled the test under msys.  I can reproduce this with
  Perl 5.28 from msys2.  Its kill(9, ...) fails for any non-msys2 process (any
  ordinary Windows process).  [commits 947a350, 2bc0474]

- The regress.dll path needed MSYS-to-w32 path translation.  [commit 9daefff]

- The changes to port selection in get_new_node() made Linux-specific
  assumptions, so Windows builds had much less protection against port
  conflict.  [commit 4ab02e8]

- Got EPIPE when writing to stdin of a child process that exited too quickly.
  [commit e12a472]

Things have been stable on the buildfarm for the last twelve days, so I think
this one is over.

[1] https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=jacana&dt=2019-04-13%2014%3A39%3A17

  30. Re: [HACKERS] Weaker shmem interlock w/o postmaster.pid

    Noah Misch <noah@leadboat.com> — 2019-05-01T00:57:11Z

    On Tue, Apr 30, 2019 at 10:47:37PM +1200, Thomas Munro wrote:
> On Mon, Apr 22, 2019 at 4:59 AM Noah Misch <noah@leadboat.com> wrote:
> > On Thu, Apr 18, 2019 at 04:30:46PM +1200, Thomas Munro wrote:
> > > This causes make check-world to deliver a flurry of pop-ups from
> > > macOS's built-in Firewall asking if perl should be allowed to listen
> > > to all interfaces [...].
> >
> > That is unfortunate.  The "Allowing specific applications" section of
> > https://support.apple.com/en-us/HT201642 appears to offer a way to allow perl
> > permanently.  Separately, it wouldn't cost much for us to abandon that check
> > on !$use_tcp (non-Windows) configurations.
> 
> My system is set up not to allow that and I suppose I could go and
> argue with my IT department about that, but I'm interested in your
> second suggestion if the test is in fact not serving any useful
> purpose for non-Windows systems anyway.  Do you mean like this?
> 
> --- a/src/test/perl/PostgresNode.pm
> +++ b/src/test/perl/PostgresNode.pm
> @@ -1098,17 +1098,12 @@ sub get_new_node
>                 # native Perl (https://stackoverflow.com/a/14388707),
> so we also test
>                 # individual addresses.
>                 #
> -               # This seems like a good idea on Unixen as well, even
> though we don't
> -               # ask the postmaster to open a TCP port on Unix.  On Non-Linux,
> -               # non-Windows kernels, binding to 127.0.0.1/24
> addresses other than
> -               # 127.0.0.1 fails with EADDRNOTAVAIL.
> -               #

Deleting that comment paragraph isn't quite right, since we're still testing
127.0.0.1 everywhere.  The paragraph does have cause to change.

>                 # XXX A port available now may become unavailable by
> the time we start
>                 # the postmaster.
>                 if ($found == 1)
>                 {
> -                       foreach my $addr (qw(127.0.0.1 0.0.0.0),
> -                               $use_tcp ? qw(127.0.0.2 127.0.0.3) : ())
> +                       foreach my $addr (qw(127.0.0.1),
> +                               $use_tcp ? qw(0.0.0.0 127.0.0.2 127.0.0.3) : ())

This is what I meant, yes.

>                         {
>                                 can_bind($addr, $port) or $found = 0;
>                         }

  31. Re: [HACKERS] Weaker shmem interlock w/o postmaster.pid

    Thomas Munro <thomas.munro@gmail.com> — 2019-05-06T03:30:24Z

    On Wed, May 1, 2019 at 12:57 PM Noah Misch <noah@leadboat.com> wrote:
> On Tue, Apr 30, 2019 at 10:47:37PM +1200, Thomas Munro wrote:
> > --- a/src/test/perl/PostgresNode.pm
> > +++ b/src/test/perl/PostgresNode.pm
> > @@ -1098,17 +1098,12 @@ sub get_new_node
> >                 # native Perl (https://stackoverflow.com/a/14388707),
> > so we also test
> >                 # individual addresses.
> >                 #
> > -               # This seems like a good idea on Unixen as well, even
> > though we don't
> > -               # ask the postmaster to open a TCP port on Unix.  On Non-Linux,
> > -               # non-Windows kernels, binding to 127.0.0.1/24
> > addresses other than
> > -               # 127.0.0.1 fails with EADDRNOTAVAIL.
> > -               #
>
> Deleting that comment paragraph isn't quite right, since we're still testing
> 127.0.0.1 everywhere.  The paragraph does have cause to change.

Hi Noah,

I put the second sentence back and tweaked it thus: s/fails/might
fail/.  Maybe I'm being too pedantic here, but binding to 127.0.0.2
works on other OSes too, as long as you've configured an interface or
alias for it (and it's not terribly uncommon to do so).  Here's a
version with a commit message.  Please let me know if you want to
tweak the comments further.

-- 
Thomas Munro
https://enterprisedb.com

  32. Re: [HACKERS] Weaker shmem interlock w/o postmaster.pid

    Thomas Munro <thomas.munro@gmail.com> — 2019-05-08T10:10:35Z

    On Mon, May 6, 2019 at 3:30 PM Thomas Munro <thomas.munro@gmail.com> wrote:
> I put the second sentence back and tweaked it thus: s/fails/might
> fail/.  Maybe I'm being too pedantic here, but binding to 127.0.0.2
> works on other OSes too, as long as you've configured an interface or
> alias for it (and it's not terribly uncommon to do so).  Here's a
> version with a commit message.  Please let me know if you want to
> tweak the comments further.

Pushed, with a further adjustment to the comment.

-- 
Thomas Munro
https://enterprisedb.com

  33. Re: [HACKERS] Weaker shmem interlock w/o postmaster.pid

    Noah Misch <noah@leadboat.com> — 2019-05-10T07:47:03Z

    On Wed, May 08, 2019 at 10:10:35PM +1200, Thomas Munro wrote:
> On Mon, May 6, 2019 at 3:30 PM Thomas Munro <thomas.munro@gmail.com> wrote:
> > I put the second sentence back and tweaked it thus: s/fails/might
> > fail/.  Maybe I'm being too pedantic here, but binding to 127.0.0.2
> > works on other OSes too, as long as you've configured an interface or
> > alias for it (and it's not terribly uncommon to do so).

Even if, say, 99% of FreeBSD systems did configure such an interface, that
would not be enough to make it okay to probe 127.0.0.2 on FreeBSD.

> Pushed, with a further adjustment to the comment.

I'm fine with what you committed.