Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit 7de19fb

Browse filesBrowse files
tglsfdctglsfdc
committed
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
1 parent 8b94dab commit 7de19fb
Copy full SHA for 7de19fb

File tree

Expand file treeCollapse file tree

12 files changed

+159
-124
lines changed
Filter options
Expand file treeCollapse file tree

12 files changed

+159
-124
lines changed

‎src/backend/port/posix_sema.c

Copy file name to clipboardExpand all lines: src/backend/port/posix_sema.c
+17-6Lines changed: 17 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@
2929
#include <semaphore.h>
3030
#include <signal.h>
3131
#include <unistd.h>
32+
#include <sys/stat.h>
3233

3334
#include "miscadmin.h"
3435
#include "storage/ipc.h"
@@ -181,10 +182,6 @@ PGSemaphoreShmemSize(int maxSemas)
181182
* are acquired here or in PGSemaphoreCreate, register an on_shmem_exit
182183
* callback to release them.
183184
*
184-
* The port number is passed for possible use as a key (for Posix, we use
185-
* it to generate the starting semaphore name). In a standalone backend,
186-
* zero will be passed.
187-
*
188185
* In the Posix implementation, we acquire semaphores on-demand; the
189186
* maxSemas parameter is just used to size the arrays. For unnamed
190187
* semaphores, there is an array of PGSemaphoreData structs in shared memory.
@@ -196,8 +193,22 @@ PGSemaphoreShmemSize(int maxSemas)
196193
* we don't have to expose the counters to other processes.)
197194
*/
198195
void
199-
PGReserveSemaphores(int maxSemas, int port)
196+
PGReserveSemaphores(int maxSemas)
200197
{
198+
struct stat statbuf;
199+
200+
/*
201+
* We use the data directory's inode number to seed the search for free
202+
* semaphore keys. This minimizes the odds of collision with other
203+
* postmasters, while maximizing the odds that we will detect and clean up
204+
* semaphores left over from a crashed postmaster in our own directory.
205+
*/
206+
if (stat(DataDir, &statbuf) < 0)
207+
ereport(FATAL,
208+
(errcode_for_file_access(),
209+
errmsg("could not stat data directory \"%s\": %m",
210+
DataDir)));
211+
201212
#ifdef USE_NAMED_POSIX_SEMAPHORES
202213
mySemPointers = (sem_t **) malloc(maxSemas * sizeof(sem_t *));
203214
if (mySemPointers == NULL)
@@ -215,7 +226,7 @@ PGReserveSemaphores(int maxSemas, int port)
215226

216227
numSems = 0;
217228
maxSems = maxSemas;
218-
nextSemKey = port * 1000;
229+
nextSemKey = statbuf.st_ino;
219230

220231
on_shmem_exit(ReleaseSemaphores, 0);
221232
}

‎src/backend/port/sysv_sema.c

