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." 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' Other than that the changes in this particular patch looks good to me. -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
