Skip to content

Commit d1cbd26

Browse files
tglsfdctglsfdc
committed
Repair two places where SIGTERM exit could leave shared memory state
corrupted. (Neither is very important if SIGTERM is used to shut down the whole database cluster together, but there's a problem if someone tries to SIGTERM individual backends.) To do this, introduce new infrastructure macros PG_ENSURE_ERROR_CLEANUP/PG_END_ENSURE_ERROR_CLEANUP that take care of transiently pushing an on_shmem_exit cleanup hook. Also use this method for createdb cleanup --- that wasn't a shared-memory-corruption problem, but SIGTERM abort of createdb could leave orphaned files lying around. Backpatch as far as 8.2. The shmem corruption cases don't exist in 8.1, and the createdb usage doesn't seem important enough to risk backpatching further.
1 parent 74be868 commit d1cbd26

File tree

9 files changed

+153
-57
lines changed

9 files changed

+153
-57
lines changed

‎src/backend/access/nbtree/nbtree.c

Lines changed: 6 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@
1212
* Portions Copyright (c) 1994, Regents of the University of California
1313
*
1414
* IDENTIFICATION
15-
* $PostgreSQL: pgsql/src/backend/access/nbtree/nbtree.c,v 1.158 2008/04/13 19:18:14 tgl Exp $
15+
* $PostgreSQL: pgsql/src/backend/access/nbtree/nbtree.c,v 1.159 2008/04/16 23:59:39 tgl Exp $
1616
*
1717
*-------------------------------------------------------------------------
1818
*/
@@ -24,6 +24,7 @@
2424
#include "commands/vacuum.h"
2525
#include "miscadmin.h"
2626
#include "storage/freespace.h"
27+
#include "storage/ipc.h"
2728
#include "storage/lmgr.h"
2829
#include "utils/memutils.h"
2930

@@ -530,21 +531,15 @@ btbulkdelete(PG_FUNCTION_ARGS)
530531
stats = (IndexBulkDeleteResult *) palloc0(sizeof(IndexBulkDeleteResult));
531532

532533
/* Establish the vacuum cycle ID to use for this scan */
533-
PG_TRY();
534+
/* The ENSURE stuff ensures we clean up shared memory on failure */
535+
PG_ENSURE_ERROR_CLEANUP(_bt_end_vacuum_callback, PointerGetDatum(rel));
534536
{
535537
cycleid = _bt_start_vacuum(rel);
536538

537539
btvacuumscan(info, stats, callback, callback_state, cycleid);
538-
539-
_bt_end_vacuum(rel);
540-
}
541-
PG_CATCH();
542-
{
543-
/* Make sure shared memory gets cleaned up */
544-
_bt_end_vacuum(rel);
545-
PG_RE_THROW();
546540
}
547-
PG_END_TRY();
541+
PG_END_ENSURE_ERROR_CLEANUP(_bt_end_vacuum_callback, PointerGetDatum(rel));
542+
_bt_end_vacuum(rel);
548543

549544
PG_RETURN_POINTER(stats);
550545
}

‎src/backend/access/nbtree/nbtutils.c

Lines changed: 15 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@
88
*
99
*
1010
* IDENTIFICATION
11-
* $PostgreSQL: pgsql/src/backend/access/nbtree/nbtutils.c,v 1.88 2008/01/01 19:45:46 momjian Exp $
11+
* $PostgreSQL: pgsql/src/backend/access/nbtree/nbtutils.c,v 1.89 2008/04/16 23:59:40 tgl Exp $
1212
*
1313
*-------------------------------------------------------------------------
1414
*/
@@ -1252,8 +1252,11 @@ _bt_vacuum_cycleid(Relation rel)
12521252
/*
12531253
* _bt_start_vacuum --- assign a cycle ID to a just-starting VACUUM operation
12541254
*
1255-
* Note: the caller must guarantee (via PG_TRY) that it will eventually call
1256-
* _bt_end_vacuum, else we'll permanently leak an array slot.
1255+
* Note: the caller must guarantee that it will eventually call
1256+
* _bt_end_vacuum, else we'll permanently leak an array slot. To ensure
1257+
* that this happens even in elog(FATAL) scenarios, the appropriate coding
1258+
* is not just a PG_TRY, but
1259+
* PG_ENSURE_ERROR_CLEANUP(_bt_end_vacuum_callback, PointerGetDatum(rel))
12571260
*/
12581261
BTCycleId
12591262
_bt_start_vacuum(Relation rel)
@@ -1337,6 +1340,15 @@ _bt_end_vacuum(Relation rel)
13371340
LWLockRelease(BtreeVacuumLock);
13381341
}
13391342

