Re: Replication slot stats misgivings

On Fri, Apr 16, 2021 at 2:58 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Thu, Apr 15, 2021 at 4:35 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
> >
> > Thank you for the update! The patch looks good to me.
> >

BTW regarding the commit f5fc2f5b23 that added total_txns and
total_bytes, we add the reorder buffer size (i.g., rb->size) to
rb->totalBytes but I think we should use the transaction size (i.g.,
txn->size) instead:

@@ -1363,6 +1365,11 @@ ReorderBufferIterTXNNext(ReorderBuffer *rb,
ReorderBufferIterTXNState *state)
dlist_delete(&change->node);
dlist_push_tail(&state->old_change, &change->node);

+ /*
+ * Update the total bytes processed before releasing the current set
+ * of changes and restoring the new set of changes.
+ */
+ rb->totalBytes += rb->size;
if (ReorderBufferRestoreChanges(rb, entry->txn, &entry->file,
&state->entries[off].segno))
{
@@ -2363,6 +2370,20 @@ ReorderBufferProcessTXN(ReorderBuffer *rb,
ReorderBufferTXN *txn,
ReorderBufferIterTXNFinish(rb, iterstate);
iterstate = NULL;

+ /*
+ * Update total transaction count and total transaction bytes
+ * processed. Ensure to not count the streamed transaction multiple
+ * times.
+ *
+ * Note that the statistics computation has to be done after
+ * ReorderBufferIterTXNFinish as it releases the serialized change
+ * which we have already accounted in ReorderBufferIterTXNNext.
+ */
+ if (!rbtxn_is_streamed(txn))
+ rb->totalTxns++;
+
+ rb->totalBytes += rb->size;
+

IIUC rb->size could include multiple decoded transactions. So it's not
appropriate to add that value to the counter as the transaction size
passed to the logical decoding plugin. If the reorder buffer process a
transaction while having a large transaction that is being decoded, we
could end up more increasing txn_bytes than necessary.

Please review the attached patch.

Regards,

--
Masahiko Sawada
EDB: https://www.enterprisedb.com/