Re: Replication slot stats misgivings

Thanks for the comments.

On Fri, Apr 9, 2021 at 4:13 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Wed, Apr 7, 2021 at 2:51 PM vignesh C <vignesh21(at)gmail(dot)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.

I have split the patch as suggested.

> 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.
>

Modified it.

> 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.
>

Modified it.

> 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.

Modified it.
These comments are fixed in the patch available at [1].

[1] -
https://www.postgresql.org/message-id/CALDaNm1A%3DbjSrQjBNwNsOtTig%2B6pZpunmAj_P7Au0H0XjtvCyA%40mail.gmail.com

Regards,
Vignesh