Обсуждение: pgsql: Use data directory inode number, not port,to select SysV resour
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(-)
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
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