On Thu, 8 Oct 2020 at 22:57, Amit Kapila <amit.kapil...@gmail.com> wrote: > > On Thu, Oct 8, 2020 at 7:46 AM Masahiko Sawada > <masahiko.saw...@2ndquadrant.com> wrote: > > > > On Wed, 7 Oct 2020 at 17:52, Amit Kapila <amit.kapil...@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
