Amit Langote <amitlangote09@gmail.com> writes:
>> Fwiw, I am fine with applying the memory-leak fix in all branches down
>> to v12 if we are satisfied with the implementation.
> I have revised the above patch slightly to introduce a variable for
> the condition whether to use a temporary memory context.
This CF entry has been marked "ready for committer", which I find
inappropriate since there doesn't seem to be any consensus about
whether we need it.
I tried running the original test case under HEAD. I do not see
any visible memory leak, which I think indicates that 5b9312378 or
some other fix has taken care of the leak since the original report.
However, after waiting awhile and noting that the ADD FOREIGN KEY
wasn't finishing, I poked into its progress with a debugger and
observed that each iteration of RI_Initial_Check() was taking about
15 seconds. I presume we have to do that for each partition,
which leads to the estimate that it'll take 34 hours to finish this
... and that's with no data in the partitions, god help me if
there'd been a lot.
Some quick "perf" work says that most of the time seems to be
getting spent in the planner, in get_eclass_for_sort_expr().
So this is likely just a variant of performance issues we've
seen before. (I do wonder why we're not able to prune the
join to just the matching PK partition, though.)
Anyway, the long and the short of it is that this test case is far
larger than anything anyone could practically use in HEAD, let alone
in released branches. I can't get excited about back-patching a fix
to a memory leak if that's just going to allow people to hit other
performance-killing issues.
In short, I don't see a reason why we need this patch in any branch,
so I recommend rejecting it. If we did think we need a leak fix in
the back branches, back-porting 5b9312378 would likely be a saner
way to proceed.
regards, tom lane
