On Fri, Jun 26, 2020 at 11:47 AM Amit Kapila <amit.kapil...@gmail.com> wrote: > > On Thu, Jun 25, 2020 at 7:11 PM Dilip Kumar <dilipbal...@gmail.com> wrote: > > > > On Wed, Jun 24, 2020 at 4:04 PM Amit Kapila <amit.kapil...@gmail.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
