Обсуждение: Cleanup palloc'd structs on soft error path in `record_in`

Hi hackers!

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?

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

WDYT?

PFA trivial fix.


-- 
Best regards,
Kirill Reshke

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

On Sat, 17 Jan 2026 at 23:15, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> 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


Thank you for your clarification.

Indeed, if `record_in` leaks, we need to blame the caller function for
not doing MemoryContextReset.

Yes, almost all coverity reports about memory leaks are garbage.

-- 
Best regards,
Kirill Reshke