Making background psql nicer to use in tap tests

From: Andres Freund <andres(at)anarazel(dot)de>
To: pgsql-hackers(at)postgresql(dot)org
Cc: Melanie Plageman <melanieplageman(at)gmail(dot)com>
Subject: Making background psql nicer to use in tap tests
Date: 2023-01-30 19:43:50
Message-ID: 20230130194350.zj5v467x4jgqt3d6@awork3.anarazel.de
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
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

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Andres Freund 2023-01-30 19:48:10 Re: recovery modules
Previous Message Nathan Bossart 2023-01-30 19:38:20 Re: recovery modules