RE: pgsql: pgstat: Bring up pgstat in BaseInit() to fix uninitialized use o - Mailing list pgsql-hackers

On Sat, Aug 21, 2021 8:38 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> On Wed, Aug 18, 2021 at 3:45 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> > I was looking into this, so if we want to do that I think the outline
> > will look like this
> >
> > - There will be a fileset.c and fileset.h files, and we will expose a
> > new structure FileSet, which will be the same as SharedFileSet, except
> > mutext and refcount.  The fileset.c will expose FileSetInit(),
> > FileSetCreate(), FileSetOpen(), FileSetDelete() and FileSetDeleteAll()
> > interfaces.
> >
> > - sharefileset.c will internally call the fileset.c's interfaces.  The
> > SharedFileSet structure will also contain FileSet and other members
> > i.e. mutex and refcount.
> >
> > - the buffile.c's interfaces which are ending with Shared e.g.
> > BufFileCreateShared, BufFileOpenShared, should be converted to
> > BufFileCreate and BufFileOpen respectively.  And the input to these
> > interfaces can be converted to FileSet instead of SharedFileSet.
> 
> Here is the first draft based on the idea we discussed, 0001, splits
> sharedfileset.c in sharedfileset.c and fileset.c and 0002 is same patch I
> submitted earlier(use single fileset throughout the worker), just it is rebased on
> top of 0001.  Please let me know your thoughts.

Hi,

Here are some comments for the new version patches.

1)
+    TempTablespacePath(tempdirpath, tablespace);
+    snprintf(path, MAXPGPATH, "%s/%s%lu.%u.sharedfileset",
+             tempdirpath, PG_TEMP_FILE_PREFIX,
+             (unsigned long) fileset->creator_pid, fileset->number);

do we need to use different filename for shared and un-shared fileset ?

2)
I think we can remove or adjust the following comments in sharedfileset.c.

----
 * SharedFileSets can be used by backends when the temporary files need to be
 * opened/closed multiple times and the underlying files need to survive across
 * transactions.
----
 * We can also use this interface if the temporary files are used only by
 * single backend but the files need to be opened and closed multiple times
 * and also the underlying files need to survive across transactions.  For
----

3)
The 0002 patch still used the word "shared fileset" in some places, I think we
should change it to "fileset".

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.

Best regards,
Hou zj