Re: Track statistics for streaming of in-progress transactions

On Tue, 20 Oct 2020 at 14:29, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Mon, Oct 19, 2020 at 1:52 PM Ajin Cherian <itsajin(at)gmail(dot)com> wrote:
> >
> > On Wed, Oct 14, 2020 at 2:39 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> > >
> > > Commit 464824323e has added the support of the streaming of
> > > in-progress transactions into the built-in logical replication. The
> > > attached patch adds the statistics about transactions streamed to the
> > > decoding output plugin from ReorderBuffer. Users can query the
> > > pg_stat_replication_slots view to check these stats and call
> > > pg_stat_reset_replication_slot to reset the stats of a particular
> > > slot. Users can pass NULL in pg_stat_reset_replication_slot to reset
> > > stats of all the slots.
> > >
> > > Commit 9868167500 has added the basic infrastructure to capture the
> > > stats of slot and this commit extends the statistics collector to
> > > track additional information about slots.
> > >
> > > This patch was originally written by Ajin Cherian [1]. I have fixed
> > > bugs and modified some comments in the code.
> > >
> > > Thoughts?
> > >
> > > [1] - https://www.postgresql.org/message-id/CAFPTHDZ8RnOovefzB%2BOMoRxLSD404WRLqWBUHe6bWqM5ew1bNA%40mail.gmail.com
> >
> > I've applied the patch. It applies cleanly. I've reviewed the patch
> > and have no comments to report.
> > I have also run some tests to get streaming stats as well as reset the
> > stats counter, everything seems to be working as expected.
> > I am fine with the changes.
> >
>
> Thanks. One thing I have considered while updating this patch was to
> write a test case similar to what we have for spilled stats in
> test_decoding/sql/stats.sql but I decided not to do it as that doesn't
> seem to add much value for the streaming case because we already have
> some tests in test_decoding/sql/stream.sql which indicates that the
> streaming is happening. If we could have a way to get the exact
> streaming stats then it would have been better but while writing tests
> for spilled stats we found that it is not possible because some
> background transactions (like autovacuum) might send the stats earlier
> making the actual number inconsistent. What do you think?
>
> Sawada-San, do you have any thoughts on this matter?

I basically agree with that. Reading the patch, I have a question that
might be relevant to this matter:

The patch has the following code:

+ /*
+ * Remember this information to be used later to update stats. We can't
+ * update the stats here as an error while processing the changes would
+ * lead to the accumulation of stats even though we haven't streamed all
+ * the changes.
+ */
+ txn_is_streamed = rbtxn_is_streamed(txn);
+ stream_bytes = txn->total_size;

The commend seems to mention only about when an error happened while
processing the changes but I wonder if the same is true for the
aborted transaction. That is, if we catch an error due to concurrent
transaction abort while processing the changes, we stop to stream the
changes. But the patch accumulates the stats even in this case. If we
don’t want to accumulate the stats of the abort transaction and it’s
easily reproducible, it might be better to add a test checking if we
don’t accumulate in that case.

Regards,

--
Masahiko Sawada http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services