Re: missing RelationCloseSmgr in FreeFakeRelcacheEntry?

On 04.11.2013 11:35, Andres Freund wrote:
> On 2013-11-04 09:38:27 +0200, Heikki Linnakangas wrote:
>> Secondly, it will fail if you create
>> two fake relcache entries for the same relfilenode. Freeing the first will
>> close the smgr entry, and freeing the second will try to close the same smgr
>> entry again. We never do that in the current code, but it seems like a
>> possible source of bugs in the future.
>
> I think if we go there we'll need refcounted FakeRelcacheEntry's
> anyway. Otherwise we'll end up with a relation smgropen()ed multiple
> times in the same backend and such which doesn't seem like a promising
> course to me since the smgr itself isn't refcounted.
>
>> How about the attached instead?
>
>> 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().

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

- Heikki