Skip to content

Commit 50c67c2

Browse files
hlinnakahlinnaka
committed
Use ResourceOwner to track WaitEventSets.
A WaitEventSet holds file descriptors or event handles (on Windows). If FreeWaitEventSet is not called, those fds or handles are leaked. Use ResourceOwners to track WaitEventSets, to clean those up automatically on error. This was a live bug in async Append nodes, if a FDW's ForeignAsyncRequest function failed. (In back branches, I will apply a more localized fix for that based on PG_TRY-PG_FINALLY.) 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. Reviewed-by: Alexander Lakhin, Thomas Munro Discussion: https://www.postgresql.org/message-id/472235.1678387869@sss.pgh.pa.us
1 parent 414e755 commit 50c67c2

File tree

9 files changed

+84
-13
lines changed

9 files changed

+84
-13
lines changed

‎contrib/postgres_fdw/expected/postgres_fdw.out

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

1081110811
DELETE FROM result_tbl;
10812+
-- Test error handling, if accessing one of the foreign partitions errors out
10813+
CREATE FOREIGN TABLE async_p_broken PARTITION OF async_pt FOR VALUES FROM (10000) TO (10001)
10814+
SERVER loopback OPTIONS (table_name 'non_existent_table');
10815+
SELECT * FROM async_pt;
10816+
ERROR: relation "public.non_existent_table" does not exist
10817+
CONTEXT: remote SQL command: SELECT a, b, c FROM public.non_existent_table
10818+
DROP FOREIGN TABLE async_p_broken;
1081210819
-- Check case where multiple partitions use the same connection
1081310820
CREATE TABLE base_tbl3 (a int, b int, c text);
1081410821
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
@@ -3607,6 +3607,12 @@ INSERT INTO result_tbl SELECT a, b, 'AAA' || c FROM async_pt WHERE b === 505;
36073607
SELECT * FROM result_tbl ORDER BY a;
36083608
DELETE FROM result_tbl;
36093609

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

‎src/backend/executor/nodeAppend.c

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

1028-
node->as_eventset = CreateWaitEventSet(CurrentMemoryContext, nevents);
1028+
Assert(node->as_eventset == NULL);
1029+
node->as_eventset = CreateWaitEventSet(CurrentResourceOwner, nevents);
10291030
AddWaitEventToSet(node->as_eventset, WL_EXIT_ON_PM_DEATH, PGINVALID_SOCKET,
10301031
NULL, NULL);
10311032

@@ -1050,7 +1051,7 @@ ExecAppendAsyncEventWait(AppendState *node)
10501051
return;
10511052
}
10521053

1053-
/* We wait on at most EVENT_BUFFER_SIZE events. */
1054+
/* Return at most EVENT_BUFFER_SIZE events in one call. */
10541055
if (nevents > EVENT_BUFFER_SIZE)
10551056
nevents = EVENT_BUFFER_SIZE;
10561057

‎src/backend/libpq/pqcomm.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -207,7 +207,7 @@ pq_init(void)
207207
elog(FATAL, "fcntl(F_SETFD) failed on socket: %m");
208208
#endif
209209

