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

On Fri, Jun 26, 2020 at 11:47 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Thu, Jun 25, 2020 at 7:11 PM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
> >
> > On Wed, Jun 24, 2020 at 4:04 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> > >
> > >
> > > Review comments on various patches.
> > >
> > > poc_shared_fileset_cleanup_on_procexit
> > > =================================
> > > 1.
> > > - ent->subxact_fileset =
> > > - MemoryContextAlloc(ApplyContext, sizeof(SharedFileSet));
> > > + MemoryContext oldctx;
> > >
> > > + /* Shared fileset handle must be allocated in the persistent context */
> > > + oldctx = MemoryContextSwitchTo(ApplyContext);
> > > + ent->subxact_fileset = palloc(sizeof(SharedFileSet));
> > > SharedFileSetInit(ent->subxact_fileset, NULL);
> > > + MemoryContextSwitchTo(oldctx);
> > > fd = BufFileCreateShared(ent->subxact_fileset, path);
> > >
> > > Why is this change required for this patch and why we only cover
> > > SharedFileSetInit in the Apply context and not BufFileCreateShared?
> > > The comment is also not very clear on this point.
> >
> > Added the comments for the same.
> >
>
> 1.
> + /*
> + * Shared fileset handle must be allocated in the persistent context.
> + * Also, SharedFileSetInit allocate the memory for sharefileset list
> + * so we need to allocate that in the long term meemory context.
> + */
>
> How about "We need to maintain shared fileset across multiple stream
> open/close calls. So, we allocate it in a persistent context."

Done

> 2.
> + /*
> + * If the caller is following the dsm based cleanup then we don't
> + * maintain the filesetlist so return.
> + */
> + if (filesetlist == NULL)
> + return;
>
> The check here should use 'NIL' instead of 'NULL'

Done

> Other than that the changes in this particular patch looks good to me.
Added as a last patch in the series, in the next version I will merge
this to 0012 and 0013.

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