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

On Fri, May 22, 2020 at 6:21 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
>
> On Mon, May 18, 2020 at 5:57 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> >
> > Few comments on v20-0010-Bugfix-handling-of-incomplete-toast-tuple
> > 1.
> > + /*
> > + * If this is a toast insert then set the corresponding bit.  Otherwise, if
> > + * we have toast insert bit set and this is insert/update then clear the
> > + * bit.
> > + */
> > + if (toast_insert)
> > + toptxn->txn_flags |= RBTXN_HAS_TOAST_INSERT;
> > + else if (rbtxn_has_toast_insert(txn) &&
> > + ChangeIsInsertOrUpdate(change->action))
> > + {
> >
> > Here, it might better to add a comment on why we expect only
> > Insert/Update?  Also, it might be better that we add an assert for
> > other operations.
>
> I have added comments that why on Insert/Update we clean the flag.
> But I don't think we only expect insert/update,  we might get the
> toast delete right? because in toast update we will do toast delete +
> toast insert.  So when we get toast delete we just don't want to do
> anything.
>

Okay, that makes sense.

> >
> > 2.
> > @@ -1865,8 +1920,8 @@ ReorderBufferProcessTXN(ReorderBuffer *rb,
> > ReorderBufferTXN *txn,
> >   * disk.
> >   */
> >   dlist_delete(&change->node);
> > - ReorderBufferToastAppendChunk(rb, txn, relation,
> > -   change);
> > + ReorderBufferToastAppendChunk(rb, txn, relation,
> > +   change);
> >   }
> >
> > This seems to be a spurious change.
>
> Done
>
> 2. There is a bug fix in handling the stream abort in 0008 (earlier it
> was 0006).
>

The code changes look fine but it is not clear what was the exact
issue.  Can you explain?

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