210-
FeBeWaitSet = CreateWaitEventSet(TopMemoryContext, FeBeWaitSetNEvents);
210+
FeBeWaitSet = CreateWaitEventSet(NULL, FeBeWaitSetNEvents);
211211
socket_pos = AddWaitEventToSet(FeBeWaitSet, WL_SOCKET_WRITEABLE,
212212
MyProcPort->sock, NULL, NULL);
213213
latch_pos = AddWaitEventToSet(FeBeWaitSet, WL_LATCH_SET, PGINVALID_SOCKET,

‎src/backend/postmaster/postmaster.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1695,7 +1695,7 @@ ConfigurePostmasterWaitSet(bool accept_connections)
16951695
FreeWaitEventSet(pm_wait_set);
16961696
pm_wait_set = NULL;
16971697

1698-
pm_wait_set = CreateWaitEventSet(CurrentMemoryContext,
1698+
pm_wait_set = CreateWaitEventSet(NULL,
16991699
accept_connections ? (1 + NumListenSockets) : 1);
17001700
AddWaitEventToSet(pm_wait_set, WL_LATCH_SET, PGINVALID_SOCKET, MyLatch,
17011701
NULL);

‎src/backend/postmaster/syslogger.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -311,7 +311,7 @@ SysLoggerMain(int argc, char *argv[])
311311
* syslog pipe, which implies that all other backends have exited
312312
* (including the postmaster).
313313
*/
314-
wes = CreateWaitEventSet(CurrentMemoryContext, 2);
314+
wes = CreateWaitEventSet(NULL, 2);
315315
AddWaitEventToSet(wes, WL_LATCH_SET, PGINVALID_SOCKET, MyLatch, NULL);
316316
#ifndef WIN32
317317
AddWaitEventToSet(wes, WL_SOCKET_READABLE, syslogPipe[0], NULL, NULL);

‎src/backend/storage/ipc/latch.c

Lines changed: 61 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -62,6 +62,7 @@
6262
#include "storage/pmsignal.h"
6363
#include "storage/shmem.h"
6464
#include "utils/memutils.h"
65+
#include "utils/resowner.h"
6566

6667
/*
6768
* Select the fd readiness primitive to use. Normally the "most modern"
@@ -101,6 +102,8 @@
101102
/* typedef in latch.h */
102103
struct WaitEventSet
103104
{
105+
ResourceOwner owner;
106+
104107
int nevents; /* number of registered events */
105108
int nevents_space; /* maximum number of events in this set */
106109

@@ -195,6 +198,31 @@ static void WaitEventAdjustWin32(WaitEventSet *set, WaitEvent *event);
195198
static inline int WaitEventSetWaitBlock(WaitEventSet *set, int cur_timeout,
196199
WaitEvent *occurred_events, int nevents);
197200

201+
/* ResourceOwner support to hold WaitEventSets */
202+
static void ResOwnerReleaseWaitEventSet(Datum res);
203+
204+
static const ResourceOwnerDesc wait_event_set_resowner_desc =
205+
{
206+
.name = "WaitEventSet",
207+
.release_phase = RESOURCE_RELEASE_AFTER_LOCKS,
208+
.release_priority = RELEASE_PRIO_WAITEVENTSETS,
209+
.ReleaseResource = ResOwnerReleaseWaitEventSet,
210+
.DebugPrint = NULL
211+
};
212+
213+
/* Convenience wrappers over ResourceOwnerRemember/Forget */
214+
static inline void
215+
ResourceOwnerRememberWaitEventSet(ResourceOwner owner, WaitEventSet *set)
216+
{
217+
ResourceOwnerRemember(owner, PointerGetDatum(set), &wait_event_set_resowner_desc);
218+
}
219+
static inline void
220+
ResourceOwnerForgetWaitEventSet(ResourceOwner owner, WaitEventSet *set)
221+
{
222+
ResourceOwnerForget(owner, PointerGetDatum(set), &wait_event_set_resowner_desc);
223+
}
224+
225+
198226
/*
199227
* Initialize the process-local latch infrastructure.
200228
*
@@ -323,7 +351,7 @@ InitializeLatchWaitSet(void)
323351
Assert(LatchWaitSet == NULL);
324352

325353
/* Set up the WaitEventSet used by WaitLatch(). */
326-
LatchWaitSet = CreateWaitEventSet(TopMemoryContext, 2);
354+
LatchWaitSet = CreateWaitEventSet(NULL, 2);
327355
latch_pos = AddWaitEventToSet(LatchWaitSet, WL_LATCH_SET, PGINVALID_SOCKET,
328356
MyLatch, NULL);
329357
if (IsUnderPostmaster)
@@ -541,7 +569,7 @@ WaitLatchOrSocket(Latch *latch, int wakeEvents, pgsocket sock,
541569
int ret = 0;
542570
int rc;
543571
WaitEvent event;
544-
WaitEventSet *set = CreateWaitEventSet(CurrentMemoryContext, 3);
572+
WaitEventSet *set = CreateWaitEventSet(CurrentResourceOwner, 3);
545573

546574
if (wakeEvents & WL_TIMEOUT)
547575
Assert(timeout >= 0);
@@ -716,9 +744,12 @@ ResetLatch(Latch *latch)
716744
*
717745
* These events can then be efficiently waited upon together, using
718746
* WaitEventSetWait().
747+
*
748+
* The WaitEventSet is tracked by the given 'resowner'. Use NULL for session
749+
* lifetime.
719750
*/
720751
WaitEventSet *
721-
CreateWaitEventSet(MemoryContext context, int nevents)
752+
CreateWaitEventSet(ResourceOwner resowner, int nevents)
722753
{
723754
WaitEventSet *set;
724755
char *data;
@@ -744,7 +775,10 @@ CreateWaitEventSet(MemoryContext context, int nevents)
744775
sz += MAXALIGN(sizeof(HANDLE) * (nevents + 1));
745776
#endif
746777

747-
data = (char *) MemoryContextAllocZero(context, sz);
778+
if (resowner != NULL)
779+
ResourceOwnerEnlarge(resowner);
780+
781+
data = (char *) MemoryContextAllocZero(TopMemoryContext, sz);
748782

749783
set = (WaitEventSet *) data;
750784
data += MAXALIGN(sizeof(WaitEventSet));
@@ -770,6 +804,12 @@ CreateWaitEventSet(MemoryContext context, int nevents)
770804
set->nevents_space = nevents;
771805
set->exit_on_postmaster_death = false;
772806

807+
if (resowner != NULL)
808+
{
809+
ResourceOwnerRememberWaitEventSet(resowner, set);
810+
set->owner = resowner;
811+
}
812+
773813
#if defined(WAIT_USE_EPOLL)
774814
if (!AcquireExternalFD())
775815
{
@@ -834,16 +874,20 @@ CreateWaitEventSet(MemoryContext context, int nevents)
834874
void
835875
FreeWaitEventSet(WaitEventSet *set)
836876
{
877+
if (set->owner)
878+
{
879+
ResourceOwnerForgetWaitEventSet(set->owner, set);
880+
set->owner = NULL;
881+
}
882+
837883
#if defined(WAIT_USE_EPOLL)
838884
close(set->epoll_fd);
839885
ReleaseExternalFD();
840886
#elif defined(WAIT_USE_KQUEUE)
841887
close(set->kqueue_fd);
842888
ReleaseExternalFD();
843889
#elif defined(WAIT_USE_WIN32)
844-
WaitEvent *cur_event;
845-
846-
for (cur_event = set->events;
890+
for (WaitEvent *cur_event = set->events;
847891
cur_event < (set->events + set->nevents);
848892
cur_event++)
849893
{
@@ -2300,3 +2344,13 @@ drain(void)
23002344
}
23012345

23022346
#endif
2347+
2348+
static void
2349+
ResOwnerReleaseWaitEventSet(Datum res)
2350+
{
2351+
WaitEventSet *set = (WaitEventSet *) DatumGetPointer(res);
2352+
2353+
Assert(set->owner != NULL);
2354+
set->owner = NULL;
2355+
FreeWaitEventSet(set);
2356+
}

‎src/include/storage/latch.h

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -102,6 +102,8 @@
102102

103103
#include <signal.h>
104104

105+
#include "utils/resowner.h"
106+
105107
/*
106108
* Latch structure should be treated as opaque and only accessed through
107109
* the public functions. It is defined here to allow embedding Latches as
@@ -173,7 +175,7 @@ extern void SetLatch(Latch *latch);
173175
extern void ResetLatch(Latch *latch);
174176
extern void ShutdownLatchSupport(void);
175177

176-
extern WaitEventSet *CreateWaitEventSet(MemoryContext context, int nevents);
178+
extern WaitEventSet *CreateWaitEventSet(ResourceOwner resowner, int nevents);
177179
extern void FreeWaitEventSet(WaitEventSet *set);
178180
extern void FreeWaitEventSetAfterFork(WaitEventSet *set);
179181
extern int AddWaitEventToSet(WaitEventSet *set, uint32 events, pgsocket fd,

‎src/include/utils/resowner.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -74,6 +74,7 @@ typedef uint32 ResourceReleasePriority;
7474
#define RELEASE_PRIO_TUPDESC_REFS 400
7575
#define RELEASE_PRIO_SNAPSHOT_REFS 500
7676
#define RELEASE_PRIO_FILES 600
77+
#define RELEASE_PRIO_WAITEVENTSETS 700
7778

7879
/* 0 is considered invalid */
7980
#define RELEASE_PRIO_FIRST 1

0 commit comments

Comments
 (0)