Hi,
SMgrRelationData objects don't currently have a defined lifetime, so
it's hard to know when the result of smgropen() might become a
dangling pointer. This has caused a few bugs in the past, and the
usual fix is to just call smgropen() more often and not hold onto
pointers. If you're doing that frequently enough, the hash table
lookups can show up in profiles. I'm interested in this topic for
more than just micro-optimisations, though: in order to be able to
batch/merge smgr operations, I'd like to be able to collect them in
data structures that survive more than just a few lines of code.
(Examples to follow in later emails).
The simplest idea seems to be to tie object lifetime to transactions
using the existing AtEOXact_SMgr() mechanism. In recovery, the
obvious corresponding time would be the commit/abort record that
destroys the storage.
This could be achieved by extending smgrrelease(). That was a
solution to the same problem in a narrower context: we didn't want
CFIs to randomly free SMgrRelations, but we needed to be able to
force-close fds in other backends, to fix various edge cases.
The new idea is to overload smgrrelease(it) so that it also clears the
owner, which means that AtEOXact_SMgr() will eventually smgrclose(it),
unless it is re-owned by a relation before then. That choice stems
from the complete lack of information available via sinval in the case
of an overflow. We must (1) close all descriptors because any file
might have been unlinked, (2) keep all pointers valid and yet (3) not
leak dropped smgr objects forever. In this patch, smgrreleaseall()
achieves those goals.
Proof-of-concept patch attached. Are there holes in this scheme?
Better ideas welcome. In terms of spelling, another option would be
to change the behaviour of smgrclose() to work as described, ie it
would call smgrrelease() and then also disown, so we don't have to
change most of those callers, and then add a new function
smgrdestroy() for the few places that truly need it. Or something
like that.
Other completely different ideas I've bounced around with various
hackers and decided against: references counts, "holder" objects that
can be an "owner" (like Relation, but when you don't have a Relation)
but can re-open on demand. Seemed needlessly complicated.
While studying this I noticed a minor thinko in smgrrelease() in
15+16, so here's a fix for that also. I haven't figured out a
sequence that makes anything bad happen, but we should really
invalidate smgr_targblock when a relfilenode is reused, since it might
point past the end. This becomes more obvious once smgrrelease() is
used for truncation, as proposed here.