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

On Tue, May 19, 2020 at 3:31 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
>
> On Tue, May 19, 2020 at 2:34 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Mon, May 18, 2020 at 5:57 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> > >
> > > 3.
> > > + /*
> > > + * If streaming is enable and we have serialized this transaction because
> > > + * it had incomplete tuple.  So if now we have got the complete tuple we
> > > + * can stream it.
> > > + */
> > > + if (ReorderBufferCanStream(rb) && can_stream && rbtxn_is_serialized(toptxn)
> > > + && !(rbtxn_has_toast_insert(txn)) && !(rbtxn_has_spec_insert(txn)))
> > > + {
> > >
> > > This comment is just saying what you are doing in the if-check.  I
> > > think you need to explain the rationale behind it. I don't like the
> > > variable name 'can_stream' because it matches ReorderBufferCanStream
> > > whereas it is for a different purpose, how about naming it as
> > > 'change_complete' or something like that.  The check has many
> > > conditions, can we move it to a separate function to make the code
> > > here look clean?
> > >
> >
> > Do we really need this?  Immediately after this check, we are calling
> > ReorderBufferCheckMemoryLimit which will anyway stream the changes if
> > required.
>
> Actually, ReorderBufferCheckMemoryLimit is only meant for checking
> whether we need to stream the changes due to the memory limit.  But
> suppose when memory limit exceeds that time we could not stream the
> transaction because there was only incomplete toast insert so we
> serialized.  Now,  when we get the tuple which makes the changes
> complete but now it is not crossing the memory limit as changes were
> already serialized.  So I am not sure whether it is a good idea to
> stream the transaction as soon as we get the complete changes or we
> shall wait till next time memory limit exceed and that time we select
> the suitable candidate.
>

I think it is better to wait till next time we exceed the memory threshold.

>  Ideally, we were are in streaming more and
> the transaction is serialized means it was already a candidate for
> streaming but could not stream due to the incomplete changes so
> shouldn't we stream it immediately as soon as its changes are complete
> even though now we are in memory limit.
>

The only time we need to stream or spill is when we exceed memory
threshold.  In the above case, it is possible that next time there is
some other candidate transaction that we can stream.

> >
> > Another comments on v20-0010-Bugfix-handling-of-incomplete-toast-tuple:
> >
> > + else if (rbtxn_has_toast_insert(txn) &&
> > + ChangeIsInsertOrUpdate(change->action))
> > + {
> > + toptxn->txn_flags &= ~RBTXN_HAS_TOAST_INSERT;
> > + can_stream = true;
> > + }
> > ..
> > +#define ChangeIsInsertOrUpdate(action) \
> > + (((action) == REORDER_BUFFER_CHANGE_INSERT) || \
> > + ((action) == REORDER_BUFFER_CHANGE_UPDATE) || \
> > + ((action) == REORDER_BUFFER_CHANGE_INTERNAL_SPEC_INSERT))
> >
> > How can we clear the RBTXN_HAS_TOAST_INSERT flag on
> > REORDER_BUFFER_CHANGE_INTERNAL_SPEC_INSERT action?
>
> Partial toast insert means we have inserted in the toast but not in
> the main table.  So even if it is spec insert we can form the complete
> tuple, however, we can still not stream it because we haven't got
> spec_confirm but for that, we are marking another flag.  So if the
> insert is aspect insert the toast insert will also be spec insert and
> as part of that toast, spec inserts we are marking partial tuple so
> cleaning that flag should happen when the spec insert is done for the
> main table right?
>

Sounds reasonable.


-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com