projects / postgresql.git / commitdiff
summary | shortlog | log | commit | commitdiff | tree
raw | patch | inline | side by side (parent: d98cefe)
Fix memory leakage in postgres_fdw's DirectModify code path.
authorTom Lane <tgl@sss.pgh.pa.us>
Fri, 30 May 2025 17:45:41 +0000 (13:45 -0400)
committerTom Lane <tgl@sss.pgh.pa.us>
Fri, 30 May 2025 17:45:41 +0000 (13:45 -0400)
postgres_fdw tries to use PG_TRY blocks to ensure that it will
eventually free the PGresult created by the remote modify command.
However, it's fundamentally impossible for this scheme to work
reliably when there's RETURNING data, because the query could fail
in between invocations of postgres_fdw's DirectModify methods.
There is at least one instance of exactly this situation in the
regression tests, and the ensuing session-lifespan leak is visible
under Valgrind.

We can improve matters by using a memory context reset callback
attached to the ExecutorState context.  That ensures that the
PGresult will be freed when the ExecutorState context is torn
down, even if control never reaches postgresEndDirectModify.

I have little faith that there aren't other potential PGresult
leakages in the backend modules that use libpq.  So I think it'd
be a good idea to apply this concept universally by creating
infrastructure that attaches a reset callback to every PGresult
generated in the backend.  However, that seems too invasive for
v18 at this point, let alone the back branches.  So for the
moment, apply this narrow fix that just makes DirectModify safe.
I have a patch in the queue for the more general idea, but it
will have to wait for v19.

Author: Tom Lane <tgl@sss.pgh.pa.us>
Reviewed-by: Matheus Alcantara <matheusssilv97@gmail.com>
Discussion: https://postgr.es/m/2976982.1748049023@sss.pgh.pa.us
Backpatch-through: 13

contrib/postgres_fdw/postgres_fdw.c patch | blob | blame | history

diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c
index 331f3fc088d1bc582bbab3664915768b3e173e70..4283ce9f96252e521687fab1228da2ea57bf36bb 100644 (file)
--- 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, int eflags)
    dmstate = (PgFdwDirectModifyState *) palloc0(sizeof(PgFdwDirectModifyState));
    node->fdw_state = dmstate;
 
+   /*
+    * We use a memory context callback to ensure that the dmstate's PGresult
+    * (if any) will be released, even if the query fails somewhere that's
+    * outside our control.  The callback is always armed for the duration of
+    * the query; this relies on PQclear(NULL) being a no-op.
+    */
+   dmstate->result_cb.func = (MemoryContextCallbackFunction) PQclear;
+   dmstate->result_cb.arg = NULL;
+   MemoryContextRegisterResetCallback(CurrentMemoryContext,
+                                      &dmstate->result_cb);
+
    /*
     * Identify which user to do the remote access as.  This should match what
     * ExecCheckPermissions() does.
@@ -2817,7 +2829,13 @@ postgresEndDirectModify(ForeignScanState *node)
        return;
 
    /* Release PGresult */
-   PQclear(dmstate->result);
+   if (dmstate->result)
+   {
+       PQclear(dmstate->result);
+       dmstate->result = NULL;
+       /* ... and don't forget to disable the callback */
+       dmstate->result_cb.arg = 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 control.
+    * The callback is already registered, just need to fill in its arg.
     */
+   Assert(dmstate->result == NULL);
    dmstate->result = pgfdw_get_result(dmstate->conn);
+   dmstate->result_cb.arg = dmstate->result;
+
    if (PQresultStatus(dmstate->result) !=
        (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 = 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 = 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 = slot;
This is the main PostgreSQL git repository.