Fix resource leak when a FDW's ForeignAsyncRequest function fails

If an error is thrown after calling CreateWaitEventSet(), the memory
of a WaitEventSet is free'd as it's allocated in the short-lived
memory context, but the file descriptor (on epoll- or kqueue-based
systems) or handles (on Windows) that it contains are leaked.

Use PG_TRY-FINALLY to ensure it gets freed. (On master, I will apply a
better fix, using ResourceOwners to track the WaitEventSet, but that's
not backpatchable.)

The added test doesn't check for leaking resources, so it passed even
before this commit. But at least it covers the code path.

In the passing, fix misleading comment on what the 'nevents' argument
to WaitEventSetWait means.

Report by Alexander Lakhin, analysis and suggestion for the fix by Tom
Lane. Fixes bug #17828. Backpatch to v14 where async execution was
introduced, but master gets a different fix.

Discussion: https://www.postgresql.org/message-id/17828-122da8cba23236be@postgresql.org
Discussion: https://www.postgresql.org/message-id/472235.1678387869@sss.pgh.pa.us

diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out
index 93ae45d85ed7bdf047a12d924d980364f29f0065..d21a3d8aaea16af55664aae1b542f9effc1a53d8 100644 (file)
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -10576,6 +10576,13 @@ SELECT * FROM result_tbl ORDER BY a;
 (2 rows)
 
 DELETE FROM result_tbl;
+-- Test error handling, if accessing one of the foreign partitions errors out
+CREATE FOREIGN TABLE async_p_broken PARTITION OF async_pt FOR VALUES FROM (10000) TO (10001)
+  SERVER loopback OPTIONS (table_name 'non_existent_table');
+SELECT * FROM async_pt;
+ERROR:  relation "public.non_existent_table" does not exist
+CONTEXT:  remote SQL command: SELECT a, b, c FROM public.non_existent_table
+DROP FOREIGN TABLE async_p_broken;
 -- Check case where multiple partitions use the same connection
 CREATE TABLE base_tbl3 (a int, b int, c text);
 CREATE FOREIGN TABLE async_p3 PARTITION OF async_pt FOR VALUES FROM (3000) TO (4000)

diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql
index f40972916a9029946a46b1a75f97ac72971d8ad5..80aa7fb56d8a892e92fa0eaff3b53e17f6a955c3 100644 (file)
--- a/contrib/postgres_fdw/sql/postgres_fdw.sql
+++ b/contrib/postgres_fdw/sql/postgres_fdw.sql
@@ -3404,6 +3404,12 @@ INSERT INTO result_tbl SELECT a, b, 'AAA' || c FROM async_pt WHERE b === 505;
 SELECT * FROM result_tbl ORDER BY a;
 DELETE FROM result_tbl;
 
+-- Test error handling, if accessing one of the foreign partitions errors out
+CREATE FOREIGN TABLE async_p_broken PARTITION OF async_pt FOR VALUES FROM (10000) TO (10001)
+  SERVER loopback OPTIONS (table_name 'non_existent_table');
+SELECT * FROM async_pt;
+DROP FOREIGN TABLE async_p_broken;
+
 -- Check case where multiple partitions use the same connection
 CREATE TABLE base_tbl3 (a int, b int, c text);
 CREATE FOREIGN TABLE async_p3 PARTITION OF async_pt FOR VALUES FROM (3000) TO (4000)

diff --git a/src/backend/executor/nodeAppend.c b/src/backend/executor/nodeAppend.c
index 357e10a1d7b78c749dae8cf2895145f9d413ca7b..68f13f2a43f2d11ba582cee09e107dd2800b73e7 100644 (file)
--- a/src/backend/executor/nodeAppend.c
+++ b/src/backend/executor/nodeAppend.c
@@ -1016,43 +1016,51 @@ ExecAppendAsyncEventWait(AppendState *node)
    /* We should never be called when there are no valid async subplans. */
    Assert(node->as_nasyncremain > 0);
 
+   Assert(node->as_eventset == NULL);
    node->as_eventset = CreateWaitEventSet(CurrentMemoryContext, nevents);
-   AddWaitEventToSet(node->as_eventset, WL_EXIT_ON_PM_DEATH, PGINVALID_SOCKET,
-                     NULL, NULL);
-
-   /* Give each waiting subplan a chance to add an event. */
-   i = -1;
-   while ((i = bms_next_member(node->as_asyncplans, i)) >= 0)
+   PG_TRY();
    {
-       AsyncRequest *areq = node->as_asyncrequests[i];
+       AddWaitEventToSet(node->as_eventset, WL_EXIT_ON_PM_DEATH, PGINVALID_SOCKET,
+                         NULL, NULL);
 
-       if (areq->callback_pending)
-           ExecAsyncConfigureWait(areq);
-   }
+       /* Give each waiting subplan a chance to add an event. */
+       i = -1;
+       while ((i = bms_next_member(node->as_asyncplans, i)) >= 0)
+       {
+           AsyncRequest *areq = node->as_asyncrequests[i];
 
-   /*
-    * No need for further processing if there are no configured events other
-    * than the postmaster death event.
-    */
-   if (GetNumRegisteredWaitEvents(node->as_eventset) == 1)
+           if (areq->callback_pending)
+               ExecAsyncConfigureWait(areq);
+       }
+
+       /*
+        * No need for further processing if there are no configured events
+        * other than the postmaster death event.
+        */
+       if (GetNumRegisteredWaitEvents(node->as_eventset) == 1)
+       {
+           FreeWaitEventSet(node->as_eventset);
+           node->as_eventset = NULL;
+           return;
+       }
+
+       /* Return at most EVENT_BUFFER_SIZE events in one call. */
+       if (nevents > EVENT_BUFFER_SIZE)
+           nevents = EVENT_BUFFER_SIZE;
+
+       /*
+        * If the timeout is -1, wait until at least one event occurs.  If the
+        * timeout is 0, poll for events, but do not wait at all.
+        */
+       noccurred = WaitEventSetWait(node->as_eventset, timeout, occurred_event,
+                                    nevents, WAIT_EVENT_APPEND_READY);
+   }
+   PG_FINALLY();
    {
        FreeWaitEventSet(node->as_eventset);
        node->as_eventset = NULL;
-       return;
    }
-
-   /* We wait on at most EVENT_BUFFER_SIZE events. */
-   if (nevents > EVENT_BUFFER_SIZE)
-       nevents = EVENT_BUFFER_SIZE;
-
-   /*
-    * If the timeout is -1, wait until at least one event occurs.  If the
-    * timeout is 0, poll for events, but do not wait at all.
-    */
-   noccurred = WaitEventSetWait(node->as_eventset, timeout, occurred_event,
-                                nevents, WAIT_EVENT_APPEND_READY);
-   FreeWaitEventSet(node->as_eventset);
-   node->as_eventset = NULL;
+   PG_END_TRY();
    if (noccurred == 0)
        return;