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

On Fri, Jan 31, 2020 at 8:08 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Thu, Jan 30, 2020 at 6:10 PM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
> >
> > On Thu, Jan 30, 2020 at 4:11 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> > >
> > > Also, if we need to copy the snapshot here, then do we need to again
> > > copy it in ReorderBufferProcessTXN(in below code and in catch block in
> > > the same function).
> > I think so because as part of the
> > "REORDER_BUFFER_CHANGE_INTERNAL_SNAPSHOT" change, we might directly
> > point to the snapshot and that will get truncated when we truncate all
> > the changes of the ReorderBufferTXN. So I think we can check if
> > snapshot_now->copied is true then we can avoid copying otherwise we
> > can copy?
> >
>
> Yeah, that makes sense, but I think then we also need to ensure that
> ReorderBufferStreamTXN frees the snapshot only when it is copied. It
> seems to me it should be always copied in the place where we are
> trying to free it, so probably we should have an Assert there.
>
> One more thing:
> ReorderBufferProcessTXN()
> {
> ..
> + if (streaming)
> + {
> + /*
> + * While streaming an in-progress transaction there is a
> + * possibility that the (sub)transaction might get aborted
> + * concurrently. In such case if the (sub)transaction has
> + * catalog update then we might decode the tuple using wrong
> + * catalog version. So for detecting the concurrent abort we
> + * set CheckXidAlive to the current (sub)transaction's xid for
> + * which this change belongs to. And, during catalog scan we
> + * can check the status of the xid and if it is aborted we will
> + * report an specific error which we can ignore. We might have
> + * already streamed some of the changes for the aborted
> + * (sub)transaction, but that is fine because when we decode the
> + * abort we will stream abort message to truncate the changes in
> + * the subscriber.
> + */
> + CheckXidAlive = change->txn->xid;
> + }
> ..
> }
>
> I think it is better to move the above code into an inline function
> (something like SetXidAlive). It will make the code in function
> ReorderBufferProcessTXN look cleaner and easier to understand.
>
Fixed in the latest version sent upthread.

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