Horton

Re: Fixing memory leaks in postgres_fdw

Tom Lane <tgl@sss.pgh.pa.us>

View thread View on postgr.es
From: Tom Lane <tgl@sss.pgh.pa.us>
To: Etsuro Fujita <etsuro.fujita@gmail.com>
Cc: pgsql-hackers@lists.postgresql.org
Date: 2025-05-25T19:53:17Z
Lists: pgsql-hackers

Attachments

Etsuro Fujita <etsuro.fujita@gmail.com> writes:
> On Sat, May 24, 2025 at 10:10 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I thought of fixing this by using a memory context reset callback
>> to ensure that the PGresult is cleaned up when the executor's context
>> goes away, and that seems to work nicely (see 0001 attached).
>> However, I feel like this is just a POC, because now that we have that
>> concept we might be able to use it elsewhere in postgres_fdw to
>> eliminate most or even all of its reliance on PG_TRY.  That should be
>> faster as well as much less bug-prone.  But I'm putting it up at this
>> stage for comments, in case anyone thinks this is not the direction to
>> head in.

> I think that that is a good idea; +1 for removing the reliance not
> only in DirectModify but in other places.  I think that that would be
> also useful if extending batch INSERT to cases with RETURNING data in
> postgres_fdw.

Here is an attempt at making a bulletproof fix by having all backend
users of libpq go through a wrapper layer that provides the memory
context callback.  Perhaps this is more code churn than we want to
accept; I'm not sure.  I thought about avoiding most of the niggling
code changes by adding

#define PGresult BEPGresult
#define PQclear BEPQclear
#define PQresultStatus BEPQresultStatus

and so forth at the bottom of the new header file, but I'm afraid
that would create a lot of confusion.

There is a lot yet to do towards getting rid of no-longer-needed
PG_TRYs and other complication, but I decided to stop here pending
comments on the notational decisions I made.

One point that people might find particularly dubious is that
I put the new stuff into a new header file libpq-be-fe.h, rather
than adding it to libpq-be-fe-helpers.h which would seem more
obvious.  The reason for that is the code layout in postgres_fdw.
postgres_fdw.h needs to include libpq-fe.h to get the PGresult
typedef, and with these changes it instead needs to get BEPGresult.
But only connection.c currently includes libpq-be-fe-helpers.h,
and I didn't like the idea of making all of postgres_fdw's .c
files include that.  Maybe that's not worth worrying about though.

The 0002 patch is the same as before.

			regards, tom lane

Commits

  1. Silence leakage complaint about postgres_fdw's InitPgFdwOptions.

  2. Run pgindent on the changes of the previous patch.

  3. Reap the benefits of not having to avoid leaking PGresults.

  4. Create infrastructure to reliably prevent leakage of PGresults.

  5. Fix memory leakage in postgres_fdw's DirectModify code path.

  6. Avoid resource leaks when a dblink connection fails.