On Thu, Feb 16, 2023 at 01:17:54PM -0800, Andres Freund wrote:
> On 2023-02-16 12:15:12 -0800, Nathan Bossart wrote:
>> On Thu, Feb 16, 2023 at 11:29:56AM -0800, Andres Freund wrote:
>>> Not related the things changed here, but this should never have been pushed
>>> down into individual archive modules. There's absolutely no way that we're
>>> going to keep this up2date and working correctly in random archive
>>> modules. And it would break if archive modules are ever called outside of
>>> pgarch.c.
Hmm, yes. That's a bad idea to copy the error handling stack. The
maintenance of this code could quickly go wrong. All that had better
be put into their own threads, IMO, to bring more visibility on these
subjects.
> 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.
As does this part, probably with a backpatch..
Saying that, I have spent more time on the revamped version of the
archive modules and it was already doing a lot, so I have applied
it as it covered all the points discussed..
--
Michael