Re: Fixing memory leaks in postgres_fdw

Etsuro Fujita <etsuro(dot)fujita(at)gmail(dot)com> writes:
> On Sat, May 24, 2025 at 10:10 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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