Re: Extending SMgrRelation lifetimes

On 14/08/2023 05:41, Thomas Munro wrote:
> 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.

Makes sense.

Some of the smgrclose() calls could perhaps still be smgrclose(). If you
can guarantee that no-one is holding the SMgrRelation, it's still OK to
call smgrclose(). There's little gain from closing earlier, though.

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

If you change smgrclose() to do what smgrrelease() does now, then it
will apply automatically to extensions.

If an extension is currently using smgropen()/smgrclose() correctly,
this patch alone won't make it wrong, so it's not very critical for
extensions to adopt the change. However, if after this we consider it OK
to hold a pointer to SMgrRelation for longer, and start doing that in
the backend, then extensions need to be adapted too.

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

+1. You can move the smgr_targblock clearing out of the loop, though.

--
Heikki Linnakangas
Neon (https://neon.tech)