Re: Trap errors from streaming child in pg_basebackup to exit early

On Fri, Feb 18, 2022 at 10:00:43PM +0100, Daniel Gustafsson wrote:
> This is good idea, I was going in a different direction earlier with a test but
> this is cleaner. The attached 0001 refactors pump_until; 0002 fixes a trivial
> spelling error found while hacking; and 0003 is the previous patch complete
> with a test that passes on Cirrus CI.

This looks rather sane to me, and I can confirm that this passes
the CI and a manual run of MSVC tests with my own box.

+is($node->poll_query_until('postgres',
+ "SELECT pg_terminate_backend(pid) FROM pg_stat_activity WHERE " .
+ "application_name = '010_pg_basebackup.pl' AND wait_event =
'WalSenderMain' " .
+ "AND backend_type = 'walsender'"), "1", "Walsender killed");
If you do that, don't you have a risk to kill the WAL sender doing the
BASE_BACKUP? That could falsify the test. It seems to me that it
would be safer to add a check on query ~ 'START_REPLICATION' or
something like that.

- diag("aborting wait: program timed out");
- diag("stream contents: >>", $$stream, "<<");
- diag("pattern searched for: ", $untl);
Keeping some of this information around would be useful for
debugging in the refactored routine.

+my $sigchld_bb = IPC::Run::start(
+ [
+ @pg_basebackup_defs, '-X', 'stream', '-D', "$tempdir/sigchld",
+ '-r', '32', '-d', $node->connstr('postgres')
+ ],
I would recommend the use of long options here as a matter to
self-document what this does, and add a comment explaining why
--max-rate is preferable, mainly for fast machines.
--
Michael