On Thu, Nov 7, 2019 at 3:19 PM Amit Kapila <amit.kapil...@gmail.com> wrote: > > On Wed, Nov 6, 2019 at 11:33 AM vignesh C <vignes...@gmail.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