v5-0005-review.patch

From 6dfeb61ffddeedc8e00f8de5eb6b644b28ae1f62 Mon Sep 17 00:00:00 2001
From: Tomas Vondra <tomas@2ndquadrant.com>
Date: Fri, 23 Feb 2024 13:15:44 +0100
Subject: [PATCH v5 5/5] review

---
 .../replication/logical/reorderbuffer.c       | 32 ++++++++++++-------
 1 file changed, 21 insertions(+), 11 deletions(-)

diff --git a/src/backend/replication/logical/reorderbuffer.c b/src/backend/replication/logical/reorderbuffer.c
index f22cf2fb9b8..40fa2ba9843 100644
--- a/src/backend/replication/logical/reorderbuffer.c
+++ b/src/backend/replication/logical/reorderbuffer.c
@@ -1537,7 +1537,7 @@ ReorderBufferCleanupTXN(ReorderBuffer *rb, ReorderBufferTXN *txn)
 {
 	bool		found;
 	dlist_mutable_iter iter;
-	Size		mem_freed = 0;
+	Size		mem_freed = 0;	/* XXX why don't we use txn->size directly? */
 
 	/* cleanup subtransactions & their changes */
 	dlist_foreach_modify(iter, &txn->subtxns)
@@ -1571,11 +1571,6 @@ ReorderBufferCleanupTXN(ReorderBuffer *rb, ReorderBufferTXN *txn)
 		ReorderBufferReturnChange(rb, change, false);
 	}
 
-	/* Update the memory counter */
-	Assert(mem_freed == txn->size);
-	if (mem_freed > 0)
-		ReorderBufferTXNMemoryUpdate(rb, txn, false, mem_freed);
-
 	/*
 	 * Cleanup the tuplecids we stored for decoding catalog snapshot access.
 	 * They are always stored in the toplevel transaction.
@@ -1635,14 +1630,21 @@ ReorderBufferCleanupTXN(ReorderBuffer *rb, ReorderBufferTXN *txn)
 	/* deallocate */
 	ReorderBufferReturnTXN(rb, txn);
 
+	/* Update the memory counter */
+	Assert(mem_freed == txn->size);
+	ReorderBufferTXNMemoryUpdate(rb, txn, false, mem_freed);
+
 	/*
-	 * Check if the number of transactions get lower than the threshold. If
+	 * Check if the number of transactions got lower than the threshold. If
 	 * so, switch to NO_MAXHEAP state and reset the max-heap.
 	 *
-	 * XXX: If a new transaction is added and the memory usage reached the
+	 * XXX: If a new transaction is added and the memory usage reaches the
 	 * limit soon, we will end up building the max-heap again. It might be
 	 * more efficient if we accept a certain amount of transactions to switch
 	 * back to the NO_MAXHEAP state, say 95% of the threshold.
+	 *
+	 * XXX Yes, having the enable/disable threshold exactly the same can lead
+	 * to trashing. Something like 90% would work, I think.
 	 */
 	if (rb->memtrack_state == REORDER_BUFFER_MEM_TRACK_MAINTAIN_MAXHEAP &&
 		(binaryheap_size(rb->txn_heap) < REORDER_BUFFER_MEM_TRACK_THRESHOLD))
@@ -3257,6 +3259,10 @@ ReorderBufferChangeMemoryUpdate(ReorderBuffer *rb,
  * counters instead - we don't really care about subtransactions as we
  * can't stream them individually anyway, and we only pick toplevel
  * transactions for eviction. So only toplevel transactions matter.
+ *
+ * XXX Not sure the naming is great, it seems pretty similar to the earlier
+ * function, can be quite confusing. Why do we even need the separate function
+ * and can't simply call ReorderBufferChangeMemoryUpdate from everywhere?
  */
 static void
 ReorderBufferTXNMemoryUpdate(ReorderBuffer *rb, ReorderBufferTXN *txn,
@@ -3264,6 +3270,9 @@ ReorderBufferTXNMemoryUpdate(ReorderBuffer *rb, ReorderBufferTXN *txn,
 {
 	ReorderBufferTXN *toptxn;
 
+	if (sz == 0)
+		return;
+
 	/*
 	 * Update the total size in top level as well. This is later used to
 	 * compute the decoding stats.
@@ -3745,6 +3754,8 @@ ReorderBufferCheckMemoryLimit(ReorderBuffer *rb)
 	 * Check the number of transactions in max-heap after evicting large
 	 * transactions. If the number of transactions is small, we switch back
 	 * to the NO_MAXHEAP state, and reset the current the max-heap.
+	 *
+	 * XXX We already have this block elsewhere, maybe have a function?
 	 */
 	if (rb->memtrack_state == REORDER_BUFFER_MEM_TRACK_MAINTAIN_MAXHEAP &&
 		(binaryheap_size(rb->txn_heap) < REORDER_BUFFER_MEM_TRACK_THRESHOLD))
@@ -3769,7 +3780,7 @@ ReorderBufferSerializeTXN(ReorderBuffer *rb, ReorderBufferTXN *txn)
 	XLogSegNo	curOpenSegNo = 0;
 	Size		spilled = 0;
 	Size		size = txn->size;
-	Size		mem_freed = 0;
+	Size		mem_freed = 0;	/* XXX why needed? can't we just use txn->size? */
 
 	elog(DEBUG2, "spill %u changes in XID %u to disk",
 		 (uint32) txn->nentries_mem, txn->xid);
@@ -3831,8 +3842,7 @@ ReorderBufferSerializeTXN(ReorderBuffer *rb, ReorderBufferTXN *txn)
 
 	/* Update the memory counter */
 	Assert(mem_freed == txn->size);
-	if (mem_freed > 0)
-		ReorderBufferTXNMemoryUpdate(rb, txn, false, mem_freed);
+	ReorderBufferTXNMemoryUpdate(rb, txn, false, mem_freed);
 
 	/* update the statistics iff we have spilled anything */
 	if (spilled)
-- 
2.43.0