Re: Replication slot stats misgivings

On Fri, Apr 9, 2021 at 4:13 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> 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.
>

The attached patch should fix the above two comments. I think it
should be sufficient if we just update the stats after processing the
TXN. We need to ensure that don't count streamed transactions multiple
times. I have not tested the attached, can you please review/test it
and include it in the next set of patches if you agree with this
change.

-- 
With Regards,
Amit Kapila.