From 3400bb826d27b4f7d615e9c4030024dcf4ce842f Mon Sep 17 00:00:00 2001 From: Andrey Borodin Date: Mon, 26 Jan 2026 16:26:22 +0500 Subject: [PATCH v2 2/2] Add sts_delete_files() to avoid unnecessary accessor allocation Introduce a lower-level API for deleting SharedTuplestore files that doesn't require first attaching accessor. This addresses the memory leak in ExecParallelHashDeleteOldPartitions() where sts_attach() was called only to immediately dispose files, leaving the allocated accessor unfreed. The new sts_delete_files() function takes the SharedTuplestore pointer, participant number, and fileset directly, allowing callers to delete files without the overhead of creating accessor. The existing sts_dispose() is refactored to use this new function. Also refactor sts_filename() to take SharedTuplestore* instead of SharedTuplestoreAccessor*, so sts_delete_files() can reuse it without duplicating the filename formatting logic. Suggested-by: Heikki Linnakangas --- src/backend/executor/nodeHash.c | 18 +++++++------- src/backend/utils/sort/sharedtuplestore.c | 29 ++++++++++++++++------- src/include/utils/sharedtuplestore.h | 2 ++ 3 files changed, 32 insertions(+), 17 deletions(-) diff --git a/src/backend/executor/nodeHash.c b/src/backend/executor/nodeHash.c index 65668c7d9ad..23ddc29b154 100644 --- a/src/backend/executor/nodeHash.c +++ b/src/backend/executor/nodeHash.c @@ -1597,16 +1597,16 @@ ExecParallelHashDeleteOldPartitions(HashJoinTable hashtable) for (int i = 1; i < old_nbatch; ++i) { ParallelHashJoinBatch *shared = - NthParallelHashJoinBatch(old_batches, i); - SharedTuplestoreAccessor *accessor; - - accessor = sts_attach(ParallelHashJoinBatchInner(shared), - ParallelWorkerNumber + 1, - &pstate->fileset); - sts_dispose(accessor); - /* XXX free */ - } + NthParallelHashJoinBatch(old_batches, i); + /* + * Delete files for this participant directly without attaching. + * This avoids allocating an accessor just to delete files. + */ + sts_delete_files(ParallelHashJoinBatchInner(shared), + ParallelWorkerNumber + 1, + &pstate->fileset); + } } /* diff --git a/src/backend/utils/sort/sharedtuplestore.c b/src/backend/utils/sort/sharedtuplestore.c index 894139752c6..9603ed6f836 100644 --- a/src/backend/utils/sort/sharedtuplestore.c +++ b/src/backend/utils/sort/sharedtuplestore.c @@ -92,8 +92,7 @@ struct SharedTuplestoreAccessor char *write_end; /* One past the end of the current chunk. */ }; -static void sts_filename(char *name, SharedTuplestoreAccessor *accessor, - int participant); +static void sts_filename(char *name, SharedTuplestore *sts, int participant); /* * Return the amount of shared memory required to hold SharedTuplestore for a @@ -309,7 +308,7 @@ sts_puttuple(SharedTuplestoreAccessor *accessor, void *meta_data, MemoryContext oldcxt; /* Create one. Only this backend will write into it. */ - sts_filename(name, accessor, accessor->participant); + sts_filename(name, accessor->sts, accessor->participant); oldcxt = MemoryContextSwitchTo(accessor->context); accessor->write_file = @@ -531,7 +530,7 @@ sts_parallel_scan_next(SharedTuplestoreAccessor *accessor, void *meta_data) char name[MAXPGPATH]; MemoryContext oldcxt; - sts_filename(name, accessor, accessor->read_participant); + sts_filename(name, accessor->sts, accessor->read_participant); oldcxt = MemoryContextSwitchTo(accessor->context); accessor->read_file = @@ -600,18 +599,32 @@ sts_parallel_scan_next(SharedTuplestoreAccessor *accessor, void *meta_data) */ void sts_dispose(SharedTuplestoreAccessor *accessor) +{ + sts_delete_files(accessor->sts, accessor->participant, accessor->fileset); +} + +/* + * Delete the files associated with a given participant in a SharedTuplestore. + * + * This is a lower-level interface that doesn't require an accessor, useful + * when you need to clean up files without going through sts_attach(). The + * same ownership rules apply: only delete files for your own participant + * number to respect temp_file_limit accounting. + */ +void +sts_delete_files(SharedTuplestore *sts, int participant, SharedFileSet *fileset) { char name[MAXPGPATH]; - sts_filename(name, accessor, accessor->participant); - BufFileDeleteFileSet(&accessor->fileset->fs, name, true); + sts_filename(name, sts, participant); + BufFileDeleteFileSet(&fileset->fs, name, true); } /* * Create the name used for the BufFile that a given participant will write. */ static void -sts_filename(char *name, SharedTuplestoreAccessor *accessor, int participant) +sts_filename(char *name, SharedTuplestore *sts, int participant) { - snprintf(name, MAXPGPATH, "%s.p%d", accessor->sts->name, participant); + snprintf(name, MAXPGPATH, "%s.p%d", sts->name, participant); } diff --git a/src/include/utils/sharedtuplestore.h b/src/include/utils/sharedtuplestore.h index 4ab84e7aa70..d1962b70bba 100644 --- a/src/include/utils/sharedtuplestore.h +++ b/src/include/utils/sharedtuplestore.h @@ -59,5 +59,7 @@ extern MinimalTuple sts_parallel_scan_next(SharedTuplestoreAccessor *accessor, void *meta_data); extern void sts_dispose(SharedTuplestoreAccessor *accessor); +extern void sts_delete_files(SharedTuplestore *sts, int participant, + SharedFileSet *fileset); #endif /* SHAREDTUPLESTORE_H */ -- 2.51.2