Re: [HACKERS] Weaker shmem interlock w/o postmaster.pid
On Wed, May 08, 2019 at 10:10:35PM +1200, Thomas Munro wrote: > On Mon, May 6, 2019 at 3:30 PM Thomas Munro 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.
Re: [HACKERS] Weaker shmem interlock w/o postmaster.pid
On Mon, May 6, 2019 at 3:30 PM Thomas Munro 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
Re: [HACKERS] Weaker shmem interlock w/o postmaster.pid
On Wed, May 1, 2019 at 12:57 PM Noah Misch 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 0001-Probe-only-127.0.0.1-when-looking-for-ports-on-Unix.patch Description: Binary data
Re: [HACKERS] Weaker shmem interlock w/o postmaster.pid
On Tue, Apr 30, 2019 at 10:47:37PM +1200, Thomas Munro wrote:
> On Mon, Apr 22, 2019 at 4:59 AM Noah Misch 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;
> }
Re: [HACKERS] Weaker shmem interlock w/o postmaster.pid
On Fri, Mar 29, 2019 at 09:53:51AM +, Daniel Gustafsson wrote: > On Saturday, March 9, 2019 8:16 AM, Noah Misch 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 " 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=2019-04-13%2014%3A39%3A17
Re: [HACKERS] Weaker shmem interlock w/o postmaster.pid
On Thu, Apr 18, 2019 at 04:30:46PM +1200, Thomas Munro wrote:
> On Thu, Apr 11, 2019 at 6:22 PM Noah Misch 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.
Re: [HACKERS] Weaker shmem interlock w/o postmaster.pid
On Thu, Apr 11, 2019 at 6:22 PM Noah Misch 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
Re: [HACKERS] Weaker shmem interlock w/o postmaster.pid
On Thu, Apr 11, 2019 at 12:48:35PM +1200, Thomas Munro wrote: > On Mon, Apr 8, 2019 at 6:42 PM Noah Misch 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.
Re: [HACKERS] Weaker shmem interlock w/o postmaster.pid
On Mon, Apr 8, 2019 at 6:42 PM Noah Misch 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
Re: [HACKERS] Weaker shmem interlock w/o postmaster.pid
On Mon, Apr 08, 2019 at 08:07:28PM +1200, Thomas Munro wrote:
> On Mon, Apr 8, 2019 at 6:42 PM Noah Misch 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=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
Re: [HACKERS] Weaker shmem interlock w/o postmaster.pid
On Mon, Apr 8, 2019 at 6:42 PM Noah Misch 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=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
Re: [HACKERS] Weaker shmem interlock w/o postmaster.pid
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=2019-04-04%2000%3A33%3A14
> > https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=komodoensis=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=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=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=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=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=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=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=2019-04-04%2009:47:02
diff --git a/src/Makefile.global.in b/src/Makefile.global.in
index 6b98d53..b9d86ac 100644
--- a/src/Makefile.global.in
+++ b/src/Makefile.global.in
@@ -430,13 +430,13 @@ ifeq ($(enable_tap_tests),yes)
define prove_installcheck
rm -rf '$(CURDIR)'/tmp_check
$(MKDIR_P) '$(CURDIR)'/tmp_check
-cd $(srcdir) && TESTDIR='$(CURDIR)' PATH="$(bindir):$$PATH"
PGPORT='6$(DEF_PGPORT)' top_builddir='$(CURDIR)/$(top_builddir)'
PG_REGRESS='$(CURDIR)/$(top_builddir)/src/test/regress/pg_regress' $(PROVE)
$(PG_PROVE_FLAGS) $(PROVE_FLAGS) $(if $(PROVE_TESTS),$(PROVE_TESTS),t/*.pl)
+cd $(srcdir) && TESTDIR='$(CURDIR)'
Re: [HACKERS] Weaker shmem interlock w/o postmaster.pid
On Wed, Apr 03, 2019 at 07:05:43PM -0700, Noah Misch wrote: > On Mon, Apr 01, 2019 at 08:19:56AM +, Daniel Gustafsson wrote: > > On Monday, April 1, 2019 12:42 AM, Noah Misch wrote: > > > On Fri, Mar 29, 2019 at 09:53:51AM +, 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=2019-04-04%2000%3A33%3A14 > https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=komodoensis=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.
Re: [HACKERS] Weaker shmem interlock w/o postmaster.pid
On Mon, Apr 01, 2019 at 08:19:56AM +, Daniel Gustafsson wrote: > On Monday, April 1, 2019 12:42 AM, Noah Misch wrote: > > On Fri, Mar 29, 2019 at 09:53:51AM +, 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=2019-04-04%2000%3A33%3A14 https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=komodoensis=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.
Re: [HACKERS] Weaker shmem interlock w/o postmaster.pid
On Monday, April 1, 2019 12:42 AM, Noah Misch wrote: > On Fri, Mar 29, 2019 at 09:53:51AM +, 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
Re: [HACKERS] Weaker shmem interlock w/o postmaster.pid
On Fri, Mar 29, 2019 at 09:53:51AM +, Daniel Gustafsson wrote: > On Saturday, March 9, 2019 8:16 AM, Noah Misch 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, ); > + 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.
Re: [HACKERS] Weaker shmem interlock w/o postmaster.pid
On Saturday, March 9, 2019 8:16 AM, Noah Misch 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, ); + 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
Re: [HACKERS] Weaker shmem interlock w/o postmaster.pid
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?
--- a/src/backend/utils/init/miscinit.c
+++ b/src/backend/utils/init/miscinit.c
@@ -1070,14 +1070,10 @@ CreateLockFile(const char *filename, bool amPostmaster,
if (PGSharedMemoryIsInUse(id1, id2))
ereport(FATAL,
(errcode(ERRCODE_LOCK_FILE_EXISTS),
-errmsg("pre-existing
shared memory block "
- "(key
%lu, ID %lu) is still in use",
+errmsg("pre-existing
shared memory block (key %lu, ID %lu) is still in use",
id1,
id2),
-errhint("If you're
sure there are no old "
-
"server processes still running, remove "
-"the
shared memory block "
-"or
just delete the file \"%s\".",
-
filename)));
+errhint("Terminate any
old server processes associated with data directory \"%s\".",
+
refName)));
}
}
diff --git a/src/Makefile.global.in b/src/Makefile.global.in
index c118f64..e8f6050 100644
--- a/src/Makefile.global.in
+++ b/src/Makefile.global.in
@@ -430,13 +430,13 @@ ifeq ($(enable_tap_tests),yes)
define prove_installcheck
rm -rf '$(CURDIR)'/tmp_check
$(MKDIR_P) '$(CURDIR)'/tmp_check
-cd $(srcdir) && TESTDIR='$(CURDIR)' PATH="$(bindir):$$PATH"
PGPORT='6$(DEF_PGPORT)' top_builddir='$(CURDIR)/$(top_builddir)'
PG_REGRESS='$(CURDIR)/$(top_builddir)/src/test/regress/pg_regress' $(PROVE)
$(PG_PROVE_FLAGS) $(PROVE_FLAGS) $(if $(PROVE_TESTS),$(PROVE_TESTS),t/*.pl)
+cd $(srcdir) && TESTDIR='$(CURDIR)' PATH="$(bindir):$$PATH"
PGPORT='6$(DEF_PGPORT)' top_builddir='$(CURDIR)/$(top_builddir)'
PG_REGRESS='$(CURDIR)/$(top_builddir)/src/test/regress/pg_regress'
REGRESS_SHLIB='$(abs_top_builddir)/src/test/regress/regress$(DLSUFFIX)'
$(PROVE) $(PG_PROVE_FLAGS) $(PROVE_FLAGS) $(if
$(PROVE_TESTS),$(PROVE_TESTS),t/*.pl)
endef
define prove_check
rm -rf '$(CURDIR)'/tmp_check
$(MKDIR_P) '$(CURDIR)'/tmp_check
-cd $(srcdir) && TESTDIR='$(CURDIR)' $(with_temp_install)
PGPORT='6$(DEF_PGPORT)'
PG_REGRESS='$(CURDIR)/$(top_builddir)/src/test/regress/pg_regress' $(PROVE)
$(PG_PROVE_FLAGS) $(PROVE_FLAGS) $(if $(PROVE_TESTS),$(PROVE_TESTS),t/*.pl)
+cd $(srcdir) && TESTDIR='$(CURDIR)' $(with_temp_install)
PGPORT='6$(DEF_PGPORT)'
PG_REGRESS='$(CURDIR)/$(top_builddir)/src/test/regress/pg_regress'
REGRESS_SHLIB='$(abs_top_builddir)/src/test/regress/regress$(DLSUFFIX)'
$(PROVE) $(PG_PROVE_FLAGS) $(PROVE_FLAGS) $(if
$(PROVE_TESTS),$(PROVE_TESTS),t/*.pl)
endef
else
diff --git a/src/backend/port/sysv_shmem.c b/src/backend/port/sysv_shmem.c
index 1511a61..c9f6e43 100644
--- a/src/backend/port/sysv_shmem.c
+++ b/src/backend/port/sysv_shmem.c
@@ -72,6 +72,26 @@
typedef key_t IpcMemoryKey;/* shared memory key passed to
shmget(2) */
typedef int IpcMemoryId; /* shared memory ID returned by
Re: [HACKERS] Weaker shmem interlock w/o postmaster.pid
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 wrote in > <20180812064815.gb2301...@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
Re: [HACKERS] Weaker shmem interlock w/o postmaster.pid
Hello. At Sun, 2 Dec 2018 16:41:06 -0800, Noah Misch wrote in <20181203004106.ga2860...@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 wrote in <20180812064815.gb2301...@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
Re: [HACKERS] Weaker shmem interlock w/o postmaster.pid
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 signature.asc Description: PGP signature
Re: [HACKERS] Weaker shmem interlock w/o postmaster.pid
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.
--- a/src/backend/utils/init/miscinit.c
+++ b/src/backend/utils/init/miscinit.c
@@ -1070,14 +1070,10 @@ CreateLockFile(const char *filename, bool amPostmaster,
if (PGSharedMemoryIsInUse(id1, id2))
ereport(FATAL,
(errcode(ERRCODE_LOCK_FILE_EXISTS),
-errmsg("pre-existing
shared memory block "
- "(key
%lu, ID %lu) is still in use",
+errmsg("pre-existing
shared memory block (key %lu, ID %lu) is still in use",
id1,
id2),
-errhint("If you're
sure there are no old "
-
"server processes still running, remove "
-"the
shared memory block "
-"or
just delete the file \"%s\".",
-
filename)));
+errhint("Terminate any
old server processes associated with data directory \"%s\".",
+
refName)));
}
}
diff --git a/src/Makefile.global.in b/src/Makefile.global.in
index 956fd27..a67455b 100644
--- a/src/Makefile.global.in
+++ b/src/Makefile.global.in
@@ -421,13 +421,13 @@ ifeq ($(enable_tap_tests),yes)
define prove_installcheck
rm -rf '$(CURDIR)'/tmp_check
$(MKDIR_P) '$(CURDIR)'/tmp_check
-cd $(srcdir) && TESTDIR='$(CURDIR)' PATH="$(bindir):$$PATH"
PGPORT='6$(DEF_PGPORT)' top_builddir='$(CURDIR)/$(top_builddir)'
PG_REGRESS='$(CURDIR)/$(top_builddir)/src/test/regress/pg_regress' $(PROVE)
$(PG_PROVE_FLAGS) $(PROVE_FLAGS) $(if $(PROVE_TESTS),$(PROVE_TESTS),t/*.pl)
+cd $(srcdir) && TESTDIR='$(CURDIR)' PATH="$(bindir):$$PATH"
PGPORT='6$(DEF_PGPORT)' top_builddir='$(CURDIR)/$(top_builddir)'
PG_REGRESS='$(CURDIR)/$(top_builddir)/src/test/regress/pg_regress'
REGRESS_SHLIB='$(abs_top_builddir)/src/test/regress/regress$(DLSUFFIX)'
$(PROVE) $(PG_PROVE_FLAGS) $(PROVE_FLAGS) $(if
$(PROVE_TESTS),$(PROVE_TESTS),t/*.pl)
endef
define prove_check
rm -rf '$(CURDIR)'/tmp_check
$(MKDIR_P) '$(CURDIR)'/tmp_check
-cd $(srcdir) && TESTDIR='$(CURDIR)' $(with_temp_install)
PGPORT='6$(DEF_PGPORT)'
PG_REGRESS='$(CURDIR)/$(top_builddir)/src/test/regress/pg_regress' $(PROVE)
$(PG_PROVE_FLAGS) $(PROVE_FLAGS) $(if $(PROVE_TESTS),$(PROVE_TESTS),t/*.pl)
+cd $(srcdir) && TESTDIR='$(CURDIR)' $(with_temp_install)
PGPORT='6$(DEF_PGPORT)'
PG_REGRESS='$(CURDIR)/$(top_builddir)/src/test/regress/pg_regress'
REGRESS_SHLIB='$(abs_top_builddir)/src/test/regress/regress$(DLSUFFIX)'
$(PROVE) $(PG_PROVE_FLAGS) $(PROVE_FLAGS) $(if
$(PROVE_TESTS),$(PROVE_TESTS),t/*.pl)
endef
else
diff --git a/src/backend/port/sysv_shmem.c b/src/backend/port/sysv_shmem.c
index 741c455..7a9a68e 100644
--- a/src/backend/port/sysv_shmem.c
+++ b/src/backend/port/sysv_shmem.c
@@ -71,6 +71,26 @@
typedef key_t IpcMemoryKey;/* shared memory key passed to
shmget(2) */
typedef int IpcMemoryId; /* shared memory ID returned by
shmget(2) */
+/*
+ *
Re: [HACKERS] Weaker shmem interlock w/o postmaster.pid
On Thu, Nov 29, 2018 at 01:10:57PM +0100, Dmitry Dolgov wrote: > On Sun, Aug 12, 2018 at 8:48 AM Noah Misch 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.
Re: [HACKERS] Weaker shmem interlock w/o postmaster.pid
> On Sun, Aug 12, 2018 at 8:48 AM Noah Misch 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 (n...@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)'
Re: [HACKERS] Weaker shmem interlock w/o postmaster.pid
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 (n...@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
(0x0001 to 0x002d 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
---
