Subject: Re: Memory leak in RelationBuildRowSecurity
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: pgsql-bugs@lists.postgresql.org
References: <21356c12-8917-8249-b35f-1c447231922b@postgrespro.ru>
 <1852787.1601041699@sss.pgh.pa.us>
 <3f670fa6-5e81-fbd9-b880-eaa275f15e1c@postgrespro.ru>
 <1937613.1601057234@sss.pgh.pa.us>
From: Konstantin Knizhnik <k.knizhnik@postgrespro.ru>
Message-ID: <c28d5597-1933-1933-c15c-edadd9186757@postgrespro.ru>
Date: Sat, 26 Sep 2020 19:37:45 +0300
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.10.0
MIME-Version: 1.0
In-Reply-To: <1937613.1601057234@sss.pgh.pa.us>
Content-Type: text/plain; charset=windows-1252; format=flowed
Content-Transfer-Encoding: 7bit
Content-Language: en-US
Precedence: bulk


On 25.09.2020 21:07, Tom Lane wrote:
> Konstantin Knizhnik <k.knizhnik@postgrespro.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.