Horton

Thread

Commits

  1. Always skip recovery SysV shared memory tests on Windows

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

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-09-05T17:32:04Z

    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: https://postgr.es/m/16908.1557521200@sss.pgh.pa.us

Branch
------
master

Details
-------
https://git.postgresql.org/pg/commitdiff/7de19fbc0b1a9172d0907017302b32846b2887b9

Modified Files
--------------
src/backend/port/posix_sema.c       |  23 ++++--
src/backend/port/sysv_sema.c        |  23 ++++--
src/backend/port/sysv_shmem.c       |  38 +++++----
src/backend/port/win32_sema.c       |   2 +-
src/backend/port/win32_shmem.c      |   2 +-
src/backend/postmaster/postmaster.c |  25 +++---
src/backend/storage/ipc/ipci.c      |   6 +-
src/backend/utils/init/postinit.c   |   8 +-
src/include/storage/ipc.h           |   2 +-
src/include/storage/pg_sema.h       |   2 +-
src/include/storage/pg_shmem.h      |   2 +-
src/test/recovery/t/017_shm.pl      | 150 +++++++++++++++++++-----------------
12 files changed, 159 insertions(+), 124 deletions(-)

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

    Andrew Dunstan <andrew.dunstan@2ndquadrant.com> — 2019-09-06T15:09:02Z

    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: https://postgr.es/m/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

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-09-06T15:35:13Z

    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

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

    Andrew Dunstan <andrew.dunstan@2ndquadrant.com> — 2019-09-06T18:26:13Z

    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

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-09-06T18:42:06Z

    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

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

    Andrew Dunstan <andrew.dunstan@2ndquadrant.com> — 2019-09-06T19:51:54Z

    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

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

    Andrew Dunstan <andrew.dunstan@2ndquadrant.com> — 2019-09-08T21:54:12Z

    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

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-09-08T22:00:29Z

    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

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

    Noah Misch <noah@leadboat.com> — 2019-09-13T06:20:28Z

    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.)