Re: Memory leak in RelationBuildRowSecurity

On 25.09.2020 21:07, Tom Lane wrote:
> Konstantin Knizhnik <k(dot)knizhnik(at)postgrespro(dot)ru> writes:
>> On 25.09.2020 16:48, Tom Lane wrote:
>>> Hm ... this smells a whole lot like the issue we fixed recently for
>>> partition expressions, namely: what the devil are we doing reloading
>>> these expressions during RelationClearRelation? We should delay
>>> populating that cache until the value is requested.
>> Sorry, this stack trace was obtained at 9.6 version of Postgres which
>> the customer is using.
>> So may be the problem with cache invalidation is already fixed.
> Not sure if we've changed anything much in that area. I've occasionally
> wondered if we should not increase the size of the sinval message queue,
> but it hasn't happened yet.
>
> Anyway, looking more closely at RelationBuildRowSecurity, it doesn't
> seem to be doing anything more dangerous than RelationBuildRuleLock
> is; it's definitely not risking recursion, as the partition stuff did.
> So I take back the idea that we need to postpone it till the data is
> referenced. However, it is leaking memory.
>
> Attached is a proposed rewrite that borrows some ideas from
> RelationBuildRuleLock, and also makes use of MemoryContextSetParent
> so we don't need a PG_TRY block. In a very simple test
> (using regress_rls_schema.rls_tbl from the regression database),
> this doesn't seem to leak any memory in the caller context,
> and it also doesn't make the rscxt any bigger than it was before.
>
>> But at least calling pfree for a tree is very strange idea: we should
>> not do it all or allocate tree in separate memory context.
> Yeah, that's pretty useless. But also I think the string forms of
> the expressions are leaking as much memory as the tree forms.
>
> This is against HEAD but I think it wouldn't be hard to back-patch.
> Are you in a position to see if it fixes the problem in your customer's
> environment?

Thank you.
Your patch fixes the problem.