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

On Tue, Aug 17, 2021 at 10:54 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Mon, Aug 16, 2021 at 8:18 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> >
> > On Fri, Aug 13, 2021 at 9:29 PM Andres Freund <andres@anarazel.de> wrote:
> >
> > > > I think we can extend this API but I guess it is better to then do it
> > > > for dsm-based as well so that these get tracked via resowner.
> > >
> > > DSM segments are resowner managed already, so it's not obvious that that'd buy
> > > us much? Although I guess there could be a few edge cases that'd look cleaner,
> > > because we could reliably trigger cleanup in the leader instead of relying on
> > > dsm detach hooks + refcounts to manage when a set is physically deleted?
> > >
> >
> > In an off-list discussion with Thomas and Amit, we tried to discuss
> > how to clean up the shared files set in the current use case.  Thomas
> > suggested that instead of using individual shared fileset for storing
> > the data for each XID why don't we just create a single shared fileset
> > for complete worker lifetime and when the worker is exiting we can
> > just remove that shared fileset.  And for storing XID data, we can
> > just create the files under the same shared fileset and delete those
> > files when we longer need them.  I like this idea and it looks much
> > cleaner, after this, we can get rid of the special cleanup mechanism
> > using 'filesetlist'.  I have attached a patch for the same.
> >
>
> It seems to me that this idea would obviate any need for resource
> owners as we will have only one fileset now. I have a few initial
> comments on the patch:
>
> 1.
> + /* do cleanup on worker exit (e.g. after DROP SUBSCRIPTION) */
> + on_shmem_exit(worker_cleanup, (Datum) 0);
>
> It should be registered with before_shmem_exit() hook to allow sending
> stats for file removal.

Done

> 2. After these changes, the comments atop stream_open_file and
> SharedFileSetInit need to be changed.

Done

> 3. In function subxact_info_write(), we are computing subxact file
> path twice which doesn't seem to be required.

Fixed

> 4.
> + if (missing_ok)
> + return NULL;
>   ereport(ERROR,
>   (errcode_for_file_access(),
> - errmsg("could not open temporary file \"%s\" from BufFile \"%s\": %m",
> + errmsg("could not open temporary file \"%s\" from BufFile \"%s\": %m",
>   segment_name, name)));
>
> There seems to be a formatting issue with errmsg. Also, it is better
> to keep an empty line before ereport.

Done

> 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.

Yeah, we can do something like that, can't we just use an existing
variable instead of adding new, e.g. refcnt is required only when
multiple processes are attached, so maybe if dsm segment is not passed
then we can keep refcnt as 0 and based on we can give an error.  For
example, if we try to call SharedFileSetAttach for the SharedFileSet
which has refcnt as 0 then we error out?

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