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:
> >
> > On Wed, 7 Oct 2020 at 17:52, Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> >
> > > I think after we are done with this the next
> > > step would be to finish the streaming stats work [1]. We probably need
> > > to review and add the test case in that patch. If nobody else shows up
> > > I will pick it up and complete it.
> >
> > +1
> > I can review that patch.
> >
>
> 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()?
BTW maybe it's better to start a new thread for this patch as the
title is no longer relevant.
Regards,
--
Masahiko Sawada http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
