Skip to content

Commit 3a70b66

Browse files
nmischnmisch
committed
Consistently test for in-use shared memory.
postmaster startup scrutinizes any shared memory segment recorded in postmaster.pid, exiting if that segment matches the current data directory and has an attached process. When the postmaster.pid file was missing, a starting postmaster used weaker checks. Change to use the same checks in both scenarios. This increases the chance of a startup failure, in lieu of data corruption, if the DBA does "kill -9 `head -n1 postmaster.pid` && rm postmaster.pid && pg_ctl -w start". A postmaster will no longer recycle segments pertaining to other data directories. That's good for production, but it's bad for integration tests that crash a postmaster and immediately delete its data directory. Such a test now leaks a segment indefinitely. No "make check-world" test does that. win32_shmem.c already avoided all these problems. In 9.6 and later, enhance PostgresNode to facilitate testing. Back-patch to 9.4 (all supported versions). Reviewed by Daniel Gustafsson and Kyotaro HORIGUCHI. Discussion: https://postgr.es/m/20130911033341.GD225735@tornado.leadboat.com
1 parent f3461cb commit 3a70b66

File tree

7 files changed

+164
-138
lines changed

7 files changed

+164
-138
lines changed

‎src/backend/port/sysv_shmem.c

Lines changed: 145 additions & 110 deletions
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,26 @@
7070
typedef key_t IpcMemoryKey; /* shared memory key passed to shmget(2) */
7171
typedef int IpcMemoryId; /* shared memory ID returned by shmget(2) */
7272

73+
/*
74+
* How does a given IpcMemoryId relate to this PostgreSQL process?
75+
*
76+
* One could recycle unattached segments of different data directories if we
77+
* distinguished that case from other SHMSTATE_FOREIGN cases. Doing so would
78+
* cause us to visit less of the key space, making us less likely to detect a
79+
* SHMSTATE_ATTACHED key. It would also complicate the concurrency analysis,
80+
* in that postmasters of different data directories could simultaneously
81+
* attempt to recycle a given key. We'll waste keys longer in some cases, but
82+
* avoiding the problems of the alternative justifies that loss.
83+
*/
84+
typedef enum
85+
{
86+
SHMSTATE_ANALYSIS_FAILURE, /* unexpected failure to analyze the ID */
87+
SHMSTATE_ATTACHED, /* pertinent to DataDir, has attached PIDs */
88+
SHMSTATE_ENOENT, /* no segment of that ID */
89+
SHMSTATE_FOREIGN, /* exists, but not pertinent to DataDir */
90+
SHMSTATE_UNATTACHED /* pertinent to DataDir, no attached PIDs */
91+
} IpcMemoryState;
92+
7393

7494
unsigned long UsedShmemSegID = 0;
7595
void *UsedShmemSegAddr = NULL;
@@ -82,8 +102,8 @@ static void *AnonymousShmem = NULL;
82102
static void *InternalIpcMemoryCreate(IpcMemoryKey memKey, Size size);
83103
static void IpcMemoryDetach(int status, Datum shmaddr);
84104
static void IpcMemoryDelete(int status, Datum shmId);
85-
static PGShmemHeader *PGSharedMemoryAttach(IpcMemoryKey key,
86-
IpcMemoryId *shmid);
105+
static IpcMemoryState PGSharedMemoryAttach(IpcMemoryId shmId,
106+
PGShmemHeader **addr);
87107

88108

