Re: [PATCH] Free BufFile metadata in close and append paths

On Thu, Apr 30, 2026 at 12:46:59AM +0900, DaeMyung Kang wrote:
> 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);
> }

Yeah, I can get behind that. We care about resources in this specific
call.

> @@ -907,6 +909,7 @@ BufFileAppend(BufFile *target, BufFile *source)
>
> Assert(source->readOnly);
> Assert(!source->dirty);
> + Assert(target != source);

The addition of this assertion looks sensible.

> + /*
> + * 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;

However I cannot really get behind the pfree() calls you are adding
here. What if the caller cares about keeping a track of the source
data? Your assumptions are based on the sole caller of
BufFileAppend() in the tree. There could be callers outside the core
tree, in extension code.

None of this is material for v19. Could you add an entry in the
upcoming commit fest at [1]? You can add me as reviewer and/or
committer, so as I don't forget about it. I am writing a note down,
to not forget, but notebooks are not perfect.

[1]: https://commitfest.postgresql.org/59/
--
Michael