| From: | Kirill Reshke <reshkekirill(at)gmail(dot)com> |
|---|---|
| To: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
| Cc: | pgsql-hackers <pgsql-hackers(at)postgresql(dot)org> |
| Subject: | Re: Cleanup palloc'd structs on soft error path in `record_in` |
| Date: | 2026-01-17 18:21:00 |
| Message-ID: | CALdSSPgb39Wwh8C39hQsXy8Mt7n9Wvzqo9aPukGrZhtvDOeSeQ@mail.gmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
On Sat, 17 Jan 2026 at 23:15, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> Kirill Reshke <reshkekirill(at)gmail(dot)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
| From | Date | Subject | |
|---|---|---|---|
| Previous Message | Tom Lane | 2026-01-17 18:15:07 | Re: Cleanup palloc'd structs on soft error path in `record_in` |