89109
/*
@@ -287,11 +307,36 @@ IpcMemoryDelete(int status, Datum shmId)
287307
bool
288308
PGSharedMemoryIsInUse(unsigned long id1, unsigned long id2)
289309
{
290-
IpcMemoryId shmId = (IpcMemoryId) id2;
310+
PGShmemHeader *memAddress;
311+
IpcMemoryState state;
312+
313+
state = PGSharedMemoryAttach((IpcMemoryId) id2, &memAddress);
314+
if (memAddress && shmdt(memAddress) < 0)
315+
elog(LOG, "shmdt(%p) failed: %m", memAddress);
316+
switch (state)
317+
{
318+
case SHMSTATE_ENOENT:
319+
case SHMSTATE_FOREIGN:
320+
case SHMSTATE_UNATTACHED:
321+
return false;
322+
case SHMSTATE_ANALYSIS_FAILURE:
323+
case SHMSTATE_ATTACHED:
324+
return true;
325+
}
326+
return true;
327+
}
328+
329+
/* See comment at IpcMemoryState. */
330+
static IpcMemoryState
331+
PGSharedMemoryAttach(IpcMemoryId shmId,
332+
PGShmemHeader **addr)
333+
{
291334
struct shmid_ds shmStat;
292335
struct stat statbuf;
293336
PGShmemHeader *hdr;
294337

338+
*addr = NULL;
339+
295340
/*
296341
* We detect whether a shared memory segment is in use by seeing whether
297342
* it (a) exists and (b) has any processes attached to it.
@@ -304,15 +349,15 @@ PGSharedMemoryIsInUse(unsigned long id1, unsigned long id2)
304349
* exists.
305350
*/
306351
if (errno == EINVAL)
307-
return false;
352+
return SHMSTATE_ENOENT;
308353

309354
/*
310355
* EACCES implies that the segment belongs to some other userid, which
311356
* means it is not a Postgres shmem segment (or at least, not one that
312357
* is relevant to our data directory).
313358
*/
314359
if (errno == EACCES)
315-
return false;
360+
return SHMSTATE_FOREIGN;
316361

317362
/*
318363
* Some Linux kernel versions (in fact, all of them as of July 2007)
@@ -323,50 +368,47 @@ PGSharedMemoryIsInUse(unsigned long id1, unsigned long id2)
323368
*/
324369
#ifdef HAVE_LINUX_EIDRM_BUG
325370
if (errno == EIDRM)
326-
return false;
371+
return SHMSTATE_ENOENT;
327372
#endif
328373

329374
/*
330375
* Otherwise, we had better assume that the segment is in use. The
331376
* only likely case is EIDRM, which implies that the segment has been
332377
* IPC_RMID'd but there are still processes attached to it.
333378
*/
334-
return true;
379+
return SHMSTATE_ANALYSIS_FAILURE;
335380
}
336381

337-
/* If it has no attached processes, it's not in use */
338-
if (shmStat.shm_nattch == 0)
339-
return false;
340-
341382
/*
342383
* Try to attach to the segment and see if it matches our data directory.
343384
* This avoids shmid-conflict problems on machines that are running
344385
* several postmasters under the same userid.
345386
*/
346387
if (stat(DataDir, &statbuf) < 0)
347-
return true; /* if can't stat, be conservative */
348-
349-
hdr = (PGShmemHeader *) shmat(shmId, NULL, PG_SHMAT_FLAGS);
388+
return SHMSTATE_ANALYSIS_FAILURE; /* can't stat; be conservative */
350389

390+
/*
391+
* If we can't attach, be conservative. This may fail if postmaster.pid
392+
* furnished the shmId and another user created a world-readable segment
393+
* of the same shmId.
394+
*/
395+
hdr = (PGShmemHeader *) shmat(shmId, UsedShmemSegAddr, PG_SHMAT_FLAGS);
351396
if (hdr == (PGShmemHeader *) -1)
352-
return true; /* if can't attach, be conservative */
397+
return SHMSTATE_ANALYSIS_FAILURE;
398+
*addr = hdr;
353399

354400
if (hdr->magic != PGShmemMagic ||
355401
hdr->device != statbuf.st_dev ||
356402
hdr->inode != statbuf.st_ino)
357403
{
358404
/*
359405
* It's either not a Postgres segment, or not one for my data
360-
* directory. In either case it poses no threat.
406+
* directory.
361407
*/
362-
shmdt((void *) hdr);
363-
return false;
408+
return SHMSTATE_FOREIGN;
364409
}
365410

366-
/* Trouble --- looks a lot like there's still live backends */
367-
shmdt((void *) hdr);
368-
369-
return true;
411+
return shmStat.shm_nattch == 0 ? SHMSTATE_UNATTACHED : SHMSTATE_ATTACHED;
370412
}
371413

