Re: PATCH: logical_work_mem and logical streaming of large in-progress transactions

On Tue, Jun 23, 2020 at 8:18 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Mon, Jun 22, 2020 at 6:38 PM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
> >
> > On Mon, Jun 22, 2020 at 5:26 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> > >
> > > > > @@ -2012,8 +2014,6 @@ ReorderBufferForget(ReorderBuffer *rb,
> > > > > TransactionId xid, XLogRecPtr lsn)
> > > > > if (txn->base_snapshot != NULL && txn->ninvalidations > 0)
> > > > > ReorderBufferImmediateInvalidation(rb, txn->ninvalidations,
> > > > > txn->invalidations);
> > > > > - else
> > > > > - Assert(txn->ninvalidations == 0);
> > > > >
> > > > > Why this Assert is removed?
> > > >
> > > > Even if the base_snapshot is NULL, now we are collecting the
> > > > txn->invalidation.
> > > >
> > >
> > > But there doesn't seem to be any check even before this patch which
> > > directly prohibits accumulating invalidations in DecodeCommit. We
> > > have check for base_snapshot in ReorderBufferCommit. Did you get any
> > > failure with that check?
> >
> > Because earlier ReorderBufferForget for toptxn will be called if the
> > top transaction is aborted and in abort case, we are not logging any
> > invalidation so that will be 0. However same is not true now.
> >
>
> AFAICS, ReorderBufferForget() is called (via DecodeCommit) only when
> we need to skip the transaction. It doesn't seem to be called from
> Abort path (DecodeAbort/ReorderBufferAbort doesn't use
> ReorderBufferForget). I am not sure which code path are you referring
> here, can you please share the code flow which you are referring to
> here.

I think you are right, during some intermediate code change, it
crashed on that assert (I guess I might be adding invalidation to the
sub-transaction but not sure what was that state) and I assumed that
is the reason that I explained above but, now I see my assumption was
wrong. I will put back that assert. By testing, I could not hit any
case where we hit that assert even after my changes, still I will put
more thought if by any chance our case is different then the base
code.

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