Copy file name to clipboardExpand all lines: src/backend/port/sysv_sema.c
+17-6Lines changed: 17 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
#include <signal.h>
1818
#include <unistd.h>
1919
#include <sys/file.h>
20+
#include <sys/stat.h>
2021
#ifdef HAVE_SYS_IPC_H
2122
#include <sys/ipc.h>
2223
#endif
@@ -301,10 +302,6 @@ PGSemaphoreShmemSize(int maxSemas)
301302
* are acquired here or in PGSemaphoreCreate, register an on_shmem_exit
302303
* callback to release them.
303304
*
304-
* The port number is passed for possible use as a key (for SysV, we use
305-
* it to generate the starting semaphore key). In a standalone backend,
306-
* zero will be passed.
307-
*
308305
* In the SysV implementation, we acquire semaphore sets on-demand; the
309306
* maxSemas parameter is just used to size the arrays. There is an array
310307
* of PGSemaphoreData structs in shared memory, and a postmaster-local array
@@ -314,8 +311,22 @@ PGSemaphoreShmemSize(int maxSemas)
314311
* have clobbered.)
315312
*/
316313
void
317-
PGReserveSemaphores(int maxSemas, int port)
314+
PGReserveSemaphores(int maxSemas)
318315
{
316+
struct stat statbuf;
317+
318+
/*
319+
* We use the data directory's inode number to seed the search for free
320+
* semaphore keys. This minimizes the odds of collision with other
321+
* postmasters, while maximizing the odds that we will detect and clean up
322+
* semaphores left over from a crashed postmaster in our own directory.
323+
*/
324+
if (stat(DataDir, &statbuf) < 0)
325+
ereport(FATAL,
326+
(errcode_for_file_access(),
327+
errmsg("could not stat data directory \"%s\": %m",
328+
DataDir)));
329+
319330
/*
320331
* We must use ShmemAllocUnlocked(), since the spinlock protecting
321332
* ShmemAlloc() won't be ready yet. (This ordering is necessary when we
@@ -332,7 +343,7 @@ PGReserveSemaphores(int maxSemas, int port)
332343
if (mySemaSets == NULL)
333344
elog(PANIC, "out of memory");
334345
numSemaSets = 0;
335-
nextSemaKey = port * 1000;
346+
nextSemaKey = statbuf.st_ino;
336347
nextSemaNumber = SEMAS_PER_SET; /* force sema set alloc on 1st call */
337348

338349
on_shmem_exit(ReleaseSemaphores, 0);

‎src/backend/port/sysv_shmem.c

Copy file name to clipboardExpand all lines: src/backend/port/sysv_shmem.c
+21-17Lines changed: 21 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -390,11 +390,12 @@ PGSharedMemoryAttach(IpcMemoryId shmId,
390390

391391
/*
392392
* Try to attach to the segment and see if it matches our data directory.
393-
* This avoids key-conflict problems on machines that are running several
394-
* postmasters under the same userid and port number. (That would not
395-
* ordinarily happen in production, but it can happen during parallel
396-
* testing. Since our test setups don't open any TCP ports on Unix, such
397-
* cases don't conflict otherwise.)
393+
* This avoids any risk of duplicate-shmem-key conflicts on machines that
394+
* are running several postmasters under the same userid.
395+
*
396+
* (When we're called from PGSharedMemoryCreate, this stat call is
397+
* duplicative; but since this isn't a high-traffic case it's not worth
398+
* trying to optimize.)
398399
*/
399400
if (stat(DataDir, &statbuf) < 0)
400401
return SHMSTATE_ANALYSIS_FAILURE; /* can't stat; be conservative */
@@ -617,12 +618,9 @@ AnonymousShmemDetach(int status, Datum arg)
617618
* we do not fail upon collision with foreign shmem segments. The idea here
618619
* is to detect and re-use keys that may have been assigned by a crashed
619620
* postmaster or backend.
620-
*
621-
* The port number is passed for possible use as a key (for SysV, we use
622-
* it to generate the starting shmem key).
623621
*/
624622
PGShmemHeader *
625-
PGSharedMemoryCreate(Size size, int port,
623+
PGSharedMemoryCreate(Size size,
626624
PGShmemHeader **shim)
627625
{
628626
IpcMemoryKey NextShmemSegID;
@@ -631,6 +629,17 @@ PGSharedMemoryCreate(Size size, int port,
631629
struct stat statbuf;
632630
Size sysvsize;
633631

632+
/*
633+
* We use the data directory's ID info (inode and device numbers) to
634+
* positively identify shmem segments associated with this data dir, and
635+
* also as seeds for searching for a free shmem key.
636+
*/
637+
if (stat(DataDir, &statbuf) < 0)
638+
ereport(FATAL,
639+
(errcode_for_file_access(),
640+
errmsg("could not stat data directory \"%s\": %m",
641+
DataDir)));
642+
634643
/* Complain if hugepages demanded but we can't possibly support them */
635644
#if !defined(MAP_HUGETLB)
636645
if (huge_pages == HUGE_PAGES_ON)
@@ -659,10 +668,10 @@ PGSharedMemoryCreate(Size size, int port,
659668
/*
660669
* Loop till we find a free IPC key. Trust CreateDataDirLockFile() to
661670
* ensure no more than one postmaster per data directory can enter this
662-
* loop simultaneously. (CreateDataDirLockFile() does not ensure that,
663-
* but prefer fixing it over coping here.)
671+
* loop simultaneously. (CreateDataDirLockFile() does not entirely ensure
672+
* that, but prefer fixing it over coping here.)
664673
*/
665-
NextShmemSegID = 1 + port * 1000;
674+
NextShmemSegID = statbuf.st_ino;
666675

667676
for (;;)
668677
{
@@ -748,11 +757,6 @@ PGSharedMemoryCreate(Size size, int port,
748757
hdr->dsm_control = 0;
749758

750759
/* Fill in the data directory ID info, too */
751-
if (stat(DataDir, &statbuf) < 0)
752-
ereport(FATAL,
753-
(errcode_for_file_access(),
754-
errmsg("could not stat data directory \"%s\": %m",
755-
DataDir)));
756760
hdr->device = statbuf.st_dev;
757761
hdr->inode = statbuf.st_ino;
758762

‎src/backend/port/win32_sema.c

Copy file name to clipboardExpand all lines: src/backend/port/win32_sema.c
+1-1Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,7 @@ PGSemaphoreShmemSize(int maxSemas)
4444
* process exits.
4545
*/
4646
void
47-
PGReserveSemaphores(int maxSemas, int port)
47+
PGReserveSemaphores(int maxSemas)
4848
{
4949
mySemSet = (HANDLE *) malloc(maxSemas * sizeof(HANDLE));
5050
if (mySemSet == NULL)

‎src/backend/port/win32_shmem.c

Copy file name to clipboardExpand all lines: src/backend/port/win32_shmem.c
+1-1Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -194,7 +194,7 @@ EnableLockPagesPrivilege(int elevel)
194194
* standard header.
195195
*/
196196
PGShmemHeader *
197-
PGSharedMemoryCreate(Size size, int port,
197+
PGSharedMemoryCreate(Size size,
198198
PGShmemHeader **shim)
199199
{
200200
void *memAddress;

‎src/backend/postmaster/postmaster.c

Copy file name to clipboardExpand all lines: src/backend/postmaster/postmaster.c
+12-13Lines changed: 12 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -385,7 +385,7 @@ static void getInstallationPaths(const char *argv0);
385385
static void checkControlFile(void);
386386
static Port *ConnCreate(int serverFd);
387387
static void ConnFree(Port *port);
388-
static void reset_shared(int port);
388+
static void reset_shared(void);
389389
static void SIGHUP_handler(SIGNAL_ARGS);
390390
static void pmdie(SIGNAL_ARGS);
391391
static void reaper(SIGNAL_ARGS);
@@ -1175,7 +1175,7 @@ PostmasterMain(int argc, char *argv[])
11751175
/*
11761176
* Set up shared memory and semaphores.
11771177
*/
1178-
reset_shared(PostPortNumber);
1178+
reset_shared();
11791179

11801180
/*
11811181
* Estimate number of openable files. This must happen after setting up
@@ -2599,17 +2599,16 @@ InitProcessGlobals(void)
25992599
* reset_shared -- reset shared memory and semaphores
26002600
*/
26012601
static void
2602-
reset_shared(int port)
2602+
reset_shared(void)
26032603
{
26042604
/*
26052605
* Create or re-create shared memory and semaphores.
26062606
*
26072607
* Note: in each "cycle of life" we will normally assign the same IPC keys
2608-
* (if using SysV shmem and/or semas), since the port number is used to
2609-
* determine IPC keys. This helps ensure that we will clean up dead IPC
2610-
* objects if the postmaster crashes and is restarted.
2608+
* (if using SysV shmem and/or semas). This helps ensure that we will
2609+
* clean up dead IPC objects if the postmaster crashes and is restarted.
26112610
*/
2612-
CreateSharedMemoryAndSemaphores(port);
2611+
CreateSharedMemoryAndSemaphores();
26132612
}
26142613

26152614

@@ -3934,7 +3933,7 @@ PostmasterStateMachine(void)
39343933
/* re-read control file into local memory */
39353934
LocalProcessControlFile(true);
39363935

3937-
reset_shared(PostPortNumber);
3936+
reset_shared();
39383937

39393938
StartupPID = StartupDataBase();
39403939
Assert(StartupPID != 0);
@@ -4962,7 +4961,7 @@ SubPostmasterMain(int argc, char *argv[])
49624961
InitProcess();
49634962

49644963
/* Attach process to shared data structures */
4965-
CreateSharedMemoryAndSemaphores(0);
4964+
CreateSharedMemoryAndSemaphores();
49664965

49674966
/* And run the backend */
49684967
BackendRun(&port); /* does not return */
@@ -4976,7 +4975,7 @@ SubPostmasterMain(int argc, char *argv[])
49764975
InitAuxiliaryProcess();
49774976

49784977
/* Attach process to shared data structures */
4979-
CreateSharedMemoryAndSemaphores(0);
4978+
CreateSharedMemoryAndSemaphores();
49804979

49814980
AuxiliaryProcessMain(argc - 2, argv + 2); /* does not return */
49824981
}
@@ -4989,7 +4988,7 @@ SubPostmasterMain(int argc, char *argv[])
49894988
InitProcess();
49904989

49914990
/* Attach process to shared data structures */
4992-
CreateSharedMemoryAndSemaphores(0);
4991+
CreateSharedMemoryAndSemaphores();
49934992

49944993
AutoVacLauncherMain(argc - 2, argv + 2); /* does not return */
49954994
}
@@ -5002,7 +5001,7 @@ SubPostmasterMain(int argc, char *argv[])
50025001
InitProcess();
50035002

50045003
/* Attach process to shared data structures */
5005-
CreateSharedMemoryAndSemaphores(0);
5004+
CreateSharedMemoryAndSemaphores();
50065005

50075006
AutoVacWorkerMain(argc - 2, argv + 2); /* does not return */
50085007
}
@@ -5020,7 +5019,7 @@ SubPostmasterMain(int argc, char *argv[])
50205019
InitProcess();
50215020

50225021
/* Attach process to shared data structures */
5023-
CreateSharedMemoryAndSemaphores(0);
5022+
CreateSharedMemoryAndSemaphores();
50245023

50255024
/* Fetch MyBgworkerEntry from shared memory */
50265025
shmem_slot = atoi(argv[1] + 15);

‎src/backend/storage/ipc/ipci.c

Copy file name to clipboardExpand all lines: src/backend/storage/ipc/ipci.c
+3-3Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -91,7 +91,7 @@ RequestAddinShmemSpace(Size size)
9191
* This is a bit code-wasteful and could be cleaned up.)
9292
*/
9393
void
94-
CreateSharedMemoryAndSemaphores(int port)
94+
CreateSharedMemoryAndSemaphores(void)
9595
{
9696
PGShmemHeader *shim = NULL;
9797

@@ -163,14 +163,14 @@ CreateSharedMemoryAndSemaphores(int port)
163163
/*
164164
* Create the shmem segment
165165
*/
166-
seghdr = PGSharedMemoryCreate(size, port, &shim);
166+
seghdr = PGSharedMemoryCreate(size, &shim);
167167

168168
InitShmemAccess(seghdr);
169169

170170
/*
171171
* Create semaphores
172172
*/
173-
PGReserveSemaphores(numSemas, port);
173+
PGReserveSemaphores(numSemas);
174174

175175
/*
176176
* If spinlocks are disabled, initialize emulation layer (which

‎src/backend/utils/init/postinit.c

Copy file name to clipboardExpand all lines: src/backend/utils/init/postinit.c
+3-5Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -445,12 +445,10 @@ InitCommunication(void)
445445
if (!IsUnderPostmaster) /* postmaster already did this */
446446
{
447447
/*
448-
* We're running a postgres bootstrap process or a standalone backend.
449-
* Though we won't listen on PostPortNumber, use it to select a shmem
450-
* key. This increases the chance of detecting a leftover live
451-
* backend of this DataDir.
448+
* We're running a postgres bootstrap process or a standalone backend,
449+
* so we need to set up shmem.
452450
*/
453-
CreateSharedMemoryAndSemaphores(PostPortNumber);
451+
CreateSharedMemoryAndSemaphores();
454452
}
455453
}
456454

‎src/include/storage/ipc.h

Copy file name to clipboardExpand all lines: src/include/storage/ipc.h
+1-1Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -76,6 +76,6 @@ extern void on_exit_reset(void);
7676
/* ipci.c */
7777
extern PGDLLIMPORT shmem_startup_hook_type shmem_startup_hook;
7878

79-
extern void CreateSharedMemoryAndSemaphores(int port);
79+
extern void CreateSharedMemoryAndSemaphores(void);
8080

8181
#endif /* IPC_H */

‎src/include/storage/pg_sema.h

Copy file name to clipboardExpand all lines: src/include/storage/pg_sema.h
+1-1Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,7 @@ typedef HANDLE PGSemaphore;
4141
extern Size PGSemaphoreShmemSize(int maxSemas);
4242

4343
/* Module initialization (called during postmaster start or shmem reinit) */
44-
extern void PGReserveSemaphores(int maxSemas, int port);
44+
extern void PGReserveSemaphores(int maxSemas);
4545

4646
/* Allocate a PGSemaphore structure with initial count 1 */
4747
extern PGSemaphore PGSemaphoreCreate(void);

‎src/include/storage/pg_shmem.h

Copy file name to clipboardExpand all lines: src/include/storage/pg_shmem.h
+1-1Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -82,7 +82,7 @@ extern void PGSharedMemoryReAttach(void);
8282
extern void PGSharedMemoryNoReAttach(void);
8383
#endif
8484

85-
extern PGShmemHeader *PGSharedMemoryCreate(Size size, int port,
85+
extern PGShmemHeader *PGSharedMemoryCreate(Size size,
8686
PGShmemHeader **shim);
8787
extern bool PGSharedMemoryIsInUse(unsigned long id1, unsigned long id2);
8888
extern void PGSharedMemoryDetach(void);

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.