1343+
/*
1344+
* _bt_end_vacuum wrapped as an on_shmem_exit callback function
1345+
*/
1346+
void
1347+
_bt_end_vacuum_callback(int code, Datum arg)
1348+
{
1349+
_bt_end_vacuum((Relation) DatumGetPointer(arg));
1350+
}
1351+
13401352
/*
13411353
* BTreeShmemSize --- report amount of shared memory space needed
13421354
*/

‎src/backend/access/transam/xlog.c

Lines changed: 17 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@
77
* Portions Copyright (c) 1996-2008, PostgreSQL Global Development Group
88
* Portions Copyright (c) 1994, Regents of the University of California
99
*
10-
* $PostgreSQL: pgsql/src/backend/access/transam/xlog.c,v 1.296 2008/04/05 01:34:06 momjian Exp $
10+
* $PostgreSQL: pgsql/src/backend/access/transam/xlog.c,v 1.297 2008/04/16 23:59:40 tgl Exp $
1111
*
1212
*-------------------------------------------------------------------------
1313
*/
@@ -43,6 +43,7 @@
4343
#include "postmaster/bgwriter.h"
4444
#include "storage/bufpage.h"
4545
#include "storage/fd.h"
46+
#include "storage/ipc.h"
4647
#include "storage/pmsignal.h"
4748
#include "storage/procarray.h"
4849
#include "storage/smgr.h"
@@ -419,11 +420,11 @@ static void writeTimeLineHistory(TimeLineID newTLI, TimeLineID parentTLI,
419420
static void WriteControlFile(void);
420421
static void ReadControlFile(void);
421422
static char *str_time(pg_time_t tnow);
422-
static void issue_xlog_fsync(void);
423-
424423
#ifdef WAL_DEBUG
425424
static void xlog_outrec(StringInfo buf, XLogRecord *record);
426425
#endif
426+
static void issue_xlog_fsync(void);
427+
static void pg_start_backup_callback(int code, Datum arg);
427428
static bool read_backup_label(XLogRecPtr *checkPointLoc,
428429
XLogRecPtr *minRecoveryLoc);
429430
static void rm_redo_error_callback(void *arg);
@@ -6442,8 +6443,8 @@ pg_start_backup(PG_FUNCTION_ARGS)
64426443
XLogCtl->Insert.forcePageWrites = true;
64436444
LWLockRelease(WALInsertLock);
64446445

6445-
/* Use a TRY block to ensure we release forcePageWrites if fail below */
6446-
PG_TRY();
6446+
/* Ensure we release forcePageWrites if fail below */
6447+
PG_ENSURE_ERROR_CLEANUP(pg_start_backup_callback, (Datum) 0);
64476448
{
64486449
/*
64496450
* Force a CHECKPOINT. Aside from being necessary to prevent torn
@@ -6515,16 +6516,7 @@ pg_start_backup(PG_FUNCTION_ARGS)
65156516
errmsg("could not write file \"%s\": %m",
65166517
BACKUP_LABEL_FILE)));
65176518
}
6518-
PG_CATCH();
6519-
{
6520-
/* Turn off forcePageWrites on failure */
6521-
LWLockAcquire(WALInsertLock, LW_EXCLUSIVE);
6522-
XLogCtl->Insert.forcePageWrites = false;
6523-
LWLockRelease(WALInsertLock);
6524-
6525-
PG_RE_THROW();
6526-
}
6527-
PG_END_TRY();
6519+
PG_END_ENSURE_ERROR_CLEANUP(pg_start_backup_callback, (Datum) 0);
65286520

65296521
/*
65306522
* We're done. As a convenience, return the starting WAL location.
@@ -6534,6 +6526,16 @@ pg_start_backup(PG_FUNCTION_ARGS)
65346526
PG_RETURN_TEXT_P(cstring_to_text(xlogfilename));
65356527
}
65366528

6529+
/* Error cleanup callback for pg_start_backup */
6530+
static void
6531+
pg_start_backup_callback(int code, Datum arg)
6532+
{
6533+
/* Turn off forcePageWrites on failure */
6534+
LWLockAcquire(WALInsertLock, LW_EXCLUSIVE);
6535+
XLogCtl->Insert.forcePageWrites = false;
6536+
LWLockRelease(WALInsertLock);
6537+
}
6538+
65376539
/*
65386540
* pg_stop_backup: finish taking an on-line backup dump
65396541
*

‎src/backend/commands/dbcommands.c

Lines changed: 32 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@
1313
*
1414
*
1515
* IDENTIFICATION
16-
* $PostgreSQL: pgsql/src/backend/commands/dbcommands.c,v 1.205 2008/03/26 21:10:37 alvherre Exp $
16+
* $PostgreSQL: pgsql/src/backend/commands/dbcommands.c,v 1.206 2008/04/16 23:59:40 tgl Exp $
1717
*
1818
*-------------------------------------------------------------------------
1919
*/
@@ -41,6 +41,7 @@
4141
#include "pgstat.h"
4242
#include "postmaster/bgwriter.h"
4343
#include "storage/freespace.h"
44+
#include "storage/ipc.h"
4445
#include "storage/procarray.h"
4546
#include "storage/smgr.h"
4647
#include "utils/acl.h"
@@ -53,7 +54,14 @@
5354
#include "utils/tqual.h"
5455

5556

57+
typedef struct
58+
{
59+
Oid src_dboid; /* source (template) DB */
60+
Oid dest_dboid; /* DB we are trying to create */
61+
} createdb_failure_params;
62+
5663
/* non-export function prototypes */
64+
static void createdb_failure_callback(int code, Datum arg);
5765
static bool get_db_info(const char *name, LOCKMODE lockmode,
5866
Oid *dbIdP, Oid *ownerIdP,
5967
int *encodingP, bool *dbIsTemplateP, bool *dbAllowConnP,
@@ -99,6 +107,7 @@ createdb(const CreatedbStmt *stmt)
99107
int encoding = -1;
100108
int dbconnlimit = -1;
101109
int ctype_encoding;
110+
createdb_failure_params fparms;
102111

103112
/* Extract options from the statement node tree */
104113
foreach(option, stmt->options)
@@ -449,12 +458,15 @@ createdb(const CreatedbStmt *stmt)
449458

450459
/*
451460
* Once we start copying subdirectories, we need to be able to clean 'em
452-
* up if we fail. Establish a TRY block to make sure this happens. (This
461+
* up if we fail. Use an ENSURE block to make sure this happens. (This
453462
* is not a 100% solution, because of the possibility of failure during
454463
* transaction commit after we leave this routine, but it should handle
455464
* most scenarios.)
456465
*/
457-
PG_TRY();
466+
fparms.src_dboid = src_dboid;
467+
fparms.dest_dboid = dboid;
468+
PG_ENSURE_ERROR_CLEANUP(createdb_failure_callback,
469+
PointerGetDatum(&fparms));
458470
{
459471
/*
460472
* Iterate through all tablespaces of the template database, and copy
@@ -565,18 +577,25 @@ createdb(const CreatedbStmt *stmt)
565577
*/
566578
database_file_update_needed();
567579
}
568-
PG_CATCH();
569-
{
570-
/* Release lock on source database before doing recursive remove */
571-
UnlockSharedObject(DatabaseRelationId, src_dboid, 0,
572-
ShareLock);
580+
PG_END_ENSURE_ERROR_CLEANUP(createdb_failure_callback,
581+
PointerGetDatum(&fparms));
582+
}
583+
584+
/* Error cleanup callback for createdb */
585+
static void
586+
createdb_failure_callback(int code, Datum arg)
587+
{
588+
createdb_failure_params *fparms = (createdb_failure_params *) DatumGetPointer(arg);
573589

574-
/* Throw away any successfully copied subdirectories */
575-
remove_dbtablespaces(dboid);
590+
/*
591+
* Release lock on source database before doing recursive remove.
592+
* This is not essential but it seems desirable to release the lock
593+
* as soon as possible.
594+
*/
595+
UnlockSharedObject(DatabaseRelationId, fparms->src_dboid, 0, ShareLock);
576596

577-
PG_RE_THROW();
578-
}
579-
PG_END_TRY();
597+
/* Throw away any successfully copied subdirectories */
598+
remove_dbtablespaces(fparms->dest_dboid);
580599
}
581600

582601

‎src/backend/port/ipc_test.c

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@
2121
*
2222
*
2323
* IDENTIFICATION
24-
* $PostgreSQL: pgsql/src/backend/port/ipc_test.c,v 1.24 2008/03/24 18:08:47 tgl Exp $
24+
* $PostgreSQL: pgsql/src/backend/port/ipc_test.c,v 1.25 2008/04/16 23:59:40 tgl Exp $
2525
*
2626
*-------------------------------------------------------------------------
2727
*/
@@ -58,7 +58,7 @@ char *DataDir = ".";
5858

5959
static struct ONEXIT
6060
{
61-
void (*function) (int code, Datum arg);
61+
pg_on_exit_callback function;
6262
Datum arg;
6363
} on_proc_exit_list[MAX_ON_EXITS], on_shmem_exit_list[MAX_ON_EXITS];
6464

@@ -85,7 +85,7 @@ shmem_exit(int code)
8585
}
8686

