Re: missing RelationCloseSmgr in FreeFakeRelcacheEntry? - Mailing list pgsql-hackers

On 2013-11-04 14:37:53 +0200, Heikki Linnakangas wrote:
> On 04.11.2013 11:35, Andres Freund wrote:
> >On 2013-11-04 09:38:27 +0200, Heikki Linnakangas wrote:
> >>diff --git a/src/backend/access/transam/xlogutils.c b/src/backend/access/transam/xlogutils.c
> >>index 5429d5e..f732e71 100644
> >>--- a/src/backend/access/transam/xlogutils.c
> >>+++ b/src/backend/access/transam/xlogutils.c
> >>@@ -422,7 +422,11 @@ CreateFakeRelcacheEntry(RelFileNode rnode)
> >>      rel->rd_lockInfo.lockRelId.dbId = rnode.dbNode;
> >>      rel->rd_lockInfo.lockRelId.relId = rnode.relNode;
> >>
> >>-    rel->rd_smgr = NULL;
> >>+    /*
> >>+     * Open the underlying storage, but don't set rd_owner. We want the
> >>+     * SmgrRelation to persist after the fake relcache entry is freed.
> >>+     */
> >>+    rel->rd_smgr = smgropen(rnode, InvalidBackendId);
> >>
> >>      return rel;
> >>  }
> >
> >I don't really like that - that will mean we'll leak open smgr handles
> >for every relation touched via smgr during recovery since they will
> >never be closed unless the relation is dropped or such. And in some
> >databases there can be huge amounts of relations.
> >Since recovery is long lived that doesn't seem like a good idea, besides
> >the memory usage it will also bloat smgr's internal hash which actually
> >seems just as likely to hurt performance.
> 
> That's the way SmgrRelations are supposed to be used. Opened on first use,
> and kept open forever. We never smgrclose() during normal operation either,
> unless the relation is dropped or something like that. And see how
> XLogReadBuffer() also calls smgropen() with no corresponding smgrclose().

Ok, that's quite the fair argument although I'd say normal backends
won't open many relations in comparison to the startup process when
doing replication. But that certainly isn't sufficient argument for
changing logic in the back branches or even HEAD.

> >I think intentionally not using the owner mechanism also is dangerous -
> >what if we have longer lived fake relcache entries and we've just
> >processed sinval messages? Then we'll have a ->rd_smgr pointing into
> >nowhere.

> Hmm, the startup process doesn't participate in sinval messaging at all,
> does it?

Well, not sinval but inval, in hot standby via commit messages.

What about just unowning the smgr entry with
if (rel->rd_smgr != NULL)  smgrsetowner(NULL, rel->rd_smgr)
when closing the fake relcache entry?

That shouldn't require any further changes than changing the comment in
smgrsetowner() that this isn't something expected to frequently happen.

Greetings,

Andres Freund

-- Andres Freund                       http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services