clear the reference to a tx from the freelistCursor

We've been seeing weird retention of Tx that shouldn't still be open, and
one possible explanation is that, until a Tx actually uses the freelist
cursor (either to allocate a page or to release it back to the freelist),
the freelistCursor statically stored in the Db object continues to have a
pointer to the previous Tx which used it, which allows a Tx, and thus its
dirty page map, to be retained forever.

I previously thought this should also nil out the page maps in the Tx, but
the more I think about it, the less I think that's a good idea. The actual
lifespan of a committed Tx should be quite short. If it *does* stick around,
it's beneficial to us as debuggers to see those large maps of dirty pages
sticking around. So after thinking about it a lot I decided not to do
that.

Similarly, when closing out a container filter (whether a filter or
a rewriter), zero out the Cursor, Tx, and filter and rewriter functions.
(We don't have to worry about the cursor's Tx, because the cursor gets
closed, which zeros its Tx and returns the cursor to the cursor pool,
too.) This likely matters a lot less, as the filters in the pool
get garbage collected "soon", but it still reduces the amount of
stuff being retained.

rbf/cursor.go

@@ -22,6 +22,9 @@ const (
 //
 // If you mutate the b-tree that a cursor is attached to then you'll need to
 // re-initialize the cursor. This may be changed in the future though.
+//
+// Cursors can be reused in some cases; if you're keeping a cursor around,
+// be sure to nil out the tx, or it will hold a reference to it.
 type Cursor struct {
 	tx       *Tx
 	buffered bool // if true, Next() and Prev() do not move the cursor position
@@ -1464,11 +1467,22 @@ func (c *Cursor) difference(key uint64, data *roaring.Container) (bool, error) {
 	return false, nil
 }
 
+// Close closes a cursor out, invalidating it for future use, and puts it
+// back in a pool to reduce allocations. Contrast with unpooledClose, which
+// you probably shouldn't use.
 func (c *Cursor) Close() {
 	assert(c != nil)
+	c.unpooledClose()
 	cursorSyncPool.Put(c)
 }
 
+// unpooledClose is the part of a close operation which does not put the
+// cursor back in the pool. It is useful only for cases where we're reusing
+// a cursor, such as a Db's freelistCursor.
+func (c *Cursor) unpooledClose() {
+	c.tx = nil
+}
+
 func keysFromParents(parents []branchCell) (ckeys []int) {
 	for _, par := range parents {
 		ckeys = append(ckeys, int(par.LeftKey))

rbf/tx.go

@@ -170,6 +170,7 @@ func (tx *Tx) truncateLastFreePage() (truncated bool, outErr error) {
 	defer tx.freelistCleanup(&outErr)
 
 	c := tx.db.getFreelistCursor(tx)
+	defer c.unpooledClose()
 	if err := c.Last(); err == io.EOF {
 		return false, nil
 	} else if err != nil {
@@ -1121,6 +1122,7 @@ func (tx *Tx) freelistCleanup(outErr *error) {
 		return
 	}
 	c := tx.db.getFreelistCursor(tx)
+	defer c.unpooledClose()
 	for len(tx.pendingFreelistAdds) > 0 {
 		var pass []uint32
 		pass, tx.pendingFreelistAdds = tx.pendingFreelistAdds, nil
@@ -1157,6 +1159,7 @@ func (tx *Tx) allocatePgno() (_ uint32, outErr error) {
 	tx.modifyingFreelist = true
 	defer tx.freelistCleanup(&outErr)
 	c := tx.db.getFreelistCursor(tx)
+	defer c.unpooledClose()
 	if err := c.First(); err == io.EOF {
 		return tx.allocateNewPgno(), nil
 	} else if err != nil {
@@ -1200,6 +1203,7 @@ func (tx *Tx) freePgno(pgno uint32) (outErr error) {
 		return nil
 	}
 	c := tx.db.getFreelistCursor(tx)
+	defer c.unpooledClose()
 
 	tx.modifyingFreelist = true
 	defer tx.freelistCleanup(&outErr)
@@ -1750,8 +1754,13 @@ type containerFilter struct {
 }
 
 func (s *containerFilter) Close() {
+	// note that the cursor gets put back in the pool, but that cursor.Close
+	// zeroes out the cursor's tx for us.
 	s.cursor.Close()
 	s.cursor = nil
+	s.tx = nil
+	s.filter = nil
+	s.rewriter = nil
 	containerFilterPool.Put(s)
 }
 
@@ -2077,6 +2086,15 @@ func (tx *Tx) flush() error {
 		tx.pageMap = tx.pageMap.Set(pgno, walID)
 	}
 
+	// At this point, it is safe to nil out the dirtyPages and
+	// dirtyBitmapPages objects. We don't. The reason we don't is that
+	// we should never have a Tx lasting for long anyway -- even if we
+	// end up holding the write lock for a checkpoint, we don't keep the
+	// associated Tx around. If we nil those out, then a few stray Tx
+	// objects sticking around won't stick out in a heap profile. If we
+	// leave them alone, they'll stick out in a heap profile. I think on
+	// the whole that's better for further observability and debugging.
+
 	// Write meta page to WAL.
 	walID, err := tx.writeToWAL(w, tx.meta[:])
 	if err != nil {