Re: Replication slot stats misgivings

On Wed, Apr 7, 2021 at 2:51 PM vignesh C <vignesh21@gmail.com> wrote:
>
> That is not required, I have modified it.
> Attached v4 patch has the fixes for the same.
>

Few comments:

0001
------
1. The first patch includes changing char datatype to NameData
datatype for slotname. I feel this can be a separate patch from adding
new stats in the view. I think we can also move the change related to
moving stats to a structure rather than sending them individually in
the same patch.

2.
@@ -2051,6 +2054,17 @@ ReorderBufferProcessTXN(ReorderBuffer *rb,
ReorderBufferTXN *txn,
  rb->begin(rb, txn);
  }

+ /*
+ * Update total transaction count and total transaction bytes, if
+ * transaction is streamed or spilled it will be updated while the
+ * transaction gets spilled or streamed.
+ */
+ if (!rb->streamBytes && !rb->spillBytes)
+ {
+ rb->totalTxns++;
+ rb->totalBytes += rb->size;
+ }

I think this will skip a transaction if it is interleaved between a
streaming transaction. Assume, two transactions t1 and t2. t1 sends
changes in multiple streams and t2 sends all changes in one go at
commit time. So, now, if t2 is interleaved between multiple streams
then I think the above won't count t2.

3.
@@ -3524,9 +3538,11 @@ ReorderBufferSerializeTXN(ReorderBuffer *rb,
ReorderBufferTXN *txn)
  {
  rb->spillCount += 1;
  rb->spillBytes += size;
+ rb->totalBytes += size;

  /* don't consider already serialized transactions */
  rb->spillTxns += (rbtxn_is_serialized(txn) ||
rbtxn_is_serialized_clear(txn)) ? 0 : 1;
+ rb->totalTxns += (rbtxn_is_serialized(txn) ||
rbtxn_is_serialized_clear(txn)) ? 0 : 1;
  }

We do serialize each subtransaction separately. So totalTxns will
include subtransaction count as well when serialized, otherwise not.
The description of totalTxns also says that it doesn't include
subtransactions. So, I think updating rb->totalTxns here is wrong.

0002
-----
1.
+$node->safe_psql('postgres',
+ "SELECT data FROM pg_logical_slot_get_changes('regression_slot2',
NULL, NULL, 'include-xids', '0', 'skip-empty-xacts', '1')");
+$node->safe_psql('postgres',
+        "SELECT data FROM
pg_logical_slot_get_changes('regression_slot3', NULL, NULL,
'include-xids', '0', 'skip-empty-xacts', '1')");

The indentation of the second SELECT seems to bit off.


-- 
With Regards,
Amit Kapila.