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

On Fri, Jun 5, 2020 at 11:37 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Fri, May 29, 2020 at 8:31 PM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
> >
> > Apart from this one more fix in 0005, basically, CheckLiveXid was
> > never reset, so I have fixed that as well.
> >
>
> I have made a number of modifications in the 0001 patch and attached
> is the result. I have changed/added comments, done some cosmetic
> cleanup, and ran pgindent. The most notable change is to remove the
> below code change:
> DecodeXactOp()
> {
> ..
> - * However, it's critical to process XLOG_XACT_ASSIGNMENT records even
> + * However, it's critical to process records with subxid assignment even
> * when the snapshot is being built: it is possible to get later records
> * that require subxids to be properly assigned.
> */
> if (SnapBuildCurrentState(builder) < SNAPBUILD_FULL_SNAPSHOT &&
> - info != XLOG_XACT_ASSIGNMENT)
> + !TransactionIdIsValid(XLogRecGetTopXid(r)))
> ..
> }
>
> I have not only removed the change done by the patch but the check
> related to XLOG_XACT_ASSIGNMENT as well. That check has been added by
> commit bac2fae05c to ensure that we process XLOG_XACT_ASSIGNMENT even
> if snapshot state is not SNAPBUILD_FULL_SNAPSHOT. Now, with this
> patch that is not required because we are making the subtransaction
> and top-level transaction much earlier than this. I have verified
> that it doesn't reopen the bug by running the test provided in the
> original report [1].
>
> 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.

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