Re: Resetting spilled txn statistics in pg_stat_replication

On Wed, Sep 30, 2020 at 2:40 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Wed, Sep 30, 2020 at 1:12 PM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
> >
> > On Fri, Sep 25, 2020 at 4:33 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> > >
> > > On Thu, Sep 24, 2020 at 5:44 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)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 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.

+ /* 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.

+
+ /*
+ * 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.

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.

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

Other than that I did not see any problem.

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com