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

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.

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

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

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.

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.

-- 
With Regards,
Amit Kapila.