Re: pgsql: pgstat: Bring up pgstat in BaseInit() to fix uninitialized use o

On Sat, Aug 21, 2021 at 9:38 PM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
>
> On Wed, Aug 18, 2021 at 3:45 PM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
> >
> > On Wed, Aug 18, 2021 at 11:24 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> > >
> > > On Tue, Aug 17, 2021 at 4:34 PM Andres Freund <andres(at)anarazel(dot)de> wrote:
> > > >
> > > > On 2021-08-17 10:54:30 +0530, Amit Kapila wrote:
> > > > > 5. How can we provide a strict mechanism to not allow to use dsm APIs
> > > > > for non-dsm FileSet? One idea could be that we can have a variable
> > > > > (probably bool) in SharedFileSet structure which will be initialized
> > > > > in SharedFileSetInit based on whether the caller has provided dsm
> > > > > segment. Then in other DSM-based APIs, we can check if it is used for
> > > > > the wrong type.
> > > >
> > > > Well, isn't the issue here that it's not a shared file set in case you
> > > > explicitly don't want to share it? ISTM that the proper way to address
> > > > this would be to split out a FileSet from SharedFileSet that's then used
> > > > for worker.c and sharedfileset.c.
> > > >
> > >
> > > Okay, but note that to accomplish the same, we need to tweak the
> > > BufFile (buffile.c) APIs as well so that they can work with FileSet.
> > > As per the initial analysis, there doesn't seem to be any problem with
> > > that though.
> >
> > 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.

Here are some comments on 0001 patch:

+/*
+ * Initialize a space for temporary files. This API can be used by shared
+ * fileset as well as 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.
+ *
+ * Files will be distributed over the tablespaces configured in
+ * temp_tablespaces.
+ *
+ * Under the covers the set is one or more directories which will eventually
+ * be deleted.
+ */

I think it's better to mention cleaning up here like we do in the
comment of SharedFileSetInit().

---
I think we need to clean up both stream_fileset and subxact_fileset on
proc exit. In 0002 patch cleans up the fileset but I think we need to
do that also in 0001 patch.

---
There still are some comments using "shared fileset" in both buffile.c
and worker.c.

Regards,

--
Masahiko Sawada
EDB: https://www.enterprisedb.com/