Making background psql nicer to use in tap tests
Andres Freund <andres@anarazel.de>
From: Andres Freund <andres@anarazel.de>
To: pgsql-hackers@postgresql.org
Cc: Melanie Plageman <melanieplageman@gmail.com>
Date: 2023-01-30T19:43:50Z
Lists: pgsql-hackers
Hi,
Plenty tap tests require a background psql. But they're pretty annoying to
use.
I think the biggest improvement would be an easy way to run a single query and
get the result of that query. Manually having to pump_until() is awkward and
often leads to hangs/timeouts, instead of test failures, because one needs to
specify a match pattern to pump_until(), which on mismatch leads to trying to
keep pumping forever.
It's annoyingly hard to wait for the result of a query in a generic way with
background_psql(), and more generally for psql. background_psql() uses -XAtq,
which means that we'll not get "status" output (like "BEGIN" or "(1 row)"),
and that queries not returning anything are completely invisible.
A second annoyance is that issuing a query requires a trailing newline,
otherwise psql won't process it.
The best way I can see is to have a helper that issues the query, followed by
a trailing newline, an \echo with a recognizable separator, and then uses
pump_until() to wait for that separator.
Another area worthy of improvement is that background_psql() requires passing
in many variables externally - without a recognizable benefit afaict. What's
the point in 'stdin', 'stdout', 'timer' being passed in? stdin/stdout need to
point to empty strings, so we know what's needed - in fact we'll even reset
them if they're passed in. The timer is always going to be
PostgreSQL::Test::Utils::timeout_default, so again, what's the point?
I think it'd be far more usable if we made background_psql() return a hash
with the relevant variables. The 031_recovery_conflict.pl test has:
my $psql_timeout = IPC::Run::timer($PostgreSQL::Test::Utils::timeout_default);
my %psql_standby = ('stdin' => '', 'stdout' => '');
$psql_standby{run} =
$node_standby->background_psql($test_db, \$psql_standby{stdin},
\$psql_standby{stdout},
$psql_timeout);
$psql_standby{stdout} = '';
How about just returning a reference to a hash like that? Except that I'd also
make stderr available, which one can't currently access.
The $psql_standby{stdout} = ''; is needed because background_psql() leaves a
banner in the output, which it shouldn't, but we probably should just fix
that.
Brought to you by: Trying to write a test for vacuum_defer_cleanup_age.
- Andres
Commits
-
Fix missing installation/uninstallation rules for BackgroundPsql.pm
- 241ef53b6a95 12.20 landed
- 0d80e59a1b2c 13.16 landed
- 1185be355462 14.13 landed
- 9d7506a2f7cb 15.8 landed
-
Backport BackgroundPsql perl test module
- 4b467a6581af 12.20 landed
- 12f327b210b3 13.16 landed
- 31877cd8ec8c 14.13 landed
- d5fd7865f068 15.8 landed
- 187b8991f70f 16.4 landed
-
Add missing uninstallation rule for BackgroundPsql.pm
- 7039c7cff673 16.0 cited
-
Fix missing installation rules for BackgroundPsql.pm
- a4c17c86176c 16.0 cited
-
Skip \password TAP test on old IPC::Run versions
- 2e57ffe12f6b 16.0 landed
-
Test SCRAM iteration changes with psql \password
- bf5a894c5571 16.0 landed
-
Refactor background psql TAP functions
- 664d757531e1 16.0 landed