]> granicus.if.org Git - postgresql/commitdiff
Repair two places where SIGTERM exit could leave shared memory state
authorTom Lane <tgl@sss.pgh.pa.us>
Wed, 16 Apr 2008 23:59:51 +0000 (23:59 +0000)
committerTom Lane <tgl@sss.pgh.pa.us>
Wed, 16 Apr 2008 23:59:51 +0000 (23:59 +0000)
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.

src/backend/access/nbtree/nbtree.c
src/backend/access/nbtree/nbtutils.c
src/backend/access/transam/xlog.c
src/backend/commands/dbcommands.c
src/backend/port/ipc_test.c
src/backend/storage/ipc/ipc.c
src/include/access/nbtree.h
src/include/storage/ipc.h
src/include/utils/elog.h

index 5cfa8f315d79ee09a153c4ab72750113f62deb4d..d547b910528cd01c4ba87eadf178c9dc12827cc6 100644 (file)
@@ -12,7 +12,7 @@
  * Portions Copyright (c) 1994, Regents of the University of California
  *
  * IDENTIFICATION
- *       $PostgreSQL: pgsql/src/backend/access/nbtree/nbtree.c,v 1.156 2008/01/01 19:45:46 momjian Exp $
+ *       $PostgreSQL: pgsql/src/backend/access/nbtree/nbtree.c,v 1.156.2.1 2008/04/16 23:59:51 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -23,6 +23,7 @@
 #include "catalog/index.h"
 #include "commands/vacuum.h"
 #include "storage/freespace.h"
+#include "storage/ipc.h"
 #include "storage/lmgr.h"
 #include "utils/memutils.h"
 
@@ -538,21 +539,15 @@ btbulkdelete(PG_FUNCTION_ARGS)
                stats = (IndexBulkDeleteResult *) palloc0(sizeof(IndexBulkDeleteResult));
 
        /* Establish the vacuum cycle ID to use for this scan */
-       PG_TRY();
+       /* The ENSURE stuff ensures we clean up shared memory on failure */
+       PG_ENSURE_ERROR_CLEANUP(_bt_end_vacuum_callback, PointerGetDatum(rel));
        {
                cycleid = _bt_start_vacuum(rel);
 
                btvacuumscan(info, stats, callback, callback_state, cycleid);
-
-               _bt_end_vacuum(rel);
-       }
-       PG_CATCH();
-       {
-               /* Make sure shared memory gets cleaned up */
-               _bt_end_vacuum(rel);
-               PG_RE_THROW();
        }
-       PG_END_TRY();
+       PG_END_ENSURE_ERROR_CLEANUP(_bt_end_vacuum_callback, PointerGetDatum(rel));
+       _bt_end_vacuum(rel);
 
        PG_RETURN_POINTER(stats);
 }
index 6f88a34488e2f050cf3fd2df24f3777421aa137b..3db75d4b992e7b3970916b973436a4f963bff021 100644 (file)
@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *       $PostgreSQL: pgsql/src/backend/access/nbtree/nbtutils.c,v 1.88 2008/01/01 19:45:46 momjian Exp $
+ *       $PostgreSQL: pgsql/src/backend/access/nbtree/nbtutils.c,v 1.88.2.1 2008/04/16 23:59:51 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -1252,8 +1252,11 @@ _bt_vacuum_cycleid(Relation rel)
 /*
  * _bt_start_vacuum --- assign a cycle ID to a just-starting VACUUM operation
  *
- * Note: the caller must guarantee (via PG_TRY) that it will eventually call
- * _bt_end_vacuum, else we'll permanently leak an array slot.
+ * Note: the caller must guarantee that it will eventually call
+ * _bt_end_vacuum, else we'll permanently leak an array slot.  To ensure
+ * that this happens even in elog(FATAL) scenarios, the appropriate coding
+ * is not just a PG_TRY, but
+ *             PG_ENSURE_ERROR_CLEANUP(_bt_end_vacuum_callback, PointerGetDatum(rel))
  */
 BTCycleId
 _bt_start_vacuum(Relation rel)
@@ -1337,6 +1340,15 @@ _bt_end_vacuum(Relation rel)
        LWLockRelease(BtreeVacuumLock);
 }
 
