On Thu, Aug 17, 2023 at 1:11 AM Thomas Munro <thomas.mu...@gmail.com> wrote: > Still WIP while I think about edge cases, but so far I think this is > the better option.
I think this direction makes a lot of sense. The lack of a defined lifetime for SMgrRelation objects makes correct programming difficult, makes efficient programming difficult, and doesn't really have any advantages. I know this is just a WIP patch but for the final version I think it would make sense to try to do a bit more work on the comments. For instance: - In src/include/utils/rel.h, instead of just deleting that comment, how about documenting the new object lifetime? Or maybe that comment belongs elsewhere, but I think it would definitely good to spell it out very explicitly at some suitable place. - When we change smgrcloseall() to smgrdestroyall(), maybe it's worth spelling out why destroying is needed and not just closing. For example, the second hunk in bgwriter.c includes a comment that says "After any checkpoint, close all smgr files. This is so we won't hang onto smgr references to deleted files indefinitely." But maybe it should say something like "After any checkpoint, close all smgr files and destroy the associated smgr objects. This guarantees that we close the actual file descriptors, that we close the File objects as managed by fd.c, and that we also destroy the smgr objects. We don't want any of these resources to stick around indefinitely after a relation file has been deleted." - Maybe it's worth adding comments around some of the smgrclose[all] calls to mentioning that in those cases we want the file descriptors (and File objects?) to get closed but don't want to invalidate pointers. But I'm less sure that this is necessary. I don't want to have a million comments everywhere, just enough that someone looking at this stuff in the future can orient themselves about what's going on without too much difficulty. -- Robert Haas EDB: http://www.enterprisedb.com
