Re: Improve eviction algorithm in ReorderBuffer

On Mon, Feb 26, 2024 at 7:54 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>

A few comments on 0003:
===================
1.
+/*
+ * Threshold of the total number of top-level and sub transactions
that controls
+ * whether we switch the memory track state. While the MAINTAIN_HEAP state is
+ * effective when there are many transactions being decoded, in many systems
+ * there is generally no need to use it as long as all transactions
being decoded
+ * are top-level transactions. Therefore, we use MaxConnections as
the threshold
+ * so we can prevent switch to the state unless we use subtransactions.
+ */
+#define REORDER_BUFFER_MEM_TRACK_THRESHOLD MaxConnections

The comment seems to imply that MAINTAIN_HEAP is useful for large
number of transactions but ReorderBufferLargestTXN() switches to this
state even when there is one transaction. So, basically we use the
binary_heap technique to get the largest even when we have one
transaction but we don't maintain that heap unless we have
REORDER_BUFFER_MEM_TRACK_THRESHOLD number of transactions are
in-progress. This means there is some additional work when (build and
reset heap each time when we pick largest xact) we have fewer
transactions in the system but that may not be impacting us because of
other costs involved like serializing all the changes. I think once we
can try to stress test this by setting
debug_logical_replication_streaming to 'immediate' to see if the new
mechanism has any overhead.

2. Can we somehow measure the additional memory that will be consumed
by each backend/walsender to maintain transactions? Because I think
this also won't be accounted for logical_decoding_work_mem, so if this
is large, there could be a chance of more complaints on us for not
honoring logical_decoding_work_mem.

3.
@@ -3707,11 +3817,14 @@ ReorderBufferSerializeTXN(ReorderBuffer *rb,
ReorderBufferTXN *txn)

ReorderBufferSerializeChange(rb, txn, fd, change);
dlist_delete(&change->node);
- ReorderBufferReturnChange(rb, change, true);
+ ReorderBufferReturnChange(rb, change, false);

spilled++;
}

+ /* Update the memory counter */
+ ReorderBufferChangeMemoryUpdate(rb, NULL, txn, false, txn->size);

In ReorderBufferSerializeTXN(), we already use a size variable for
txn->size, we can probably use that for the sake of consistency.

--
With Regards,
Amit Kapila.