Re: Resetting spilled txn statistics in pg_stat_replication

On Wed, Sep 30, 2020 at 4:34 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
>
> On Wed, Sep 30, 2020 at 2:40 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Wed, Sep 30, 2020 at 1:12 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> > >
> > > On Fri, Sep 25, 2020 at 4:33 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > > >
> > > > On Thu, Sep 24, 2020 at 5:44 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > > >
> > > > I have done some more testing of this patch especially for the case
> > > > where we spill before streaming the transaction and found everything
> > > > is working as expected. Additionally, I have changed a few more
> > > > comments and ran pgindent. I am still not very sure whether we want to
> > > > display physical slots in this view as all the stats are for logical
> > > > slots but anyway we can add stats w.r.t physical slots in the future.
> > > > I am fine either way (don't show physical slots in this view or show
> > > > them but keep stats as 0). Let me know if you have any thoughts on
> > > > these points, other than that I am happy with the current state of the
> > > > patch.
> > >
> > > IMHO, It will make more sense to only show the logical replication
> > > slots in this view.
> > >
> >
> > Okay, Sawada-San, others, do you have any opinion on this matter?
>

I have changed so that view will only show logical replication slots
and adjusted the docs accordingly. I think we can easily extend it to
show physical replication slots if required in the future.

> I have started looking into this patch, I have a few comments.
>
> +        Number of times transactions were spilled to disk. Transactions
> +        may get spilled repeatedly, and this counter gets incremented on every
> +        such invocation.
> +      </para></entry>
> +     </row>
> +
> +
> +  <para>
> +   Tracking of spilled transactions works only for logical replication.  In
>
> The number of spaces used after the full stop is not uniform.
>

I have removed this sentence as it is not required as we only want to
show logical slots in the view and for that I have updated the other
parts of the doc.

> + /* update the statistics iff we have spilled anything */
> + if (spilled)
> + {
> + rb->spillCount += 1;
> + rb->spillBytes += size;
> +
> + /* Don't consider already serialized transactions. */
>
> Single line comments are not uniform,  "update the statistics" is
> starting is small letter and not ending with the full stop
> whereas 'Don't consider' is starting with capital and ending with full stop.
>

Actually, it doesn't matter in this case but I have changed to keep it
consistent.

>
> +
> + /*
> + * We set this flag to indicate if the transaction is ever serialized.
> + * We need this to accurately update the stats.
> + */
> + txn->txn_flags |= RBTXN_IS_SERIALIZED_CLEAR;
>
> I feel we can explain the exact scenario in the comment, i.e. after
> spill if we stream then we still
> need to know that it spilled in past so that we don't count this again
> as a new spilled transaction.
>

Okay, I have expanded the comment a bit more to explain this.

> old slot.  We send the drop
> + * and create messages while holding ReplicationSlotAllocationLock to
> + * reduce that possibility. If the messages reached in reverse, we would
> + * lose one statistics update message.  But
>
> Spacing after the full stop is not uniform.
>

Changed.

>
> + * Statistics about transactions spilled to disk.
> + *
> + * A single transaction may be spilled repeatedly, which is why we keep
> + * two different counters. For spilling, the transaction counter includes
> + * both toplevel transactions and subtransactions.
> + */
> + int64 spillCount; /* spill-to-disk invocation counter */
> + int64 spillTxns; /* number of transactions spilled to disk  */
> + int64 spillBytes; /* amount of data spilled to disk */
>
> Can we keep the order as spillTxns, spillTxns, spillBytes because
> every other place we kept like that
> so that way it will look more uniform.
>

Changed here and at one more place as per this suggestion.

> Other than that I did not see any problem.
>

Thanks for the review.

-- 
With Regards,
Amit Kapila.