Re: Extending SMgrRelation lifetimes

On Fri, Aug 18, 2023 at 2:30 AM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> 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.

Thanks for looking!

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

Right, let's one find one good place. I think smgropen() would be best.

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

There are several similar comments. I think they can be divided into
two categories:

1. The error-path ones, that we should now just delete along with the
code they describe, because the "various strange errors" should have
been fixed comprehensively by PROCSIGNAL_BARRIER_SMGRRELEASE. Here is
a separate patch to do that.

2. The per-checkpoint ones that still make sense to avoid unbounded
resource usage. Here is a new attempt at explaining:

/*
- * After any checkpoint, close all smgr files.
This is so we
- * won't hang onto smgr references to deleted
files indefinitely.
+ * After any checkpoint, free all smgr
objects. Otherwise we
+ * would never do so for dropped relations, as
the checkpointer
+ * does not process shared invalidation messages or call
+ * AtEOXact_SMgr().
*/
- smgrcloseall();
+ smgrdestroyall();

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

I covered that with the following comment for smgrclose():

+ * The object remains valid, but is moved to the unknown list where it will
+ * be destroyed by AtEOXact_SMgr(). It may be re-owned if it is accessed by a
+ * relation before then.