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

On Thu, Nov 7, 2019 at 3:19 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Wed, Nov 6, 2019 at 11:33 AM vignesh C <vignesh21(at)gmail(dot)com> wrote:
> >
> > I have made one change to the configuration file in
> > contrib/test_decoding directory, with that the coverage seems to be
> > fine. I have seen that the coverage is almost like the code before
> > applying the patch. I have attached the test change and the coverage
> > report for reference. Coverage report includes the core logical work
> > memory files for base code and by applying
> > 0001-Add-logical_decoding_work_mem-to-limit-ReorderBuffer and
> > 0002-Track-statistics-for-spilling patches.
> >
>
> Thanks, I have incorporated your test changes and modified the two
> patches. Please see attached.
>
> Changes:
> ---------------
> 1. In guc.c, we should include reorderbuffer.h, not logical.h as we
> define logical_decoding_work_mem in earlier.
Yeah Right.
>
> 2.
> + * To limit the amount of memory used by decoded changes, we track memory
> + * used at the reorder buffer level (i.e. total amount of memory), and for
> + * each toplevel transaction. When the total amount of used memory exceeds
> + * the limit, the toplevel transaction consuming the most memory is then
> + * serialized to disk.
>
> In the above comments, removed 'toplevel' as we track memory usage for
> both toplevel and subtransactions.
Correct.
>
> 3. There were still a few mentions of streaming which I have removed.
>
ok
> 4. In the docs, the type for stats spill_* was integer whereas it
> should be bigint.
ok
>
> 5.
> +UpdateSpillStats(LogicalDecodingContext *ctx)
> +{
> + ReorderBuffer *rb = ctx->reorder;
> +
> + SpinLockAcquire(&MyWalSnd->mutex);
> +
> + MyWalSnd->spillTxns = rb->spillTxns;
> + MyWalSnd->spillCount = rb->spillCount;
> + MyWalSnd->spillBytes = rb->spillBytes;
> +
> + elog(WARNING, "UpdateSpillStats: updating stats %p %ld %ld %ld",
> + rb, rb->spillTxns, rb->spillCount, rb->spillBytes);
>
> Changed the above elog to DEBUG1 as otherwise it was getting printed
> very frequently. I think we can make it DEBUG2 if we want.
Yeah, it should not be WARNING.
>
> 6. There was an extra space in rules.out due to which test was
> failing. I have fixed it.
My Bad. I have induced while separating out the changes for the spilling.

> What do you think?
I have reviewed your changes and looks fine to me.

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