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

On Wed, Jul 15, 2020 at 9:29 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:
>
>
> I have reviewed your changes and those look good to me,  please find
> the latest version of the patch set.
>

I have done an additional round of review and below are the changes I
made in the attached patch-set.
1. Changed comments in 0002.
2. In 0005, apart from changing a few comments and function name, I
have changed below code:
+ if (ReorderBufferCanStream(rb) &&
+ !SnapBuildXactNeedsSkip(builder, ctx->reader->ReadRecPtr))
Here, I think it is better to compare it with EndRecPtr.  I feel in
boundary case the next record could be the same as start_decoding_at,
so why to avoid streaming in that case?
3. In 0006, made below changes:
    a. Removed function ReorderBufferFreeChange and added a new
parameter in ReorderBufferReturnChange to achieve the same purpose.
    b. Changed quite a few comments, function names, added additional
Asserts, and few other cosmetic changes.
4. In 0007, made below changes:
    a. Removed the unnecessary change in .gitignore
    b. Changed the newly added option name to "stream-change".

Apart from above, I have merged patches 0004, 0005, 0006 and 0007 as
those seems one functionality to me.  For the sake of review, the
patch-set that contains merged patches is attached separately as
v34-combined.

Let me know what you think of the changes?

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com