On Thu, Aug 12, 2021 at 6:18 PM Andres Freund <and...@anarazel.de> wrote: > > Hi, > > On 2021-08-12 15:06:23 +0530, Amit Kapila wrote: > > On Thu, Aug 12, 2021 at 1:52 PM Andres Freund <and...@anarazel.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? >
Yes. > > > 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 don't deny that we can't make this at a more granular level. IIRC, at that time, we tried to follow AtProcExit_Files which cleans up temp files at proc exit and we needed something similar for temporary files used via SharedFileSet. 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. > > I think SharedFileSetInit() needs a comment explaining that it needs to be > called in a process-lifetime memory context if used without dsm > segments. > We already have a comment about proc_exit clean up of files but will extend that a bit about memory context. -- With Regards, Amit Kapila.