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.

diff --git a/src/backend/storage/ipc/standby.c b/src/backend/storage/ipc/standby.c
index 615278b8ca2adf3057e5f21057c3cedfc66480aa..c704412366d5bc4b6512e9ab673855c46f7d993f 100644 (file)
--- a/src/backend/storage/ipc/standby.c
+++ b/src/backend/storage/ipc/standby.c
@@ -865,16 +865,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 8cd871f4b40d7bfc8b0aaa7650241d5865c79ffe..273c72230274b46ba6a46bb8b295ef5375aaa36d 100644 (file)
--- a/src/backend/storage/lmgr/lock.c
+++ b/src/backend/storage/lmgr/lock.c
@@ -3398,18 +3398,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.
@@ -3467,6 +3475,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