8787
void
88-
on_shmem_exit(void (*function) (int code, Datum arg), Datum arg)
88+
on_shmem_exit(pg_on_exit_callback function, Datum arg)
8989
{
9090
if (on_shmem_exit_index >= MAX_ON_EXITS)
9191
elog(FATAL, "out of on_shmem_exit slots");
@@ -114,9 +114,9 @@ ProcessInterrupts(void)
114114
}
115115

116116
int
117-
ExceptionalCondition(char *conditionName,
118-
char *errorType,
119-
char *fileName,
117+
ExceptionalCondition(const char *conditionName,
118+
const char *errorType,
119+
const char *fileName,
120120
int lineNumber)
121121
{
122122
fprintf(stderr, "TRAP: %s(\"%s\", File: \"%s\", Line: %d)\n",

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

Lines changed: 22 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@
1313
*
1414
*
1515
* IDENTIFICATION
16-
* $PostgreSQL: pgsql/src/backend/storage/ipc/ipc.c,v 1.100 2008/01/01 19:45:51 momjian Exp $
16+
* $PostgreSQL: pgsql/src/backend/storage/ipc/ipc.c,v 1.101 2008/04/16 23:59:40 tgl Exp $
1717
*
1818
*-------------------------------------------------------------------------
1919
*/
@@ -56,7 +56,7 @@ bool proc_exit_inprogress = false;
5656

5757
static struct ONEXIT
5858
{
59-
void (*function) (int code, Datum arg);
59+
pg_on_exit_callback function;
6060
Datum arg;
6161
} on_proc_exit_list[MAX_ON_EXITS], on_shmem_exit_list[MAX_ON_EXITS];
6262

@@ -184,7 +184,7 @@ shmem_exit(int code)
184184
* ----------------------------------------------------------------
185185
*/
186186
void
187-
on_proc_exit(void (*function) (int code, Datum arg), Datum arg)
187+
on_proc_exit(pg_on_exit_callback function, Datum arg)
188188
{
189189
if (on_proc_exit_index >= MAX_ON_EXITS)
190190
ereport(FATAL,
@@ -205,7 +205,7 @@ void
205205
* ----------------------------------------------------------------
206206
*/
207207
void
208-
on_shmem_exit(void (*function) (int code, Datum arg), Datum arg)
208+
on_shmem_exit(pg_on_exit_callback function, Datum arg)
209209
{
210210
if (on_shmem_exit_index >= MAX_ON_EXITS)
211211
ereport(FATAL,
@@ -218,6 +218,24 @@ void
218218
++on_shmem_exit_index;
219219
}
220220

221+
/* ----------------------------------------------------------------
222+
* cancel_shmem_exit
223+
*
224+
* this function removes an entry, if present, from the list of
225+
* functions to be invoked by shmem_exit(). For simplicity,
226+
* only the latest entry can be removed. (We could work harder
227+
* but there is no need for current uses.)
228+
* ----------------------------------------------------------------
229+
*/
230+
void
231+
cancel_shmem_exit(pg_on_exit_callback function, Datum arg)
232+
{
233+
if (on_shmem_exit_index > 0 &&
234+
on_shmem_exit_list[on_shmem_exit_index - 1].function == function &&
235+
on_shmem_exit_list[on_shmem_exit_index - 1].arg == arg)
236+
--on_shmem_exit_index;
237+
}
238+
221239
/* ----------------------------------------------------------------
222240
* on_exit_reset
223241
*

‎src/include/access/nbtree.h

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@
77
* Portions Copyright (c) 1996-2008, PostgreSQL Global Development Group
88
* Portions Copyright (c) 1994, Regents of the University of California
99
*
10-
* $PostgreSQL: pgsql/src/include/access/nbtree.h,v 1.117 2008/04/10 22:25:25 tgl Exp $
10+
* $PostgreSQL: pgsql/src/include/access/nbtree.h,v 1.118 2008/04/16 23:59:40 tgl Exp $
1111
*
1212
*-------------------------------------------------------------------------
1313
*/
@@ -574,6 +574,7 @@ extern void _bt_killitems(IndexScanDesc scan, bool haveLock);
574574
extern BTCycleId _bt_vacuum_cycleid(Relation rel);
575575
extern BTCycleId _bt_start_vacuum(Relation rel);
576576
extern void _bt_end_vacuum(Relation rel);
577+
extern void _bt_end_vacuum_callback(int code, Datum arg);
577578
extern Size BTreeShmemSize(void);
578579
extern void BTreeShmemInit(void);
579580

0 commit comments

Comments
 (0)