+/*
+ * _bt_end_vacuum wrapped as an on_shmem_exit callback function
+ */
+void
+_bt_end_vacuum_callback(int code, Datum arg)
+{
+       _bt_end_vacuum((Relation) DatumGetPointer(arg));
+}
+
 /*
  * BTreeShmemSize --- report amount of shared memory space needed
  */
index d2075018d96d0547dcb3bb547897aaec67c32d4c..d0cce7f210e4369895717b203e688b8ddc6bb3f9 100644 (file)
@@ -7,7 +7,7 @@
  * Portions Copyright (c) 1996-2008, PostgreSQL Global Development Group
  * Portions Copyright (c) 1994, Regents of the University of California
  *
- * $PostgreSQL: pgsql/src/backend/access/transam/xlog.c,v 1.292 2008/01/21 11:17:46 petere Exp $
+ * $PostgreSQL: pgsql/src/backend/access/transam/xlog.c,v 1.292.2.1 2008/04/16 23:59:51 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -43,6 +43,7 @@
 #include "postmaster/bgwriter.h"
 #include "storage/bufpage.h"
 #include "storage/fd.h"
+#include "storage/ipc.h"
 #include "storage/pmsignal.h"
 #include "storage/procarray.h"
 #include "storage/smgr.h"
@@ -420,11 +421,11 @@ static void writeTimeLineHistory(TimeLineID newTLI, TimeLineID parentTLI,
 static void WriteControlFile(void);
 static void ReadControlFile(void);
 static char *str_time(pg_time_t tnow);
-static void issue_xlog_fsync(void);
-
 #ifdef WAL_DEBUG
 static void xlog_outrec(StringInfo buf, XLogRecord *record);
 #endif
+static void issue_xlog_fsync(void);
+static void pg_start_backup_callback(int code, Datum arg);
 static bool read_backup_label(XLogRecPtr *checkPointLoc,
                                  XLogRecPtr *minRecoveryLoc);
 static void rm_redo_error_callback(void *arg);
@@ -6445,8 +6446,8 @@ pg_start_backup(PG_FUNCTION_ARGS)
        XLogCtl->Insert.forcePageWrites = true;
        LWLockRelease(WALInsertLock);
 
-       /* Use a TRY block to ensure we release forcePageWrites if fail below */
-       PG_TRY();
+       /* Ensure we release forcePageWrites if fail below */
+       PG_ENSURE_ERROR_CLEANUP(pg_start_backup_callback, (Datum) 0);
        {
                /*
                 * Force a CHECKPOINT.  Aside from being necessary to prevent torn
@@ -6518,16 +6519,7 @@ pg_start_backup(PG_FUNCTION_ARGS)
                                         errmsg("could not write file \"%s\": %m",
                                                        BACKUP_LABEL_FILE)));
        }
-       PG_CATCH();
-       {
-               /* Turn off forcePageWrites on failure */
-               LWLockAcquire(WALInsertLock, LW_EXCLUSIVE);
-               XLogCtl->Insert.forcePageWrites = false;
-               LWLockRelease(WALInsertLock);
-
-               PG_RE_THROW();
-       }
-       PG_END_TRY();
+       PG_END_ENSURE_ERROR_CLEANUP(pg_start_backup_callback, (Datum) 0);
 
        /*
         * We're done.  As a convenience, return the starting WAL location.
@@ -6539,6 +6531,16 @@ pg_start_backup(PG_FUNCTION_ARGS)
        PG_RETURN_TEXT_P(result);
 }
 
+/* Error cleanup callback for pg_start_backup */
+static void
+pg_start_backup_callback(int code, Datum arg)
+{
+       /* Turn off forcePageWrites on failure */
+       LWLockAcquire(WALInsertLock, LW_EXCLUSIVE);
+       XLogCtl->Insert.forcePageWrites = false;
+       LWLockRelease(WALInsertLock);
+}
+
 /*
  * pg_stop_backup: finish taking an on-line backup dump
  *
index afd74af48b646a756bb8508f25dfcbd8b030b937..8b99fd11ba43c38d0f5da09cf50da2bf13f99519 100644 (file)
@@ -13,7 +13,7 @@
  *
  *
  * IDENTIFICATION
- *       $PostgreSQL: pgsql/src/backend/commands/dbcommands.c,v 1.204 2008/01/01 19:45:48 momjian Exp $
+ *       $PostgreSQL: pgsql/src/backend/commands/dbcommands.c,v 1.204.2.1 2008/04/16 23:59:51 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -41,6 +41,7 @@
 #include "pgstat.h"
 #include "postmaster/bgwriter.h"
 #include "storage/freespace.h"
+#include "storage/ipc.h"
 #include "storage/procarray.h"
 #include "storage/smgr.h"
 #include "utils/acl.h"
 #include "utils/syscache.h"
 
 
+typedef struct
+{
+       Oid                     src_dboid;              /* source (template) DB */
+       Oid                     dest_dboid;             /* DB we are trying to create */
+} createdb_failure_params;
+
 /* non-export function prototypes */
