On Mon, Oct 12, 2020 at 11:52 AM Masahiko Sawada <masahiko.saw...@2ndquadrant.com> wrote: > > 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: > > > > 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.
