On Mon, Oct 12, 2020 at 11:52 AM Masahiko Sawada
<masahiko.sawada@2ndquadrant.com> wrote:
>
> On Thu, 8 Oct 2020 at 22:57, Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Thu, Oct 8, 2020 at 7:46 AM Masahiko Sawada
> > <masahiko.sawada@2ndquadrant.com> wrote:
> >
> > I have rebased the stream stats patch and made minor modifications. I
> > haven't done a detailed review but one thing that I think is not
> > correct is:
> > @@ -3496,10 +3499,18 @@ ReorderBufferStreamTXN(ReorderBuffer *rb,
> > ReorderBufferTXN *txn)
> > txn->snapshot_now = NULL;
> > }
> >
> > +
> > + rb->streamCount += 1;
> > + rb->streamBytes += txn->total_size;
> > +
> > + /* Don't consider already streamed transaction. */
> > + rb->streamTxns += (rbtxn_is_streamed(txn)) ? 0 : 1;
> > +
> > /* Process and send the changes to output plugin. */
> > ReorderBufferProcessTXN(rb, txn, InvalidXLogRecPtr, snapshot_now,
> > command_id, true);
> >
> > I think we should update the stream stats after
> > ReorderBufferProcessTXN rather than before because any error in
> > ReorderBufferProcessTXN can lead to an unnecessary update of stats.
> > But OTOH, the txn flags, and other data can be changed after
> > ReorderBufferProcessTXN so we need to save them in a temporary
> > variable before calling the function.
>
> Thank you for updating the patch!
>
> I've not looked at the patch in-depth yet but RBTXN_IS_STREAMED could
> be cleared after ReorderBUfferProcessTXN()?
>
I think you mean to say RBTXN_IS_STREAMED could be *set* after
ReorderBUfferProcessTXN(). We need to set it for txn and subtxns and
currently, it is being done with other things in
ReorderBufferTruncateTXN so not sure if it is a good idea to do this
separately.
> BTW maybe it's better to start a new thread for this patch as the
> title is no longer relevant.
>
Yeah, that makes sense. I'll do that while posting a new version of the patch.
--
With Regards,
Amit Kapila.