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

  1. Fix missing installation/uninstallation rules for BackgroundPsql.pm

  2. Backport BackgroundPsql perl test module

  3. Add missing uninstallation rule for BackgroundPsql.pm

  4. Fix missing installation rules for BackgroundPsql.pm

  5. Skip \password TAP test on old IPC::Run versions

  6. Test SCRAM iteration changes with psql \password

  7. Refactor background psql TAP functions