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)