+static void createdb_failure_callback(int code, Datum arg);
 static bool get_db_info(const char *name, LOCKMODE lockmode,
                        Oid *dbIdP, Oid *ownerIdP,
                        int *encodingP, bool *dbIsTemplateP, bool *dbAllowConnP,
@@ -98,6 +106,7 @@ createdb(const CreatedbStmt *stmt)
        int                     encoding = -1;
        int                     dbconnlimit = -1;
        int                     ctype_encoding;
+       createdb_failure_params fparms;
 
        /* Extract options from the statement node tree */
        foreach(option, stmt->options)
@@ -448,12 +457,15 @@ createdb(const CreatedbStmt *stmt)
 
        /*
         * Once we start copying subdirectories, we need to be able to clean 'em
-        * up if we fail.  Establish a TRY block to make sure this happens. (This
+        * up if we fail.  Use an ENSURE block to make sure this happens.  (This
         * is not a 100% solution, because of the possibility of failure during
         * transaction commit after we leave this routine, but it should handle
         * most scenarios.)
         */
-       PG_TRY();
+       fparms.src_dboid = src_dboid;
+       fparms.dest_dboid = dboid;
+       PG_ENSURE_ERROR_CLEANUP(createdb_failure_callback,
+                                                       PointerGetDatum(&fparms));
        {
                /*
                 * Iterate through all tablespaces of the template database, and copy
@@ -564,18 +576,25 @@ createdb(const CreatedbStmt *stmt)
                 */
                database_file_update_needed();
        }
-       PG_CATCH();
-       {
-               /* Release lock on source database before doing recursive remove */
-               UnlockSharedObject(DatabaseRelationId, src_dboid, 0,
-                                                  ShareLock);
+       PG_END_ENSURE_ERROR_CLEANUP(createdb_failure_callback,
+                                                               PointerGetDatum(&fparms));
+}
+
+/* Error cleanup callback for createdb */
+static void
+createdb_failure_callback(int code, Datum arg)
+{
+       createdb_failure_params *fparms = (createdb_failure_params *) DatumGetPointer(arg);
 
-               /* Throw away any successfully copied subdirectories */
-               remove_dbtablespaces(dboid);
+       /*
+        * Release lock on source database before doing recursive remove.
+        * This is not essential but it seems desirable to release the lock
+        * as soon as possible.
+        */
+       UnlockSharedObject(DatabaseRelationId, fparms->src_dboid, 0, ShareLock);
 
-               PG_RE_THROW();
-       }
-       PG_END_TRY();
+       /* Throw away any successfully copied subdirectories */
+       remove_dbtablespaces(fparms->dest_dboid);
 }
 
 
index fa5066fd7e8a09d03a0e03d1cbeb909d2a0500f4..35862db2d383e94c3190f03a5e1385062f0d3989 100644 (file)
@@ -21,7 +21,7 @@
  *
  *
  * IDENTIFICATION
- *       $PostgreSQL: pgsql/src/backend/port/ipc_test.c,v 1.23 2008/01/01 19:45:51 momjian Exp $
+ *       $PostgreSQL: pgsql/src/backend/port/ipc_test.c,v 1.23.2.1 2008/04/16 23:59:51 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -58,7 +58,7 @@ char     *DataDir = ".";
 
 static struct ONEXIT
 {
-       void            (*function) (int code, Datum arg);
+       pg_on_exit_callback function;
        Datum           arg;
 }      on_proc_exit_list[MAX_ON_EXITS], on_shmem_exit_list[MAX_ON_EXITS];
 
@@ -85,7 +85,7 @@ shmem_exit(int code)
 }
 
 void
