Hi, Hackers, Continuing the storage/file audit, I found two small but consistent metadata leaks in BufFile. BufFileClose() does not release the pstrdup'd name on FileSet-based BufFiles, and BufFileAppend() silently retains the source wrapper (including its BLCKSZ buffer) even though the function semantics already say the source must not be touched after the call.
Details and the parallel-sort motivation are in the commit message. Thanks, DaeMyung --- FileSet-based BufFiles duplicate their logical name, but BufFileClose() did not release it. Release that metadata along with the other palloc'd state owned by the BufFile. BufFileAppend() transfers only the underlying file handles to the target while telling callers not to use the source again. Free the consumed source wrapper and metadata after the transfer so repeated appends do not retain a BLCKSZ-sized BufFile wrapper until context reset. This matters most for parallel sort workflows that issue many BufFileAppend() calls in a single context: each retained source wrapper holds a BLCKSZ-sized buffer until context reset, which can run into hundreds of KB at high DOP. --- src/backend/storage/file/buffile.c | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-)
diff --git a/src/backend/storage/file/buffile.c b/src/backend/storage/file/buffile.c index c4afe4d368a..1d1efcd139b 100644 --- a/src/backend/storage/file/buffile.c +++ b/src/backend/storage/file/buffile.c @@ -419,8 +419,10 @@ BufFileClose(BufFile *file) /* close and delete the underlying file(s) */ for (i = 0; i < file->numFiles; i++) FileClose(file->files[i]); - /* release the buffer space */ + /* release the buffer space and other metadata */ pfree(file->files); + if (file->name) + pfree((void *) file->name); pfree(file); } @@ -883,10 +885,10 @@ BufFileSize(BufFile *file) /* * Append the contents of the source file to the end of the target file. * - * Note that operation subsumes ownership of underlying resources from - * "source". Caller should never call BufFileClose against source having - * called here first. Resource owners for source and target must match, - * too. + * Note that this operation subsumes ownership of underlying resources from + * "source", and frees the source wrapper and metadata before returning. + * Callers must not reference source after this call. Resource owners for + * source and target must match, too. * * This operation works by manipulating lists of segment files, so the * file content is always appended at a MAX_PHYSICAL_FILESIZE-aligned @@ -907,6 +909,7 @@ BufFileAppend(BufFile *target, BufFile *source) Assert(source->readOnly); Assert(!source->dirty); + Assert(target != source); if (target->resowner != source->resowner) elog(ERROR, "could not append BufFile with non-matching resource owner"); @@ -917,6 +920,15 @@ BufFileAppend(BufFile *target, BufFile *source) target->files[i] = source->files[i - target->numFiles]; target->numFiles = newNumFiles; + /* + * The underlying files now belong to target. Free only source's wrapper + * and metadata, leaving the transferred file handles open. + */ + if (source->name) + pfree((void *) source->name); + pfree(source->files); + pfree(source); + return startBlock; }
