Andres Freund <and...@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