372414
#ifdef USE_ANONYMOUS_SHMEM
@@ -542,25 +584,21 @@ AnonymousShmemDetach(int status, Datum arg)
542584
* standard header. Also, register an on_shmem_exit callback to release
543585
* the storage.
544586
*
545-
* Dead Postgres segments are recycled if found, but we do not fail upon
546-
* collision with non-Postgres shmem segments. The idea here is to detect and
547-
* re-use keys that may have been assigned by a crashed postmaster or backend.
548-
*
549-
* makePrivate means to always create a new segment, rather than attach to
550-
* or recycle any existing segment.
587+
* Dead Postgres segments pertinent to this DataDir are recycled if found, but
588+
* we do not fail upon collision with foreign shmem segments. The idea here
589+
* is to detect and re-use keys that may have been assigned by a crashed
590+
* postmaster or backend.
551591
*
552592
* The port number is passed for possible use as a key (for SysV, we use
553-
* it to generate the starting shmem key). In a standalone backend,
554-
* zero will be passed.
593+
* it to generate the starting shmem key).
555594
*/
556595
PGShmemHeader *
557-
PGSharedMemoryCreate(Size size, bool makePrivate, int port,
596+
PGSharedMemoryCreate(Size size, int port,
558597
PGShmemHeader **shim)
559598
{
560599
IpcMemoryKey NextShmemSegID;
561600
void *memAddress;
562601
PGShmemHeader *hdr;
563-
IpcMemoryId shmid;
564602
struct stat statbuf;
565603
Size sysvsize;
566604

@@ -591,70 +629,90 @@ PGSharedMemoryCreate(Size size, bool makePrivate, int port,
591629
/* Make sure PGSharedMemoryAttach doesn't fail without need */
592630
UsedShmemSegAddr = NULL;
593631

594-
/* Loop till we find a free IPC key */
595-
NextShmemSegID = port * 1000;
632+
/*
633+
* Loop till we find a free IPC key. Trust CreateDataDirLockFile() to
634+
* ensure no more than one postmaster per data directory can enter this
635+
* loop simultaneously. (CreateDataDirLockFile() does not ensure that,
636+
* but prefer fixing it over coping here.)
637+
*/
638+
NextShmemSegID = 1 + port * 1000;
596639

597-
for (NextShmemSegID++;; NextShmemSegID++)
640+
for (;;)
598641
{
642+
IpcMemoryId shmid;
643+
PGShmemHeader *oldhdr;
644+
IpcMemoryState state;
645+
599646
/* Try to create new segment */
600647
memAddress = InternalIpcMemoryCreate(NextShmemSegID, sysvsize);
601648
if (memAddress)
602649
break; /* successful create and attach */
603650

604651
/* Check shared memory and possibly remove and recreate */
605652

606-
if (makePrivate) /* a standalone backend shouldn't do this */
607-
continue;
608-
609-
if ((memAddress = PGSharedMemoryAttach(NextShmemSegID, &shmid)) == NULL)
610-
continue; /* can't attach, not one of mine */
611-
612653
/*
613-
* If I am not the creator and it belongs to an extant process,
614-
* continue.
654+
* shmget() failure is typically EACCES, hence SHMSTATE_FOREIGN.
655+
* ENOENT, a narrow possibility, implies SHMSTATE_ENOENT, but one can
656+
* safely treat SHMSTATE_ENOENT like SHMSTATE_FOREIGN.
615657
*/
616-
hdr = (PGShmemHeader *) memAddress;
617-
if (hdr->creatorPID != getpid())
658+
shmid = shmget(NextShmemSegID, sizeof(PGShmemHeader), 0);
659+
if (shmid < 0)
618660
{
619-
if (kill(hdr->creatorPID, 0) == 0 || errno != ESRCH)
620-
{
621-
shmdt(memAddress);
622-
continue; /* segment belongs to a live process */
623-
}
661+
oldhdr = NULL;
662+
state = SHMSTATE_FOREIGN;
624663
}
664+
else
665+
state = PGSharedMemoryAttach(shmid, &oldhdr);
625666

626-
/*
627-
* The segment appears to be from a dead Postgres process, or from a
628-
* previous cycle of life in this same process. Zap it, if possible,
629-
* and any associated dynamic shared memory segments, as well. This
630-
* probably shouldn't fail, but if it does, assume the segment belongs
631-
* to someone else after all, and continue quietly.
632-
*/
633-
if (hdr->dsm_control != 0)
634-
dsm_cleanup_using_control_segment(hdr->dsm_control);
635-
shmdt(memAddress);
636-
if (shmctl(shmid, IPC_RMID, NULL) < 0)
637-
continue;
667+
switch (state)
668+
{
669+
case SHMSTATE_ANALYSIS_FAILURE:
670+
case SHMSTATE_ATTACHED:
671+
ereport(FATAL,
672+
(errcode(ERRCODE_LOCK_FILE_EXISTS),
673+
errmsg("pre-existing shared memory block (key %lu, ID %lu) is still in use",
674+
(unsigned long) NextShmemSegID,
675+
(unsigned long) shmid),
676+
errhint("Terminate any old server processes associated with data directory \"%s\".",
677+
DataDir)));
678+
case SHMSTATE_ENOENT:
638679

639-
/*
640-
* Now try again to create the segment.
641-
*/
642-
memAddress = InternalIpcMemoryCreate(NextShmemSegID, sysvsize);
643-
if (memAddress)
644-
break; /* successful create and attach */
680+
/*
681+
* To our surprise, some other process deleted since our last
682+
* InternalIpcMemoryCreate(). Moments earlier, we would have
683+
* seen SHMSTATE_FOREIGN. Try that same ID again.
684+
*/
685+
elog(LOG,
686+
"shared memory block (key %lu, ID %lu) deleted during startup",
687+
(unsigned long) NextShmemSegID,
688+
(unsigned long) shmid);
689+
break;
690+
case SHMSTATE_FOREIGN:
691+
NextShmemSegID++;
692+
break;
693+
case SHMSTATE_UNATTACHED:
645694

646-
/*
647-
* Can only get here if some other process managed to create the same
648-
* shmem key before we did. Let him have that one, loop around to try
649-
* next key.
650-
*/
695+
/*
696+
* The segment pertains to DataDir, and every process that had
697+
* used it has died or detached. Zap it, if possible, and any
698+
* associated dynamic shared memory segments, as well. This
699+
* shouldn't fail, but if it does, assume the segment belongs
700+
* to someone else after all, and try the next candidate.
701+
* Otherwise, try again to create the segment. That may fail
702+
* if some other process creates the same shmem key before we
703+
* do, in which case we'll try the next key.
704+
*/
705+
if (oldhdr->dsm_control != 0)
706+
dsm_cleanup_using_control_segment(oldhdr->dsm_control);
707+
if (shmctl(shmid, IPC_RMID, NULL) < 0)
708+
NextShmemSegID++;
709+
}
710+
711+
if (oldhdr && shmdt(oldhdr) < 0)
712+
elog(LOG, "shmdt(%p) failed: %m", oldhdr);
651713
}
652714

653-
/*
654-
* OK, we created a new segment. Mark it as created by this process. The
655-
* order of assignments here is critical so that another Postgres process
656-
* can't see the header as valid but belonging to an invalid PID!
657-
*/
715+
/* Initialize new segment. */
658716
hdr = (PGShmemHeader *) memAddress;
659717
hdr->creatorPID = getpid();
660718
hdr->magic = PGShmemMagic;
@@ -714,7 +772,8 @@ void
714772
PGSharedMemoryReAttach(void)
715773
{
716774
IpcMemoryId shmid;
717-
void *hdr;
775+
PGShmemHeader *hdr;
776+
IpcMemoryState state;
718777
void *origUsedShmemSegAddr = UsedShmemSegAddr;
719778

720779
Assert(UsedShmemSegAddr != NULL);
@@ -727,14 +786,18 @@ PGSharedMemoryReAttach(void)
727786
#endif
728787

729788
elog(DEBUG3, "attaching to %p", UsedShmemSegAddr);
730-
hdr = (void *) PGSharedMemoryAttach((IpcMemoryKey) UsedShmemSegID, &shmid);
731-
if (hdr == NULL)
789+
shmid = shmget(UsedShmemSegID, sizeof(PGShmemHeader), 0);
790+
if (shmid < 0)
791+
state = SHMSTATE_FOREIGN;
792+
else
793+
state = PGSharedMemoryAttach(shmid, &hdr);
794+
if (state != SHMSTATE_ATTACHED)
732795
elog(FATAL, "could not reattach to shared memory (key=%d, addr=%p): %m",
733796
(int) UsedShmemSegID, UsedShmemSegAddr);
734797
if (hdr != origUsedShmemSegAddr)
735798
elog(FATAL, "reattaching to shared memory returned unexpected address (got %p, expected %p)",
736799
hdr, origUsedShmemSegAddr);
737-
dsm_set_control_handle(((PGShmemHeader *) hdr)->dsm_control);
800+
dsm_set_control_handle(hdr->dsm_control);
738801

739802
UsedShmemSegAddr = hdr; /* probably redundant */
740803
}
@@ -810,31 +873,3 @@ PGSharedMemoryDetach(void)
810873
}
811874
#endif
812875
}
813-
814-
815-
/*
816-
* Attach to shared memory and make sure it has a Postgres header
817-
*
818-
* Returns attach address if OK, else NULL
819-
*/
820-
static PGShmemHeader *
821-
PGSharedMemoryAttach(IpcMemoryKey key, IpcMemoryId *shmid)
822-
{
823-
PGShmemHeader *hdr;
824-
825-
if ((*shmid = shmget(key, sizeof(PGShmemHeader), 0)) < 0)
826-
return NULL;
827-
828-
hdr = (PGShmemHeader *) shmat(*shmid, UsedShmemSegAddr, PG_SHMAT_FLAGS);
829-
830-
if (hdr == (PGShmemHeader *) -1)
831-
return NULL; /* failed: must be some other app's */
832-
833-
if (hdr->magic != PGShmemMagic)
834-
{
835-
shmdt((void *) hdr);
836-
return NULL; /* segment belongs to a non-Postgres app */
837-
}
838-
839-
return hdr;
840-
}

0 commit comments

Comments
 (0)