From: Tom Lane <tgl@sss.pgh.pa.us>
To: Matheus Alcantara <matheusssilv97@gmail.com>
cc: Etsuro Fujita <etsuro.fujita@gmail.com>,
        pgsql-hackers@lists.postgresql.org
Subject: Re: Fixing memory leaks in postgres_fdw
In-reply-to: <2017375.1748530785@sss.pgh.pa.us>
References: <2976982.1748049023@sss.pgh.pa.us>
 <CAPmGK17iOh6W25UswGcn5k75jEukSgLMpiaJEATcAZQTec7xxg@mail.gmail.com>
 <216286.1748202797@sss.pgh.pa.us> <519144.1748217540@sss.pgh.pa.us>
 <918254.1748288174@sss.pgh.pa.us>
 <CAFY6G8fEa9sZ1V9cJpMOZAp6Wfa35U_rE0Y=eoQLS2G3OAqiAw@mail.gmail.com>
 <1346940.1748381911@sss.pgh.pa.us> <1696019.1748465792@sss.pgh.pa.us>
 <CAFY6G8fY=e8n1K=sHuXovgxEgRqN1Df5db230uF1KtffvWUT-Q@mail.gmail.com>
 <1876117.1748524672@sss.pgh.pa.us> <2017375.1748530785@sss.pgh.pa.us>
Comments: In-reply-to Tom Lane <tgl@sss.pgh.pa.us>
	message dated "Thu, 29 May 2025 10:59:45 -0400"
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0"
Content-ID: <2082474.1748538063.0@sss.pgh.pa.us>
Date: Thu, 29 May 2025 13:02:39 -0400
Message-ID: <2082618.1748538159@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/2082618.1748538159%40sss.pgh.pa.us>
Precedence: bulk

------- =_aaaaaaaaaa0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <2082474.1748538063.1@sss.pgh.pa.us>

I wrote:
> Pushed v5-0001, and here are rebased versions of the other four
> patches, mostly so that the cfbot knows what is the patch-of-record.

Finally, here's a minimalistic version of the original v1-0001
patch that I think we could safely apply to fix the DirectModify
problem in the back branches.  I rejiggered it to not depend on
inventing MemoryContextUnregisterResetCallback, so that there's
not hazards of minor-version skew between postgres_fdw and the
main backend.  This will of course not fix any other PGresult-leakage
cases that may exist, but I'm content to fix the known problem
in back branches.

(Patch is labeled .txt so that cfbot doesn't think it's the
patch-of-record.)

			regards, tom lane


------- =_aaaaaaaaaa0
Content-Type: text/x-diff; name="v7-backpatchable-fix.patch.txt";
	charset="us-ascii"
Content-ID: <2082474.1748538063.2@sss.pgh.pa.us>
Content-Description: v7-backpatchable-fix.patch.txt
Content-Transfer-Encoding: quoted-printable

diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/po=
stgres_fdw.c
index 331f3fc088d..4283ce9f962 100644
--- a/contrib/postgres_fdw/postgres_fdw.c
+++ b/contrib/postgres_fdw/postgres_fdw.c
@@ -240,6 +240,7 @@ typedef struct PgFdwDirectModifyState
 	PGresult   *result;			/* result for query */
 	int			num_tuples;		/* # of result tuples */
 	int			next_tuple;		/* index of next one to return */
+	MemoryContextCallback result_cb;	/* ensures result will get freed */
 	Relation	resultRel;		/* relcache entry for the target relation */
 	AttrNumber *attnoMap;		/* array of attnums of input user columns */
 	AttrNumber	ctidAttno;		/* attnum of input ctid column */
@@ -2670,6 +2671,17 @@ postgresBeginDirectModify(ForeignScanState *node, i=
nt eflags)
 	dmstate =3D (PgFdwDirectModifyState *) palloc0(sizeof(PgFdwDirectModifyS=
tate));
 	node->fdw_state =3D dmstate;
 =

+	/*
+	 * We use a memory context callback to ensure that the dmstate's PGresul=
t
+	 * (if any) will be released, even if the query fails somewhere that's
+	 * outside our control.  The callback is always armed for the duration o=
f
+	 * the query; this relies on PQclear(NULL) being a no-op.
+	 */
+	dmstate->result_cb.func =3D (MemoryContextCallbackFunction) PQclear;
+	dmstate->result_cb.arg =3D NULL;
+	MemoryContextRegisterResetCallback(CurrentMemoryContext,
+									   &dmstate->result_cb);
+
 	/*
 	 * Identify which user to do the remote access as.  This should match wh=
at
 	 * ExecCheckPermissions() does.
@@ -2817,7 +2829,13 @@ postgresEndDirectModify(ForeignScanState *node)
 		return;
 =

 	/* Release PGresult */
-	PQclear(dmstate->result);
+	if (dmstate->result)
+	{
+		PQclear(dmstate->result);
+		dmstate->result =3D NULL;
+		/* ... and don't forget to disable the callback */
+		dmstate->result_cb.arg =3D NULL;
+	}
 =

 	/* Release remote connection */
 	ReleaseConnection(dmstate->conn);
@@ -4591,13 +4609,17 @@ execute_dml_stmt(ForeignScanState *node)
 	/*
 	 * Get the result, and check for success.
 	 *
-	 * We don't use a PG_TRY block here, so be careful not to throw error
-	 * without releasing the PGresult.
+	 * We use a memory context callback to ensure that the PGresult will be
+	 * released, even if the query fails somewhere that's outside our contro=
l.
+	 * The callback is already registered, just need to fill in its arg.
 	 */
+	Assert(dmstate->result =3D=3D NULL);
 	dmstate->result =3D pgfdw_get_result(dmstate->conn);
+	dmstate->result_cb.arg =3D dmstate->result;
+
 	if (PQresultStatus(dmstate->result) !=3D
 		(dmstate->has_returning ? PGRES_TUPLES_OK : PGRES_COMMAND_OK))
-		pgfdw_report_error(ERROR, dmstate->result, dmstate->conn, true,
+		pgfdw_report_error(ERROR, dmstate->result, dmstate->conn, false,
 						   dmstate->query);
 =

 	/* Get the number of rows affected. */
@@ -4641,30 +4663,16 @@ get_returning_data(ForeignScanState *node)
 	}
 	else
 	{
-		/*
-		 * On error, be sure to release the PGresult on the way out.  Callers
-		 * do not have PG_TRY blocks to ensure this happens.
-		 */
-		PG_TRY();
-		{
-			HeapTuple	newtup;
-
-			newtup =3D make_tuple_from_result_row(dmstate->result,
-												dmstate->next_tuple,
-												dmstate->rel,
-												dmstate->attinmeta,
-												dmstate->retrieved_attrs,
-												node,
-												dmstate->temp_cxt);
-			ExecStoreHeapTuple(newtup, slot, false);
-		}
-		PG_CATCH();
-		{
-			PQclear(dmstate->result);
-			PG_RE_THROW();
-		}
-		PG_END_TRY();
+		HeapTuple	newtup;
 =

+		newtup =3D make_tuple_from_result_row(dmstate->result,
+											dmstate->next_tuple,
+											dmstate->rel,
+											dmstate->attinmeta,
+											dmstate->retrieved_attrs,
+											node,
+											dmstate->temp_cxt);
+		ExecStoreHeapTuple(newtup, slot, false);
 		/* Get the updated/deleted tuple. */
 		if (dmstate->rel)
 			resultSlot =3D slot;

------- =_aaaaaaaaaa0--