From 6dfeb61ffddeedc8e00f8de5eb6b644b28ae1f62 Mon Sep 17 00:00:00 2001 From: Tomas Vondra 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