Extending SMgrRelation lifetimes
Thomas Munro <thomas.munro@gmail.com>
From: Thomas Munro <thomas.munro@gmail.com>
To: pgsql-hackers <pgsql-hackers@postgresql.org>
Date: 2023-08-14T02:41:56Z
Lists: pgsql-hackers
Attachments
- 0001-Invalidate-smgr_targblock-on-release.patch (text/x-patch) patch 0001
- 0002-Give-SMgrRelation-pointers-a-well-defined-lifetime.patch (text/x-patch) patch 0002
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.
Commits
-
Give SMgrRelation pointers a well-defined lifetime.
- 21d9c3ee4ef7 17.0 landed
-
Remove some obsolete smgrcloseall() calls.
- 6a8ffe812d19 17.0 landed
-
Don't try to open visibilitymap when analyzing a foreign table
- 049ef3398d05 17.0 landed
-
Invalidate smgr_targblock in smgrrelease().
- d23e60e6673a 15.5 landed
- 6337e994e897 16.0 landed
- 81e36d3e0df8 17.0 landed