Re: PATCH: logical_work_mem and logical streaming of largein-progress transactions - Mailing list pgsql-hackers

On Mon, Jun 8, 2020 at 11:53 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Sun, Jun 7, 2020 at 5:08 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> >
> > On Fri, Jun 5, 2020 at 11:37 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> > >
> > > Let me know what you think of the changes?  If you find them okay,
> > > then feel to include them in the next patch-set.
> > >
> > > [1] -
https://www.postgresql.org/message-id/CAONYFtOv%2BEr1p3WAuwUsy1zsCFrSYvpHLhapC_fMD-zNaRWxYg%40mail.gmail.com
> >
> > Thanks for the patch, I will review it and include it in my next version.

I have merged your changes 0002 in this version.

> Okay, I have done review of
> 0002-Issue-individual-invalidations-with-wal_level-lo.patch and below
> are my comments:
>
> 1. I don't think it is a good idea that logical decoding process the
> new XLOG_XACT_INVALIDATIONS and existing WAL records for invalidations
> like XLOG_INVALIDATIONS and what we do in DecodeCommit (see code in
> the check "if (parsed->nmsgs > 0)").  I think if that is required for
> some particular reason then we should write detailed comments about
> the same.  I have tried some experiments to see if those are really
> required:
> a. After applying patch 0002, I have tried by commenting out the
> processing of invalidations via DecodeCommit and found some regression
> tests were failing but the reason for failure was that we are not
> setting RBTXN_HAS_CATALOG_CHANGES for the toptxn when subtxn has
> catalog changes and when I did that all regression tests started
> passing.  See the attached diff patch
> (v27-0003-Incremental-patch-for-0002-to-test-removal-of-du) atop 0002
> patch.
> b. The processing of invalidations for XLOG_INVALIDATIONS is added by
> commit c6ff84b06a for xid-less transactions.  See
> https://postgr.es/m/CAB-SwXY6oH=9twBkXJtgR4UC1NqT-vpYAtxCseME62ADwyK5OA@mail.gmail.com
> to know why that has been added.  Now, after this patch we will
> process the same invalidations via XLOG_XACT_INVALIDATIONS and
> XLOG_INVALIDATIONS which doesn't seem warranted.  Also, the below
> assertion will fail for xid-less transactions (try create index
> concurrently statement):
> + case XLOG_XACT_INVALIDATIONS:
> + {
> + TransactionId xid;
> + xl_xact_invalidations *invals;
> +
> + xid = XLogRecGetXid(r);
> + invals = (xl_xact_invalidations *) XLogRecGetData(r);
> +
> + Assert(TransactionIdIsValid(xid));
>
> I feel we don't need the processing of XLOG_INVALIDATIONS in logical
> decoding after this patch but to prove that first we need to write a
> test case which need XLOG_INVALIDATIONS in the HEAD as commit
> c6ff84b06a doesn't add one.  I think we need two code paths in
> XLOG_XACT_INVALIDATIONS where if it is for xid-less transactions, then
> execute actions immediately as we are doing in processing of
> XLOG_INVALIDATIONS, otherwise, do what we are doing currently in the
> patch.  If the above point (b) is correct, I am not sure if it is a
> good idea to use RM_XACT_ID as resource manager if for this WAL in
> LogLogicalInvalidations, what do you think?
>
> I think one of the usages we still need is in ReorderBufferForget
> because it can be called when we skip processing the txn.  See the
> comments in DecodeCommit where we call this function.  If I am
> correct, we need to probably collect all invalidations in
> ReorderBufferTxn as we are collecting tuplecids and use them here.  We
> can do the same during processing of XLOG_XACT_INVALIDATIONS.
>
> I had also thought a bit about removing logging of invalidations at
> commit time altogether but it seems processing hot-standby is somewhat
> tightly coupled with existing WAL logging.  See xact_redo_commit (a
> comment atop call to ProcessCommittedInvalidationMessages).  It says
> we need to maintain the order when we process invalidations.  If we
> can later find a way to avoid that we can probably remove it but for
> now maybe we can live with it.

Yes, I have made the changes.  Basically, now I am only using the
XLOG_XACT_INVALIDATIONS for generating all the invalidation messages.
So whenever we are getting the new set of XLOG_XACT_INVALIDATIONS, we
are directly appending it to the txn->invalidations.  I have tested
the XLOG_INVALIDATIONS part but while sending this mail I realized
that we could write some automated test for the same.  I will work on
that soon.

> 2.
> + /* not expected, but print something anyway */
> + else if (msg->id == SHAREDINVALSMGR_ID)
> + appendStringInfoString(buf, " smgr");
> + /* not expected, but print something anyway */
> + else if (msg->id == SHAREDINVALRELMAP_ID)
>
> I think the above comment is not valid after we started logging at CCI.

Yup, fixed.

> 3.
> +
> + xid = XLogRecGetXid(r);
> + invals = (xl_xact_invalidations *) XLogRecGetData(r);
> +
> + Assert(TransactionIdIsValid(xid));
> + ReorderBufferAddInvalidation(reorder, xid, buf->origptr,
> + invals->nmsgs, invals->msgs);
>
> Here, it should check !ctx->forward as we do in DecodeCommit, do we
> have any reason for not doing so.  We can test once by changing this.

Yeah, it should have this check.

Mostly it contains changes in 0002,  apart from that we needed some
changes in 0005,0006 to rebase on 0002 and also there is one bug fix
in 0005, basically the txn->snapshot_now was not getting set to NULL
after freeing so it was getting double free.  I have also removed the
extra wait even from the 0014 as BufFile is already logging the wait
event internally and also some changes because BufFileWrite interface
is changed in recent commits.

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