From: Tom Lane <tgl@sss.pgh.pa.us>
To: Andres Freund <andres@anarazel.de>
cc: Robert Haas <robertmhaas@gmail.com>,
        Alvaro Herrera <alvherre@2ndquadrant.com>,
        PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: hyrax vs. RelationBuildPartitionDesc
In-reply-to: <20190517195929.uey6hadbgazplm3h@alap3.anarazel.de>
References: <10788.1552501588@sss.pgh.pa.us>
 <CA+TgmoYd3vqd26RiT4sp7Mez3oQf-AxAzdBpEeTyCNF-7OchhA@mail.gmail.com>
 <CA+TgmoY71GwY8WhQKBH+7yexyZEw6WDbFBtj2E3EDszc0LnSqQ@mail.gmail.com>
 <3969.1552581380@sss.pgh.pa.us>
 <CA+TgmoYg2NY3KTUFiuaCxDkW8iXD3o3H3wdXs6m-o-dGiYHD_Q@mail.gmail.com>
 <6122.1552673884@sss.pgh.pa.us>
 <CA+TgmoYMoUDQH7kGqbEUDBJkcr-G_Di6pu04uL6UPexgcodF0Q@mail.gmail.com>
 <10797.1552679128@sss.pgh.pa.us>
 <20190501155912.dgp6sg4vwueewd5l@alap3.anarazel.de>
 <CA+Tgmob-cska+-WUC7T-G4zkSJp7yum6M_bzYd4YFzwQ51qiow@mail.gmail.com>
 <20190517195929.uey6hadbgazplm3h@alap3.anarazel.de>
Comments: In-reply-to Andres Freund <andres@anarazel.de>
	message dated "Fri, 17 May 2019 12:59:29 -0700"
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <7896.1558125372.1@sss.pgh.pa.us>
Date: Fri, 17 May 2019 16:36:12 -0400
Message-ID: <7897.1558125372@sss.pgh.pa.us>
Precedence: bulk

Andres Freund <andres@anarazel.de> writes:
> On 2019-05-01 13:09:07 -0400, Robert Haas wrote:
>> In the email at issue, Tom complains about two things.  First, he
>> complains about the fact that I try to arrange things so that relcache
>> data remains valid for as long as required instead of just copying it.
>> Second, he complains about the fact repeated ATTACH and DETACH
>> PARTITION operations can produce a slow session-lifespan memory leak.
>> 
>> I think it's reasonable to fix the second issue for v12.  I am not
>> sure how important it is, because (1) the leak is small, (2) it seems
>> unlikely that anyone would execute enough ATTACH/DETACH PARTITION
>> operations in one backend for the leak to amount to anything
>> significant, and (3) if a relcache flush ever happens when you don't
>> have the relation open, all of the "leaked" memory will be un-leaked.

Yeah, I did some additional testing that showed that it's pretty darn
hard to get the leak to amount to anything.  The test case that I
originally posted did many DDLs in a single transaction, and it
seems that that's actually essential to get a meaningful leak; as
soon as you exit the transaction the leaked contexts will be recovered
during sinval cleanup.

>> However, I believe that we could fix it as follows.  First, invent
>> rd_pdoldcxt and put the first old context there; if that pointer is
>> already in use, then parent the new context under the old one.
>> Second, in RelationDecrementReferenceCount, if the refcount hits 0,
>> nuke rd_pdoldcxt and set the pointer back to NULL.  With that change,
>> you would only keep the old PartitionDesc around until the ref count
>> hits 0, whereas at present, you keep the old PartitionDesc around
>> until an invalidation happens while the ref count is 0.

> That sounds roughly reasonably. Tom, any objections?  I think it'd be
> good if we fixed this soon.

My fundamental objection is that this kluge is ugly as sin.
Adding more ugliness on top of it doesn't improve that; rather
the opposite.

> Tom, are you ok with deferring further work here for v13?

Yeah, I think that that's really what we ought to do at this point.
I'd like to see a new design here, but it's not happening for v12,
and I don't want to add even more messiness that's predicated on
a wrong design.

			regards, tom lane