On Wed, Oct 21, 2020 at 8:15 AM Masahiko Sawada
<masahiko.sawada@2ndquadrant.com> wrote:
>
> On Tue, 20 Oct 2020 at 14:29, Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> >
> > 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.
>
It would only add for the current stream and I don't think that is
wrong because we would have sent some data (at least the start
message) for which we send the stream_stop message later while
decoding Abort message. I had thought to avoid this we can update the
stats in ReorderBufferProcessTXN at the end when we know streaming is
complete but again that would miss the counter update for the data we
have sent before an error has occurred and also updating the streaming
counters in ReorderBufferStreamTXN seems more logical to me.
> 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.
>
But as explained above, I think we count it as we would have sent at
least one message (could be more) before we encounter this error.
--
With Regards,
Amit Kapila.
