Hi,
On 2023-02-16 13:58:10 -0800, Nathan Bossart wrote:
> On Thu, Feb 16, 2023 at 01:17:54PM -0800, Andres Freund wrote:
> > I'm quite baffled by:
> > /* Close any files left open by copy_file() or compare_files() */
> > AtEOSubXact_Files(false, InvalidSubTransactionId, InvalidSubTransactionId);
> >
> > in basic_archive_file(). It seems *really* off to call AtEOSubXact_Files()
> > completely outside the context of a transaction environment. And it only does
> > the thing you want because you pass parameters that aren't actually valid in
> > the normal use in AtEOSubXact_Files(). I really don't understand how that's
> > supposed to be ok.
>
> Hm. Should copy_file() and compare_files() have PG_FINALLY blocks that
> attempt to close the files instead? What would you recommend?
I don't fully now, it's not entirely clear to me what the goals here were. I
think you'd likely need to do a bit of infrastructure work to do this
sanely. So far we just didn't have the need to handle files being released in
a way like you want to do there.
I suspect a good direction would be to use resource owners. Add a separate set
of functions that release files on resource owner release. Most of the
infrastructure is there already, for temporary files
(c.f. OpenTemporaryFile()).
Then that resource owner could be reset in case of error.
I'm not even sure that erroring out is a reasonable way to implement
copy_file(), compare_files(), particularly because you want to return via a
return code from basic_archive_files().
Greetings,
Andres Freund
