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

Thanks for the review. Reflecting on it more, it probably makes more
sense to commit the VM clear fix and propose pg_combinebackup test
verbosity in a separate thread targeted at master. My current test
version only takes one incremental backup, so it shouldn't be too much
of an issue.

I've addressed your feedback below inline and attached the current
version of the patch you reviewed (with feedback addressed), so that
when I link to this thread in the new thread I plan to make, it all
makes sense.

On Wed, Jul 15, 2026 at 2:27 PM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>
> 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 do now save the logs in $self, but the rest of these thoughts seem
like they would be best discussed in a new thread, so I'll move it
there.

> 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.

Instead, I just avoided creating a temp directory and put the log
files in the node's base directory. It's still cleaned up with the
node, and still kept on failure (basedir honors PG_TEST_NOCLEAN /
test-failure retention).

> > 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.

Got it. For now I moved the push to the end of the function (after the
if (!IPC::Run::run…) block). Then if pg_combinebackup fails, it prints
right away but if it succeeds and a later assertion fails, those print
at the end. I think END won't run if perl crashes -- maybe I don't
need to solve for that, though?

- Melanie