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
In-reply-to: <794610.1703182896@sss.pgh.pa.us>
References: <794610.1703182896@sss.pgh.pa.us>
Comments: In-reply-to Tom Lane <tgl@sss.pgh.pa.us>
	message dated "Thu, 21 Dec 2023 13:21:36 -0500"
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0"
Content-ID: <1136520.1703282950.0@sss.pgh.pa.us>
Date: Fri, 22 Dec 2023 17:11:36 -0500
Message-ID: <1136842.1703283096@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/1136842.1703283096%40sss.pgh.pa.us>
Precedence: bulk

------- =_aaaaaaaaaa0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <1136520.1703282950.1@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


------- =_aaaaaaaaaa0
Content-Type: text/x-diff;
	name="v1-set-environment-safely-in-interactive_psql.patch";
	charset="us-ascii"
Content-ID: <1136520.1703282950.2@sss.pgh.pa.us>
Content-Description: v1-set-environment-safely-in-interactive_psql.patch
Content-Transfer-Encoding: quoted-printable

diff --git a/src/bin/psql/t/010_tab_completion.pl b/src/bin/psql/t/010_tab=
_completion.pl
index 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 ru=
ns
-# capture the result for possible debugging purposes.
-my $historyfile =3D "${PostgreSQL::Test::Utils::log_path}/010_psql_histor=
y.txt";
-$ENV{PSQL_HISTORY} =3D $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} =3D '/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 filenam=
e
@@ -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 outp=
ut.
+# Put it in the test log directory, so that buildfarm runs capture the re=
sult
+# for possible debugging purposes.
+my $historyfile =3D "${PostgreSQL::Test::Utils::log_path}/010_psql_histor=
y.txt";
+
 # fire up an interactive psql session
-my $h =3D $node->interactive_psql('postgres');
+my $h =3D $node->interactive_psql('postgres', history_file =3D> $historyf=
ile);
 =

 # 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/Post=
greSQL/Test/Cluster.pm
index 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
-
 =3Dover
 =

 =3Ditem extra_params =3D> ['--single-transaction']
 =

 If given, it must be an array reference containing additional parameters =
to B<psql>.
 =

+=3Ditem history_file =3D> B<path>
+
+Cause the interactive B<psql> session to write its command history to B<p=
ath>.
+If not given, the history is sent to /dev/null.
+
 =3Dback
 =

 This requires IO::Pty in addition to IPC::Run.
@@ -2148,6 +2151,29 @@ sub interactive_psql
 =

 	local %ENV =3D $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 =3D $params{history_file};
+	$history_file ||=3D '/dev/null';
+	$ENV{PSQL_HISTORY} =3D $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} =3D '/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 to=
o.
+	delete $ENV{LS_COLORS};
+
 	my @psql_params =3D (
 		$self->installed_command('psql'),
 		'-XAt', '-d', $self->connstr($dbname));

------- =_aaaaaaaaaa0--