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

On Tue, Jun 30, 2020 at 9:20 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Mon, Jun 29, 2020 at 4:24 PM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
> >
> > On Mon, Jun 22, 2020 at 4:30 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> > >
> > >
> > > Other than above tests, can we somehow verify that the invalidations
> > > generated at commit time are the same as what we do with this patch?
> > > We have verified with individual commands but it would be great if we
> > > can verify for the regression tests.
> >
> > I have verified this using a few random test cases. For verifying
> > this I have made some temporary code changes with an assert as shown
> > below. Basically, on DecodeCommit we call
> > ReorderBufferAddInvalidations function only for an assert checking.
> >
> > -void
> > ReorderBufferAddInvalidations(ReorderBuffer *rb, TransactionId xid,
> > XLogRecPtr
> > lsn, Size nmsgs,
> > -
> > SharedInvalidationMessage *msgs)
> > +
> > SharedInvalidationMessage *msgs, bool commit)
> > {
> > ReorderBufferTXN *txn;
> >
> > txn = ReorderBufferTXNByXid(rb, xid, true, NULL, lsn, true);
> > -
> > + if (commit)
> > + {
> > + Assert(txn->ninvalidations == nmsgs);
> > + return;
> > + }
> >
> > The result is that for a normal local test it works fine. But with
> > regression suit, it hit an assert at many places because if the
> > rollback of the subtransaction is involved then at commit time
> > invalidation messages those are not logged whereas with command time
> > invalidation those are logged.
> >
>
> Yeah, somehow, we need to ignore rollback to savepoint tests and
> verify for others.

Yeah, I have run the regression suite, I can see a lot of failure
maybe we can somehow see the diff and confirm that all the failures
are due to rollback to savepoint only. I will work on this.

>
> > As of now, I have only put assert on the count, if we need to verify
> > the exact messages then we might need to somehow categories the
> > invalidation messages because the ordering of the messages will not be
> > the same. For testing this we will have to arrange them by category
> > i.e relcahce, catcache and then we can compare them.
> >
>
> Can't we do this by verifying that each message at commit time exists
> in the list of invalidation messages we have collected via processing
> XLOG_XACT_INVALIDATIONS?

Let me try what is the easiest way to test this.

>
> One additional question on patch
> v30-0003-Extend-the-output-plugin-API-with-stream-methods:
> +static void
> +stream_commit_cb_wrapper(ReorderBuffer *cache, ReorderBufferTXN *txn,
> + XLogRecPtr apply_lsn)
> {
> ..
> ..
> + state.report_location = apply_lsn;
> ..
> ..
> + ctx->write_location = apply_lsn;
> ..
> }
>
> Can't we name the last parameter as 'commit_lsn' as that is how
> documentation in the patch spells it and it sounds more appropriate?

You are right commit_lsn seems more appropriate here.

> Also, is there a reason for assigning report_location and
> write_location differently than what we do in commit_cb_wrapper?
> Basically, assign those as txn->final_lsn and txn->end_lsn
> respectively.

Yes, I think it should be handled in same way as commit_cb_wrapper.
Because before calling ReorderBufferStreamCommit in
ReorderBufferCommit, we are properly updating the final_lsn as well as
the end_lsn.

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