pgsql: Use data directory inode number, not port, to select SysV resour

On 9/5/19 1:32 PM, Tom Lane wrote:

Use data directory inode number, not port, to select SysV resource keys.

This approach provides a much tighter binding between a data directory
and the associated SysV shared memory block (and SysV or named-POSIX
semaphores, if we're using those). Key collisions are still possible,
but only between data directories stored on different filesystems,
so the situation should be negligible in practice. More importantly,
restarting the postmaster with a different port number no longer
risks failing to identify a relevant shared memory block, even when
postmaster.pid has been removed. A standalone backend is likewise
much more certain to detect conflicting leftover backends.

(In the longer term, we might now think about deprecating the port as
a cluster-wide value, so that one postmaster could support sockets
with varying port numbers. But that's for another day.)

The hazards fixed here apply only on Unix systems; our Windows code
paths already use identifiers derived from the data directory path
name rather than the port.

src/test/recovery/t/017_shm.pl, which intends to test key-collision
cases, has been substantially rewritten since it can no longer use
two postmasters with identical port numbers to trigger the case.
Instead, use Perl's IPC::SharedMem module to create a conflicting
shmem segment directly. The test script will be skipped if that
module is not available. (This means that some older buildfarm
members won't run it, but I don't think that that results in any
meaningful coverage loss.)

Patch by me; thanks to Noah Misch and Peter Eisentraut for discussion
and review.

Discussion: /messages/by-id/16908.1557521200@sss.pgh.pa.us

This has caused the 017_shm.pl tests to be skipped on jacana and
bowerbird, and to fail completely on my msys2 test system where the Perl
has the relevant IPC:: modules, unlike the buildfarm animals.

Maybe we need to fall back on the older code on Windows?

cheers

andrew

--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:

On 9/5/19 1:32 PM, Tom Lane wrote:

Use data directory inode number, not port, to select SysV resource keys.

This has caused the 017_shm.pl tests to be skipped on jacana and
bowerbird, and to fail completely on my msys2 test system where the Perl
has the relevant IPC:: modules, unlike the buildfarm animals.

I intended 017_shm.pl to be skipped on Windows builds; it's not apparent
to me that that script tests anything useful when we're not using SysV
shared memory.

I don't quite understand what the msys2 platform might be doing with
these IPC modules. Do they actually do anything, or just fail at
runtime? If the latter, maybe we can add something to the eval{}
block to check for present-but-doesnt-work?

regards, tom lane

On 9/6/19 11:35 AM, Tom Lane wrote:

Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:

On 9/5/19 1:32 PM, Tom Lane wrote:

Use data directory inode number, not port, to select SysV resource keys.

This has caused the 017_shm.pl tests to be skipped on jacana and
bowerbird, and to fail completely on my msys2 test system where the Perl
has the relevant IPC:: modules, unlike the buildfarm animals.

I intended 017_shm.pl to be skipped on Windows builds; it's not apparent
to me that that script tests anything useful when we're not using SysV
shared memory.

I don't quite understand what the msys2 platform might be doing with
these IPC modules. Do they actually do anything, or just fail at
runtime? If the latter, maybe we can add something to the eval{}
block to check for present-but-doesnt-work?

Given your stated intention, I think the simplest way to get it is just
this, without worrying about what the perl modules might do:

diff --git a/src/test/recovery/t/017_shm.pl b/src/test/recovery/t/017_shm.pl
index a29ef78855..dc0dcd3ca2 100644
--- a/src/test/recovery/t/017_shm.pl
+++ b/src/test/recovery/t/017_shm.pl
@@ -18,7 +18,7 @@ eval {
    require IPC::SysV;
    IPC::SysV->import(qw(IPC_CREAT IPC_EXCL S_IRUSR S_IWUSR));
 };
-if ($@)
+if ($@ || $windows_os)
 {
    plan skip_all => 'SysV shared memory not supported by this platform';
 }

cheers

andrew

--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:

Given your stated intention, I think the simplest way to get it is just
this, without worrying about what the perl modules might do:

-if ($@)
+if ($@ || $windows_os)

WFM, do you want to push that?

regards, tom lane

On 9/6/19 2:42 PM, Tom Lane wrote:

Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:

Given your stated intention, I think the simplest way to get it is just
this, without worrying about what the perl modules might do:
-if ($@)
+if ($@ || $windows_os)

WFM, do you want to push that?

done.

cheers

andrew

--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 9/6/19 3:51 PM, Andrew Dunstan wrote:

On 9/6/19 2:42 PM, Tom Lane wrote:

Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:

Given your stated intention, I think the simplest way to get it is just
this, without worrying about what the perl modules might do:
-if ($@)
+if ($@ || $windows_os)

WFM, do you want to push that?

done.

[redirected to -hackers]

I'm going to disable this test (src/test/recovery/t/017_shm.pl) on
Windows on the back branches too unless there's a violent objection. The
reason is that the script runs "postgres --single" and that fails on
Windows when run by an administrative account. We've carefully enabled
postgres and its tests to run safely under an admin account. I
discovered this as part of my myss2 testing.

cheers

andrew

Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:

I'm going to disable this test (src/test/recovery/t/017_shm.pl) on
Windows on the back branches too unless there's a violent objection.

As I said before, I think that test does nothing useful unless SysV
shmem is in use, so I see no reason not to disable it on Windows.

regards, tom lane

On Sun, Sep 08, 2019 at 05:54:12PM -0400, Andrew Dunstan wrote:

I'm going to disable this test (src/test/recovery/t/017_shm.pl) on
Windows on the back branches too unless there's a violent objection. The
reason is that the script runs "postgres --single" and that fails on
Windows when run by an administrative account. We've carefully enabled
postgres and its tests to run safely under an admin account. I
discovered this as part of my myss2 testing.

I'm reading that the test falsified this assertion that we've enabled postgres
to run safely under an admin account. Enabling safe use of admin accounts
entails fixing single-user mode. (Alternately, one could replace the "vacuum
that database in single-user mode" errhint with a reference to some
not-yet-built alternative. That sounds harder.)