From: Tom Lane <tgl@sss.pgh.pa.us>
To: pgsql-hackers@lists.postgresql.org
Cc: Daniel Gustafsson <daniel@yesql.se>
Subject: Re: authentication/t/001_password.pl trashes ~/.psql_history
Date: Fri, 22 Dec 2023 17:11:36 -0500
Message-ID: <1136842.1703283096@sss.pgh.pa.us> (raw)
In-Reply-To: <794610.1703182896@sss.pgh.pa.us>
References: <794610.1703182896@sss.pgh.pa.us>
I wrote:
> I happened to notice this stuff getting added to my .psql_history:
> \echo background_psql: ready
> SET password_encryption='scram-sha-256';
> ;
> \echo background_psql: QUERY_SEPARATOR
> SET scram_iterations=42;
> ;
> \echo background_psql: QUERY_SEPARATOR
> \password scram_role_iter
> \q
> After grepping for these strings, this is evidently the fault of
> src/test/authentication/t/001_password.pl by way of BackgroundPsql.pm,
> which fires up an interactive psql run that is not given the -n switch.
> Currently the only other user of interactive_psql() seems to be
> psql/t/010_tab_completion.pl, which avoids this problem by
> explicitly redirecting the history file. We could have 001_password.pl
> do likewise, or we could have it pass the -n switch, but I think we're
> going to have this problem resurface repeatedly if we leave it to the
> outer test script to remember to do it.
After studying this some more, my conclusion is that BackgroundPsql.pm
failed to borrow as much as it should have from 010_tab_completion.pl.
Specifically, we want all the environment-variable changes that that
script performed to be applied in any test using an interactive psql.
Maybe ~/.inputrc and so forth would never affect any other test scripts,
but that doesn't seem like a great bet.
So that leads me to the attached proposed patch.
regards, tom lane
Attachments:v1-set-environment-safely-in-interactive_psql.patch (text/x-diff, 4.0K) downloaddiff --git a/src/bin/psql/t/010_tab_completion.pl b/src/bin/psql/t/010_tab_completion.plindex 4cd0fa4680..f2d2809aef 100644--- a/src/bin/psql/t/010_tab_completion.pl+++ b/src/bin/psql/t/010_tab_completion.pl@@ -46,25 +46,6 @@ $node->safe_psql('postgres',
. "CREATE TYPE enum1 AS ENUM ('foo', 'bar', 'baz', 'BLACK');\n"
. "CREATE PUBLICATION some_publication;\n");
-# Developers would not appreciate this test adding a bunch of junk to-# their ~/.psql_history, so be sure to redirect history into a temp file.-# We might as well put it in the test log directory, so that buildfarm runs-# capture the result for possible debugging purposes.-my $historyfile = "${PostgreSQL::Test::Utils::log_path}/010_psql_history.txt";-$ENV{PSQL_HISTORY} = $historyfile;--# Another pitfall for developers is that they might have a ~/.inputrc-# file that changes readline's behavior enough to affect this test.-# So ignore any such file.-$ENV{INPUTRC} = '/dev/null';--# Unset $TERM so that readline/libedit won't use any terminal-dependent-# escape sequences; that leads to way too many cross-version variations-# in the output.-delete $ENV{TERM};-# Some versions of readline inspect LS_COLORS, so for luck unset that too.-delete $ENV{LS_COLORS};-
# In a VPATH build, we'll be started in the source directory, but we want
# to run in the build directory so that we can use relative paths to
# access the tab_comp_dir subdirectory; otherwise the output from filename
@@ -91,8 +72,13 @@ open $FH, ">", "tab_comp_dir/afile456"
print $FH "other stuff\n";
close $FH;
+# Arrange to capture, not discard, the interactive session's history output.+# Put it in the test log directory, so that buildfarm runs capture the result+# for possible debugging purposes.+my $historyfile = "${PostgreSQL::Test::Utils::log_path}/010_psql_history.txt";+
# fire up an interactive psql session
-my $h = $node->interactive_psql('postgres');+my $h = $node->interactive_psql('postgres', history_file => $historyfile);
# Simple test case: type something and see if psql responds as expected
sub check_completion
diff --git a/src/test/perl/PostgreSQL/Test/Cluster.pm b/src/test/perl/PostgreSQL/Test/Cluster.pmindex f43e54282f..4d207730c9 100644--- a/src/test/perl/PostgreSQL/Test/Cluster.pm+++ b/src/test/perl/PostgreSQL/Test/Cluster.pm@@ -2128,14 +2128,17 @@ Errors occurring later are the caller's problem.
Be sure to "quit" the returned object when done with it.
-The only extra parameter currently accepted is-
=over
=item extra_params => ['--single-transaction']
If given, it must be an array reference containing additional parameters to B<psql>.
+=item history_file => B<path>++Cause the interactive B<psql> session to write its command history to B<path>.+If not given, the history is sent to /dev/null.+
=back
This requires IO::Pty in addition to IPC::Run.
@@ -2148,6 +2151,29 @@ sub interactive_psql
local %ENV = $self->_get_env();
+ # Since the invoked psql will believe it's interactive, it will use+ # readline/libedit if available. We need to adjust some environment+ # settings to prevent unwanted side-effects.++ # Developers would not appreciate tests adding a bunch of junk to+ # their ~/.psql_history, so redirect readline history somewhere else.+ # If the calling script doesn't specify anything, just bit-bucket it.+ my $history_file = $params{history_file};+ $history_file ||= '/dev/null';+ $ENV{PSQL_HISTORY} = $history_file;++ # Another pitfall for developers is that they might have a ~/.inputrc+ # file that changes readline's behavior enough to affect the test.+ # So ignore any such file.+ $ENV{INPUTRC} = '/dev/null';++ # Unset $TERM so that readline/libedit won't use any terminal-dependent+ # escape sequences; that leads to way too many cross-version variations+ # in the output.+ delete $ENV{TERM};+ # Some versions of readline inspect LS_COLORS, so for luck unset that too.+ delete $ENV{LS_COLORS};+
my @psql_params = (
$self->installed_command('psql'),
'-XAt', '-d', $self->connstr($dbname));
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: tgl@sss.pgh.pa.us, pgsql-hackers@lists.postgresql.org, daniel@yesql.se
Subject: Re: authentication/t/001_password.pl trashes ~/.psql_history
In-Reply-To: <1136842.1703283096@sss.pgh.pa.us>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox