On 2018/10/30 4:48, Tom Lane wrote:
> I was confused about why the memory leak in Bruno's example is so much
> larger in HEAD than v11; spgbeginscan does allocate more stuff than
> before, but only if numberOfOrderBys > 0, which surely doesn't apply for
> the exclusion-check code path. Eventually I realized that the difference
> is because commit 2a6368343 made SpGistScanOpaqueData a good deal larger
> than it had been (10080 vs 6264 bytes, on my x86_64 box), so it was just
> the failure to pfree that struct that accounted for the bigger leak.
>
> There's another issue with 2a6368343, though: it added a couple of
> fmgr_info_copy calls to spgbeginscan. I don't understand why it'd be a
> good idea to do that rather than using the FmgrInfos in the index's
> relcache entry directly. Making a copy every time risks a memory leak,
> because spgendscan has no way to free any fn_extra data that gets
> allocated by the called function; and by the same token it's inefficient,
> because the function has no way to cache fn_extra data across queries.
>
> If we consider only the leak aspect, this coding presents a reason why
> we should try to change things as Alvaro suggests. However, because of
> the poor-caching aspect, I think this code is pretty broken anyway,
> and once we fix it the issue is much less clear-cut.
>
> (Looking around, it seems like we're very schizophrenic about whether
> to copy index relcache support function FmgrInfos or just use them
> directly. But nbtree and hash seem to always do the latter, so I think
> it's probably reasonable to standardize on that.)
Agreed about trying to avoid fmgr_info_copy(), at least in this case.
By the way, do I get it right that the reason to want to avoid copying in
this instance is that the currently active context could be a long-lived
one, as in the case of create index, alter table add constraint, etc.
calling the copying code for every tuple? There are other instances of
fmgr_info_copy throughout index AM implementations, including the helper
function ScanKeyEntryInitializeWithInfo() called from them, but the copies
made in those cases don't appear very leak-prone.
> Anyway, the attached proposed patch for HEAD makes Bruno's problem go
> away, and it also fixes an unrelated leak introduced by 2a6368343
> because it reallocates so->orderByTypes each time through spgrescan.
> I think we should apply this, and suitable subsets in the back branches,
> to fix the immediate problem. Then we can work at leisure on a more
> invasive HEAD-only patch, if anyone is excited about that.
Makes sense to fix it like this for back-patching.
Thanks,
Amit
