On Wed, Aug 18, 2021 at 9:30 AM Amit Kapila <amit.kapil...@gmail.com> wrote: > > On Wed, Aug 18, 2021 at 8:23 AM houzj.f...@fujitsu.com > <houzj.f...@fujitsu.com> wrote: > > > > On Wed, Aug 18, 2021 9:17 AM houzj.f...@fujitsu.com wrote: > > > Hi, > > > > > > I took a quick look at the v2 patch and noticed a typo. > > > > > > + * backends and render it read-only. If missing_ok is true then it will > > > return > > > + * NULL if file doesn not exist otherwise error. > > > */ > > > doesn not=> doesn't > > > > > > > Here are some other comments: > > > > 1). > > +BufFileOpenShared(SharedFileSet *fileset, const char *name, int mode, > > + bool missing_ok) > > { > > BufFile *file; > > char segment_name[MAXPGPATH]; > > ... > > files = palloc(sizeof(File) * capacity); > > ... > > @@ -318,10 +320,15 @@ BufFileOpenShared(SharedFileSet *fileset, const char > > *name, int mode) > > * name. > > */ > > if (nfiles == 0) > > + { > > + if (missing_ok) > > + return NULL; > > + > > > > I think it might be better to pfree(files) when return NULL. > > > > > > 2). > > /* Delete the subxact file and release the memory, if it exist */ > > - if (ent->subxact_fileset) > > - { > > - subxact_filename(path, subid, xid); > > - SharedFileSetDeleteAll(ent->subxact_fileset); > > - pfree(ent->subxact_fileset); > > - ent->subxact_fileset = NULL; > > - } > > - > > - /* Remove the xid entry from the stream xid hash */ > > - hash_search(xidhash, (void *) &xid, HASH_REMOVE, NULL); > > + subxact_filename(path, subid, xid); > > + SharedFileSetDelete(xidfileset, path, true); > > > > Without the patch it doesn't throw an error if not exist, > > But with the patch, it pass error_on_failure=true to SharedFileSetDelete(). > > > > Don't we need to use BufFileDeleteShared instead of > SharedFileSetDelete as you have used to remove the changes file?
Yeah, it should be BufFileDeleteShared. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com