Re: Memory-leak in BackgroundWriter(and Checkpointer)

Stephen Frost <sfrost@snowman.net> writes:
> * Tom Lane (tgl@sss.pgh.pa.us) wrote:
>> I think the proposed fix is fine code-wise; the real problem here is
>> crummy commenting.  GetRunningTransactionLocks isn't documented as
>> returning a palloc'd array, and why the heck do we have a long comment
>> about its implementation in LogStandbySnapshot?

> Certainly good questions and better comments would have helped here.  I
> can go back and rework the patch either way.

I'm already working on back-patching the attached.

            regards, tom lane

diff --git a/src/backend/storage/ipc/standby.c b/src/backend/storage/ipc/standby.c
index 615278b8ca2adf3057e5f21057c3cedfc66480aa..c704412366d5bc4b6512e9ab673855c46f7d993f 100644
*** a/src/backend/storage/ipc/standby.c
--- b/src/backend/storage/ipc/standby.c
*************** LogStandbySnapshot(void)
*** 865,880 ****

      /*
       * 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);

      /*
       * Log details of all in-progress transactions. This should be the last
--- 865,875 ----

      /*
       * Get details of any AccessExclusiveLocks being held at the moment.
       */
      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
*** a/src/backend/storage/lmgr/lock.c
--- b/src/backend/storage/lmgr/lock.c
*************** GetLockStatusData(void)
*** 3398,3415 ****
  }

  /*
!  * Returns a list of currently held AccessExclusiveLocks, for use
!  * by GetRunningTransactionData().
   */
  xl_standby_lock *
  GetRunningTransactionLocks(int *nlocks)
  {
      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.
--- 3398,3423 ----
  }

  /*
!  * 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;

      /*
       * Acquire lock on the entire shared lock data structure.
*************** GetRunningTransactionLocks(int *nlocks)
*** 3467,3472 ****
--- 3475,3482 ----
          }
      }

+     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