Kirill Reshke <reshkekirill@gmail.com> writes:
> I have been looking in coverity that runs against our PostgreSQL
> fork(vanilla + our poor-man patches to be more cloud-native), and
> spotted $subj.
> Unlike many other reports, this looks like a real one to me?
Sadly, Coverity's opinions about backend memory leaks are pretty
nearly complete garbage, because it does not understand about our
memory context architecture. Our expectation is that things like
input functions will be run in a short-lived context, and anything
they leak is to be cleaned up by resetting that context every
so often. If we tried to make the individual input functions
be responsible for that, we'd have leaks in hundreds of places,
many of them a lot messier to fix than record_in(). Also, the
result would likely be slower: we expect that MemoryContextReset
is cheaper than a bunch of retail pfree's.
> Looks like after ccff2d20ed96, when we changed elog to ereport in a
> number of places in this function, we now fail to release memory
> allocated for one row. Now, PostgreSQL has REJECT_LIMIT feature, so
> looks like this leak can aggregate in COPY scenarios
If you trace through that, you will find that record_in is invoked
in the ExprContext that's managed by CopyFrom(), which is reset
for each tuple (see copyfrom.c, about line 1123 as of HEAD).
If there were a leak here, I would blame the COPY logic not
record_in().
> WDYT?
If I were to do anything at all to that code, it'd be to remove
the retail pfree's in the non-error path. They're misleading, and
per the above argument they're likely a net drag on performance
(but probably not enough to measure easily).
regards, tom lane