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

Hi,

On 2021-08-12 15:06:23 +0530, Amit Kapila wrote:
> On Thu, Aug 12, 2021 at 1:52 PM Andres Freund <andres(at)anarazel(dot)de> wrote:
> > I'm not so sure. Why does sharedfileset have its own proc exit hook in the
> > first place? ISTM that this should be dealt with using resowners, rathers than
> > a sharedfileset specific mechanism?
> >

> The underlying temporary files need to be closed at xact end but need
> to survive across transactions.

Why do they need to be closed at xact end? To avoid wasting memory due to too
many buffered files?

> These are registered with the resource owner via
> PathNameOpenTemporaryFile/PathNameCreateTemporaryFile and then closed
> at xact end. So, we need a way to remove the files used by the process
> (apply worker in this particular case) before process exit and used
> this proc_exit hook (possibly on the lines of AtProcExit_Files).

What I'm wondering is why it is a good idea to have a SharedFileSet specific
cleanup mechanism. One that only operates on process lifetime level, rather
than something more granular. I get that the of the files here needs to be
longer than a transaction, but that can easily be addressed by having a longer
lived resource owner.

Process lifetime may work well for the current worker.c, but even there it
doesn't seem optimal. One e.g. could easily imagine that we'd want to handle
connection errors or configuration changes without restarting the worker, in
which case process lifetime obviously isn't a good idea anymore.

I think SharedFileSetInit() needs a comment explaining that it needs to be
called in a process-lifetime memory context if used without dsm
segments. Because otherwise SharedFileSetDeleteOnProcExit() will access
already freed memory (both for filesetlist and the SharedFileSet itself).

Greetings,

Andres Freund