postgres.git / summary / log / commit / refs

commit    c28bfb35ef6ce9e9d82da00008554d0fc9c4a7fc
Author:   Tom Lane <tgl@sss.pgh.pa.us>
Date:     Tue Jun 04 18:59:02 2013 +0000
First released in: 9.3.0, 9.0.14, 9.1.10, 9.2.5

    Fix memory leak in LogStandbySnapshot().
    
    The array allocated by GetRunningTransactionLocks() needs to be pfree'd
    when we're done with it.  Otherwise we leak some memory during each
    checkpoint, if wal_level = hot_standby.  This manifests as memory bloat
    in the checkpointer process, or in bgwriter in versions before we made
    the checkpointer separate.
    
    Reported and fixed by Naoya Anzai.  Back-patch to 9.0 where the issue
    was introduced.
    
    In passing, improve comments for GetRunningTransactionLocks(), and add
    an Assert that we didn't overrun the palloc'd array.


 src/backend/storage/ipc/standby.c |  7 +------
 src/backend/storage/lmgr/lock.c   | 16 +++++++++++++---
 2 files changed, 14 insertions(+), 9 deletions(-)

diff --git a/src/backend/storage/ipc/standby.c b/src/backend/storage/ipc/standby.c
index 1a5481177ad..1db951d1fbc 100644
--- a/src/backend/storage/ipc/standby.c
+++ b/src/backend/storage/ipc/standby.c
@@ -879,16 +879,11 @@ LogStandbySnapshot(void)
 
 	/*
 	 * Get details of any AccessExclusiveLocks being held at the moment.
-	 *
-	 * XXX GetRunningTransactionLocks() currently holds a lock on all
-	 * partitions though it is possible to further optimise the locking. By
-	 * reference counting locks and storing the value on the ProcArray entry
-	 * for each backend we can easily tell if any locks need recording without
-	 * trying to acquire the partition locks and scanning the lock table.
 	 */
 	locks = GetRunningTransactionLocks(&nlocks);
 	if (nlocks > 0)
 		LogAccessExclusiveLocks(nlocks, locks);
+	pfree(locks);
 
 	/*
 	 * Log details of all in-progress transactions. This should be the last
diff --git a/src/backend/storage/lmgr/lock.c b/src/backend/storage/lmgr/lock.c
index e9afc7c0093..1020756ae34 100644
--- a/src/backend/storage/lmgr/lock.c
+++ b/src/backend/storage/lmgr/lock.c
@@ -2360,18 +2360,26 @@ GetLockStatusData(void)
 }
 
 /*
- * Returns a list of currently held AccessExclusiveLocks, for use
- * by GetRunningTransactionData().
+ * Returns a list of currently held AccessExclusiveLocks, for use by
+ * LogStandbySnapshot().  The result is a palloc'd array,
+ * with the number of elements returned into *nlocks.
+ *
+ * XXX This currently takes a lock on all partitions of the lock table,
+ * but it's possible to do better.  By reference counting locks and storing
+ * the value in the ProcArray entry for each backend we could tell if any
+ * locks need recording without having to acquire the partition locks and
+ * scan the lock table.  Whether that's worth the additional overhead
+ * is pretty dubious though.
  */
 xl_standby_lock *
 GetRunningTransactionLocks(int *nlocks)
 {
+	xl_standby_lock *accessExclusiveLocks;
 	PROCLOCK   *proclock;
 	HASH_SEQ_STATUS seqstat;
 	int			i;
 	int			index;
 	int			els;
-	xl_standby_lock *accessExclusiveLocks;
 
 	/*
 	 * Acquire lock on the entire shared lock data structure.
@@ -2428,6 +2436,8 @@ GetRunningTransactionLocks(int *nlocks)
 		}
 	}
 
+	Assert(index <= els);
+
 	/*
 	 * And release locks.  We do this in reverse order for two reasons: (1)
 	 * Anyone else who needs more than one of the locks will be trying to lock


[parent: bb5a21746bf0]