Re: PATCH: logical_work_mem and logical streaming of largein-progress transactions

On Mon, Oct 21, 2019 at 2:50 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Mon, Oct 21, 2019 at 10:48 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> >
> > On Fri, Oct 18, 2019 at 5:32 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > > 3.
> > > @@ -2479,7 +2480,7 @@ ReorderBufferSerializeTXN(ReorderBuffer *rb,
> > > ReorderBufferTXN *txn)
> > >
> > >   /* update the statistics */
> > >   rb->spillCount += 1;
> > > - rb->spillTxns += txn->serialized ? 1 : 0;
> > > + rb->spillTxns += txn->serialized ? 0 : 1;
> > >   rb->spillBytes += size;
> > >
> > > Why is this change required?  Shouldn't we increase the spillTxns
> > > count only when the txn is serialized?
> >
> > Prior to this change it was increasing the rb->spillTxns, every time
> > we try to serialize the changes of the transaction.  Now, only we
> > increase first time when it is not yet serialized.
> >
> > >
> > > 3.
> > > ReorderBufferSerializeTXN()
> > > {
> > > ..
> > > /* update the statistics */
> > > rb->spillCount += 1;
> > > rb->spillTxns += txn->serialized ? 0 : 1;
> > > rb->spillBytes += size;
> > >
> > > Assert(spilled == txn->nentries_mem);
> > > Assert(dlist_is_empty(&txn->changes));
> > > txn->nentries_mem = 0;
> > > txn->serialized = true;
> > > ..
> > > }
> > >
> > > I am not able to understand the above code.  We are setting the
> > > serialized parameter a few lines after we check it and increment the
> > > spillTxns count. Can you please explain it?
> >
> > Basically, when the first time we attempt to serialize a transaction,
> > txn->serialized will be false, that time we will increment the
> > rb->spillTxns and after that set txn->serialized to true.  From next
> > time onwards if we try to serialize the same transaction we will not
> > increment the rb->spillTxns so that we count each transaction only
> > once.
> >
>
> Your explanation for both the above comments makes sense to me.  Can
> you please add some comments along these lines because it is not
> apparent why one wants to increase the spillTxns counter when
> txn->serialized is false?
Ok, I will add comments in the next patch.
>
> > >
> > > Also, isn't spillTxns count bit confusing, because in some cases it
> > > will include subtransactions and other cases (where the largest picked
> > > transaction is a subtransaction) it won't include it?
> >
> > I did not understand your comment completely.  Basically,  every
> > transaction which we are serializing we will increase the count first
> > time right? whether it is the main transaction or the sub-transaction.
> >
>
> It was not clear to me earlier whether we always increase the
> spillTxns counter for subtransactions or not.  But now, looking at
> code carefully, it is clear that is it is getting increased in every
> case.  In short, you don't need to do anything for this comment.
ok

-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com