From: Tom Lane <tgl@sss.pgh.pa.us>
To: Andres Freund <andres@anarazel.de>
Cc: Robert Haas <robertmhaas@gmail.com>
Cc: Alvaro Herrera <alvherre@2ndquadrant.com>
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: hyrax vs. RelationBuildPartitionDesc
Date: Fri, 17 May 2019 16:36:12 -0400
Message-ID: <7897.1558125372@sss.pgh.pa.us> (raw)
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>
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: tgl@sss.pgh.pa.us, andres@anarazel.de, robertmhaas@gmail.com, alvherre@2ndquadrant.com, pgsql-hackers@lists.postgresql.org
Subject: Re: hyrax vs. RelationBuildPartitionDesc
In-Reply-To: <7897.1558125372@sss.pgh.pa.us>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox