Location via proxy:   [ UP ]  
[Report a bug]   [Manage cookies]                
Skip to content

Commit 501cfd0

Browse files
committed
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
1 parent 1b6da28 commit 501cfd0

File tree

3 files changed

+50
-29
lines changed

3 files changed

+50
-29
lines changed

contrib/postgres_fdw/expected/postgres_fdw.out

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10825,6 +10825,13 @@ SELECT * FROM result_tbl ORDER BY a;
1082510825
(2 rows)
1082610826

1082710827
DELETE FROM result_tbl;
10828+
-- Test error handling, if accessing one of the foreign partitions errors out
10829+
CREATE FOREIGN TABLE async_p_broken PARTITION OF async_pt FOR VALUES FROM (10000) TO (10001)
10830+
SERVER loopback OPTIONS (table_name 'non_existent_table');
10831+
SELECT * FROM async_pt;
10832+
ERROR: relation "public.non_existent_table" does not exist
10833+
CONTEXT: remote SQL command: SELECT a, b, c FROM public.non_existent_table
10834+
DROP FOREIGN TABLE async_p_broken;
1082810835
-- Check case where multiple partitions use the same connection
1082910836
CREATE TABLE base_tbl3 (a int, b int, c text);
1083010837
CREATE FOREIGN TABLE async_p3 PARTITION OF async_pt FOR VALUES FROM (3000) TO (4000)

contrib/postgres_fdw/sql/postgres_fdw.sql

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3603,6 +3603,12 @@ INSERT INTO result_tbl SELECT a, b, 'AAA' || c FROM async_pt WHERE b === 505;
36033603
SELECT * FROM result_tbl ORDER BY a;
36043604
DELETE FROM result_tbl;
36053605

3606+
-- Test error handling, if accessing one of the foreign partitions errors out
3607+
CREATE FOREIGN TABLE async_p_broken PARTITION OF async_pt FOR VALUES FROM (10000) TO (10001)
3608+
SERVER loopback OPTIONS (table_name 'non_existent_table');
3609+
SELECT * FROM async_pt;
3610+
DROP FOREIGN TABLE async_p_broken;
3611+
36063612
-- Check case where multiple partitions use the same connection
36073613
CREATE TABLE base_tbl3 (a int, b int, c text);
36083614
CREATE FOREIGN TABLE async_p3 PARTITION OF async_pt FOR VALUES FROM (3000) TO (4000)

src/backend/executor/nodeAppend.c

Lines changed: 37 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -1025,43 +1025,51 @@ ExecAppendAsyncEventWait(AppendState *node)
10251025
/* We should never be called when there are no valid async subplans. */
10261026
Assert(node->as_nasyncremain > 0);
10271027

1028+
Assert(node->as_eventset == NULL);
10281029
node->as_eventset = CreateWaitEventSet(CurrentMemoryContext, nevents);
1029-
AddWaitEventToSet(node->as_eventset, WL_EXIT_ON_PM_DEATH, PGINVALID_SOCKET,
1030-
NULL, NULL);
1031-
1032-
/* Give each waiting subplan a chance to add an event. */
1033-
i = -1;
1034-
while ((i = bms_next_member(node->as_asyncplans, i)) >= 0)
1030+
PG_TRY();
10351031
{
1036-
AsyncRequest *areq = node->as_asyncrequests[i];
1032+
AddWaitEventToSet(node->as_eventset, WL_EXIT_ON_PM_DEATH, PGINVALID_SOCKET,
1033+
NULL, NULL);
10371034

1038-
if (areq->callback_pending)
1039-
ExecAsyncConfigureWait(areq);
1040-
}
1035+
/* Give each waiting subplan a chance to add an event. */
1036+
i = -1;
1037+
while ((i = bms_next_member(node->as_asyncplans, i)) >= 0)
1038+
{
1039+
AsyncRequest *areq = node->as_asyncrequests[i];
10411040

1042-
/*
1043-
* No need for further processing if there are no configured events other
1044-
* than the postmaster death event.
1045-
*/
1046-
if (GetNumRegisteredWaitEvents(node->as_eventset) == 1)
1041+
if (areq->callback_pending)
1042+
ExecAsyncConfigureWait(areq);
1043+
}
1044+
1045+
/*
1046+
* No need for further processing if there are no configured events
1047+
* other than the postmaster death event.
1048+
*/
1049+
if (GetNumRegisteredWaitEvents(node->as_eventset) == 1)
1050+
{
1051+
FreeWaitEventSet(node->as_eventset);
1052+
node->as_eventset = NULL;
1053+
return;
1054+
}
1055+
1056+
/* Return at most EVENT_BUFFER_SIZE events in one call. */
1057+
if (nevents > EVENT_BUFFER_SIZE)
1058+
nevents = EVENT_BUFFER_SIZE;
1059+
1060+
/*
1061+
* If the timeout is -1, wait until at least one event occurs. If the
1062+
* timeout is 0, poll for events, but do not wait at all.
1063+
*/
1064+
noccurred = WaitEventSetWait(node->as_eventset, timeout, occurred_event,
1065+
nevents, WAIT_EVENT_APPEND_READY);
1066+
}
1067+
PG_FINALLY();
10471068
{
10481069
FreeWaitEventSet(node->as_eventset);
10491070
node->as_eventset = NULL;
1050-
return;
10511071
}
1052-
1053-
/* We wait on at most EVENT_BUFFER_SIZE events. */
1054-
if (nevents > EVENT_BUFFER_SIZE)
1055-
nevents = EVENT_BUFFER_SIZE;
1056-
1057-
/*
1058-
* If the timeout is -1, wait until at least one event occurs. If the
1059-
* timeout is 0, poll for events, but do not wait at all.
1060-
*/
1061-
noccurred = WaitEventSetWait(node->as_eventset, timeout, occurred_event,
1062-
nevents, WAIT_EVENT_APPEND_READY);
1063-
FreeWaitEventSet(node->as_eventset);
1064-
node->as_eventset = NULL;
1072+
PG_END_TRY();
10651073
if (noccurred == 0)
10661074
return;
10671075

0 commit comments

Comments
 (0)