-                       on_shmem_exit(void (*function) (int code, Datum arg), Datum arg)
+on_shmem_exit(pg_on_exit_callback function, Datum arg)
 {
        if (on_shmem_exit_index >= MAX_ON_EXITS)
                elog(FATAL, "out of on_shmem_exit slots");
@@ -114,9 +114,9 @@ ProcessInterrupts(void)
 }
 
 int
-ExceptionalCondition(char *conditionName,
-                                        char *errorType,
-                                        char *fileName,
+ExceptionalCondition(const char *conditionName,
+                                        const char *errorType,
+                                        const char *fileName,
                                         int lineNumber)
 {
        fprintf(stderr, "TRAP: %s(\"%s\", File: \"%s\", Line: %d)\n",
index 91fbea2ea4fbfce376834c66b051fa1d076f8c2c..dfe7161f42e247b497c1c45145f22356ac07b048 100644 (file)
@@ -13,7 +13,7 @@
  *
  *
  * IDENTIFICATION
- *       $PostgreSQL: pgsql/src/backend/storage/ipc/ipc.c,v 1.100 2008/01/01 19:45:51 momjian Exp $
+ *       $PostgreSQL: pgsql/src/backend/storage/ipc/ipc.c,v 1.100.2.1 2008/04/16 23:59:51 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -56,7 +56,7 @@ bool          proc_exit_inprogress = false;
 
 static struct ONEXIT
 {
-       void            (*function) (int code, Datum arg);
+       pg_on_exit_callback function;
        Datum           arg;
 }      on_proc_exit_list[MAX_ON_EXITS], on_shmem_exit_list[MAX_ON_EXITS];
 
@@ -184,7 +184,7 @@ shmem_exit(int code)
  * ----------------------------------------------------------------
  */
 void
-                       on_proc_exit(void (*function) (int code, Datum arg), Datum arg)
+on_proc_exit(pg_on_exit_callback function, Datum arg)
 {
        if (on_proc_exit_index >= MAX_ON_EXITS)
                ereport(FATAL,
@@ -205,7 +205,7 @@ void
  * ----------------------------------------------------------------
  */
 void
-                       on_shmem_exit(void (*function) (int code, Datum arg), Datum arg)
+on_shmem_exit(pg_on_exit_callback function, Datum arg)
 {
        if (on_shmem_exit_index >= MAX_ON_EXITS)
                ereport(FATAL,
@@ -218,6 +218,24 @@ void
        ++on_shmem_exit_index;
 }
 
+/* ----------------------------------------------------------------
+ *             cancel_shmem_exit
+ *
+ *             this function removes an entry, if present, from the list of
+ *             functions to be invoked by shmem_exit().  For simplicity,
+ *             only the latest entry can be removed.  (We could work harder
+ *             but there is no need for current uses.)
+ * ----------------------------------------------------------------
+ */
+void
+cancel_shmem_exit(pg_on_exit_callback function, Datum arg)
+{
+       if (on_shmem_exit_index > 0 &&
+               on_shmem_exit_list[on_shmem_exit_index - 1].function == function &&
+               on_shmem_exit_list[on_shmem_exit_index - 1].arg == arg)
+               --on_shmem_exit_index;
+}
+
 /* ----------------------------------------------------------------
  *             on_exit_reset
  *
index 2bcf8ab38f417351f6c2dcabd87ff18819b27e1c..fc8b30b2ea91a001798577c8e0f2a5af0e43a916 100644 (file)
@@ -7,7 +7,7 @@
  * Portions Copyright (c) 1996-2008, PostgreSQL Global Development Group
  * Portions Copyright (c) 1994, Regents of the University of California
  *
- * $PostgreSQL: pgsql/src/include/access/nbtree.h,v 1.116 2008/01/01 19:45:56 momjian Exp $
+ * $PostgreSQL: pgsql/src/include/access/nbtree.h,v 1.116.2.1 2008/04/16 23:59:51 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -574,6 +574,7 @@ extern void _bt_killitems(IndexScanDesc scan, bool haveLock);
 extern BTCycleId _bt_vacuum_cycleid(Relation rel);
 extern BTCycleId _bt_start_vacuum(Relation rel);
 extern void _bt_end_vacuum(Relation rel);
+extern void _bt_end_vacuum_callback(int code, Datum arg);
 extern Size BTreeShmemSize(void);
 extern void BTreeShmemInit(void);
 
index eb89f9a7daebac1383dacc2e5d40d78c67e557e4..8f4bbcb90eb292f23e8d0d131c5ec5f44490df3d 100644 (file)
  * Portions Copyright (c) 1996-2008, PostgreSQL Global Development Group
  * Portions Copyright (c) 1994, Regents of the University of California
  *
- * $PostgreSQL: pgsql/src/include/storage/ipc.h,v 1.74 2008/01/01 19:45:59 momjian Exp $
+ * $PostgreSQL: pgsql/src/include/storage/ipc.h,v 1.74.2.1 2008/04/16 23:59:51 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
 #ifndef IPC_H
 #define IPC_H
 
+typedef void (*pg_on_exit_callback) (int code, Datum arg);
+
+/*----------
+ * API for handling cleanup that must occur during either ereport(ERROR)
+ * or ereport(FATAL) exits from a block of code.  (Typical examples are
+ * undoing transient changes to shared-memory state.)
+ *
+ *             PG_ENSURE_ERROR_CLEANUP(cleanup_function, arg);
+ *             {
+ *                     ... code that might throw ereport(ERROR) or ereport(FATAL) ...
+ *             }
+ *             PG_END_ENSURE_ERROR_CLEANUP(cleanup_function, arg);
+ *
+ * where the cleanup code is in a function declared per pg_on_exit_callback.
+ * The Datum value "arg" can carry any information the cleanup function
+ * needs.
+ *
+ * This construct ensures that cleanup_function() will be called during
+ * either ERROR or FATAL exits.  It will not be called on successful
+ * exit from the controlled code.  (If you want it to happen then too,
+ * call the function yourself from just after the construct.)
+ *
+ * Note: the macro arguments are multiply evaluated, so avoid side-effects.
+ *----------
+ */
+#define PG_ENSURE_ERROR_CLEANUP(cleanup_function, arg)  \
+       do { \
+               on_shmem_exit(cleanup_function, arg); \
+               PG_TRY()
+
+#define PG_END_ENSURE_ERROR_CLEANUP(cleanup_function, arg)  \
+               cancel_shmem_exit(cleanup_function, arg); \
+               PG_CATCH(); \
+               { \
+                       cancel_shmem_exit(cleanup_function, arg); \
+                       cleanup_function (0, arg); \
+                       PG_RE_THROW(); \
+               } \
+               PG_END_TRY(); \
+       } while (0)
+
 
 /* ipc.c */
 extern bool proc_exit_inprogress;
 
 extern void proc_exit(int code);
 extern void shmem_exit(int code);
-extern void on_proc_exit(void (*function) (int code, Datum arg), Datum arg);
-extern void on_shmem_exit(void (*function) (int code, Datum arg), Datum arg);
+extern void on_proc_exit(pg_on_exit_callback function, Datum arg);
+extern void on_shmem_exit(pg_on_exit_callback function, Datum arg);
+extern void cancel_shmem_exit(pg_on_exit_callback function, Datum arg);
 extern void on_exit_reset(void);
 
 /* ipci.c */
index 57ca1533928715b705af83c9ecbbbe29320d7df1..04843eecd300fe2ceb49b703aff79853896922f0 100644 (file)
@@ -7,7 +7,7 @@
  * Portions Copyright (c) 1996-2008, PostgreSQL Global Development Group
  * Portions Copyright (c) 1994, Regents of the University of California
  *
- * $PostgreSQL: pgsql/src/include/utils/elog.h,v 1.90 2008/01/01 19:45:59 momjian Exp $
+ * $PostgreSQL: pgsql/src/include/utils/elog.h,v 1.90.2.1 2008/04/16 23:59:51 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -198,6 +198,13 @@ extern PGDLLIMPORT ErrorContextCallback *error_context_stack;
  * of levels this will work for.  It's best to keep the error recovery
  * section simple enough that it can't generate any new errors, at least
  * not before popping the error stack.
+ *
+ * Note: an ereport(FATAL) will not be caught by this construct; control will
+ * exit straight through proc_exit().  Therefore, do NOT put any cleanup
+ * of non-process-local resources into the error recovery section, at least
+ * not without taking thought for what will happen during ereport(FATAL).
+ * The PG_ENSURE_ERROR_CLEANUP macros provided by storage/ipc.h may be
+ * helpful in such cases.
  *----------
  */
 #define PG_TRY()  \