Re: Separate out FileSet from SharedFileSet (was Re: pgsql: pgstat: Bring up pgstat in BaseInit() to fix uninitialized use o)

On Mon, Aug 23, 2021 at 11:43 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Mon, Aug 23, 2021 at 9:53 AM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
> >
> > On Mon, Aug 23, 2021 at 9:11 AM houzj(dot)fnst(at)fujitsu(dot)com
> > <houzj(dot)fnst(at)fujitsu(dot)com> wrote:
> >
> > > 4)
> > > -extern File SharedFileSetCreate(SharedFileSet *fileset, const char *name);
> > > -extern File SharedFileSetOpen(SharedFileSet *fileset, const char *name,
> > > - int mode);
> > > -extern bool SharedFileSetDelete(SharedFileSet *fileset, const char *name,
> > > - bool error_on_failure);
> > > extern void SharedFileSetDeleteAll(SharedFileSet *fileset);
> > > -extern void SharedFileSetUnregister(SharedFileSet *input_fileset);
> > >
> > > I noticed the patch delete part of public api, is it better to keep the old api and
> > > let them invoke new api internally ? Having said that, I didn’t find any open source
> > > extension use these old api, so it might be fine to delete them.
> >
> > Right, those were internally used by buffile.c but now we have changed
> > buffile.c to directly use the fileset interfaces, so we better remove
> > them.
> >
>
> I also don't see any reason to keep those exposed from
> sharedfileset.h. I see that even in the original commit dc6c4c9dc2,
> these APIs seem to be introduced to be used by buffile. Andres/Thomas,
> do let us know if you think otherwise?
>
> One more comment:
> I think v1-0001-Sharedfileset-refactoring doesn't have a way for
> cleaning up worker.c temporary files on error/exit. It is better to
> have that to make it an independent patch.

I think we should handle that in worker.c itself, by adding a
before_dsm_detach function before_shmem_exit right? Or you are
thinking that in FileSetInit, we keep the mechanism of filesetlist
like we were doing in SharedFileSetInit? I think that will just add
unnecessary complexity in the first patch which will eventually go
away in the second patch. And if we do that then SharedFileSetInit
can not directly use the FileSetInit, otherwise, the dsm based fileset
will also get registered for cleanup in filesetlist so for that we
might need to pass one parameter to the FileSetInit() that whether to
register for cleanup or not and that will again not look clean because
now we are again adding the conditional cleanup, IMHO that is the same
problem what we are trying to cleanup in SharedFileSetInit by
introducing a new FileSetInit.

I think what we can do is, introduce a new function
FileSetInitInternal(), that will do what FileSetInit() is doing today
and now both SharedFileSetInit() and the FileSetInit() will call this
function, and along with that SharedFileSetInit(), will register the
dsm based cleanup and FileSetInit() will do the filesetlist based
cleanup. But IMHO, we should try to go in this direction only if we
are sure that we want to commit the first patch and not the second.

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