Re: Why clearing the VM doesn't require registering vm buffer in wal record

On Fri, Jul 10, 2026 at 3:07 PM Melanie Plageman
<melanieplageman(at)gmail(dot)com> wrote:
> _run_pg_combinebackup_with_debug_log's error handling is basically
> copied from system_or_bail -- is that too duplicative? Also, I'm not
> sure if the END code block is done correctly or not.

I don't think that part is too duplicative.

I am not a huge fan of relying on an END block here -- global
destructors can be finnicky to work with, and while passing critical
information around in random global variables is a time-honored
PostgreSQL tradition, it's not a fantastic practice, either.

I think we might want to consider stashing the logs in $self and then
let the caller decide whether to call $self->print_deferred_logs.
Tests that rely on this functionality could choose a place at which to
call $self->print_deferred_logs if !
PostgreSQL::Test::Utils::all_tests_passing(). As compared with that
approach, nothing would be printed if we bail out, unless the caller
arranges to catch the bail out and print the logs before we actually
die, but maybe that's fine.

Or maybe we could simplify this by not even saving all the logs and
just printing the whole log if it fails, else not. Or print the whole
log if it fails and use _diag_command_output() if it succeeds?
(Granted, that's "private" right now, but we could rename it to
diag_command_output and document it.) Or maybe always use
_diag_command_output() and if a future failure needs something from
the middle of the output then we'll deal with that problem then? I'm
not sure what the chances are that we're going to need this output in
the future -- it depends on how many bugs there are in the incremental
backup code, and knowing that number is difficult. I'm hesitant to
just discard it, but overcomplicating the code isn't great, either.

+ my $debug_log = PostgreSQL::Test::Utils::tempdir('pg_combinebackup') .
+ '/debug.log';

I think this is making a separate temporary directory for each
invocation of pg_combinebackup, each containing one file. Perhaps
that's a little more profligate than we need to be.

> Also combinebackup_debug_logs_printed hash set is a little
> over-complicated -- it was added because we could print the same log
> output twice if pg_combinebackup itself failed. Maybe there is a less
> complicated way to handle this? I don't think the END block is
> guaranteed to run depending on how pg_combinebackup fails, so I don't
> know that we can be sure the logs print without doing it right when
> pg_combinebackup fails.

I think the problem here might just that you do push
@combinebackup_debug_logs, $debug_log; before running the command, so
that if the command fails it prints and then the global destructor
sees $debug_log in the list and prints again. You could presumably fix
this by either moving this push down to the end of the function, or by
having _run_pg_combinebackup_with_debug_log not also call
_print_pg_combinebackup_debug_log and rely entirely on END.

--
Robert Haas
EDB: http://www.enterprisedb.com