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 <[email protected]> wrote: > > On Fri, Jul 10, 2026 at 3:07 PM Melanie Plageman > <[email protected]> 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
From 67ff8ceca9a4b49de87bbb9f5e13dabefe0e6253 Mon Sep 17 00:00:00 2001 From: Melanie Plageman <[email protected]> Date: Thu, 30 Apr 2026 16:06:06 -0400 Subject: [PATCH] Put pg_combinebackup debug test output in temp file Having --debug as the default option to pg_combinebackup in init_from_backup() makes the logs extremely verbose. It is used for debugging failures, so put the output in a temp file and only keep it around if tests fail. Author: Melanie Plageman <[email protected]> Reviewed-by: Robert Haas <[email protected]> Disussion: https://postgr.es/m/y73coty2smkxgvelh4e32kqrt5pyn2pb7cz7x4mrz6yaxitqd5%40nzhkeazqf4bn Backpatch-through: 17 --- src/test/perl/PostgreSQL/Test/Cluster.pm | 86 +++++++++++++++++++++++- 1 file changed, 84 insertions(+), 2 deletions(-) diff --git a/src/test/perl/PostgreSQL/Test/Cluster.pm b/src/test/perl/PostgreSQL/Test/Cluster.pm index 529f49efee1..0d4fd929b6b 100644 --- a/src/test/perl/PostgreSQL/Test/Cluster.pm +++ b/src/test/perl/PostgreSQL/Test/Cluster.pm @@ -125,6 +125,7 @@ our $min_compat = 12; # list of file reservations made by get_free_port my @port_reservation_files; +my @combinebackup_debug_logs; # We want to choose a server port above the range that servers typically use # on Unix systems and below the range those systems typically use for ephemeral @@ -924,7 +925,9 @@ pathnames that should be used for the target cluster. To restore from an incremental backup, pass the parameter combine_with_prior as a reference to an array of prior backup names with which this backup -is to be combined using pg_combinebackup. +is to be combined using pg_combinebackup. pg_combinebackup is run with +--debug, but the output is captured in a temporary file and only copied to the +test log if pg_combinebackup fails or if a later test fails. Streaming replication can be enabled on this node by passing the keyword parameter has_streaming => 1. This is disabled by default. @@ -990,7 +993,7 @@ sub init_from_backup } push @combineargs, @prior_backup_path, $backup_path, '--output' => $data_path; - PostgreSQL::Test::Utils::system_or_bail(@combineargs); + $self->_run_pg_combinebackup_with_debug_log(@combineargs); } elsif (defined $params{tar_program}) { @@ -1105,6 +1108,85 @@ port = $port return; } +sub _run_pg_combinebackup_with_debug_log +{ + my ($self, @cmd) = @_; + my $debug_log = $self->basedir . '/pg_combinebackup_debug.log'; + + print("# Running: " . join(" ", @cmd) . "\n"); + # If this pg_combinebackup invocation itself fails, print its captured + # output immediately. Successful invocations may still be printed later if + # some subsequent TAP assertion in this test script fails. + if (!IPC::Run::run(\@cmd, '&>' => $debug_log)) + { + _print_pg_combinebackup_debug_log($debug_log); + + if ($? == -1) + { + BAIL_OUT( + sprintf( + "failed to execute command \"%s\": $!", join(" ", @cmd))); + } + elsif ($? & 127) + { + BAIL_OUT( + sprintf( + "command \"%s\" died with signal %d", + join(" ", @cmd), $? & 127)); + } + else + { + BAIL_OUT( + sprintf( + "command \"%s\" exited with value %d", + join(" ", @cmd), $? >> 8)); + } + } + + # pg_combinebackup succeeded. Retain its debug log so that, if a later TAP + # assertion in this test fails, the END block can print the combine step + # that produced the restored state; on a clean run the END block removes it. + # On failure above we already printed the log and bailed out, so it is + # deliberately not added to the list here (which also avoids printing it + # twice). + push @combinebackup_debug_logs, $debug_log; +} + +sub _print_pg_combinebackup_debug_log +{ + my ($debug_log) = @_; + + return unless -e $debug_log; + + diag("pg_combinebackup debug output from $debug_log:\n" . + PostgreSQL::Test::Utils::slurp_file($debug_log)); +} + +# Keep pg_combinebackup debug logs until test exit. If pg_combinebackup itself +# fails, its log is printed at the failure site above. If pg_combinebackup +# succeeds but a later TAP assertion fails, print the retained logs here so the +# regress output includes the combine step that produced the restored state. +# Remove the logs after a clean run. +END +{ + # take care not to change the script's exit value + my $exit_code = $?; + + if ($exit_code == 0 && PostgreSQL::Test::Utils::all_tests_passing()) + { + unlink @combinebackup_debug_logs; + } + else + { + foreach my $debug_log (@combinebackup_debug_logs) + { + _print_pg_combinebackup_debug_log($debug_log); + } + } + + $? = $exit_code; +} + =pod =item $node->rotate_logfile() -- 2.47.3
