Обсуждение: authentication/t/001_password.pl trashes ~/.psql_history

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.

My first idea was that BackgroundPsql.pm should take responsibility for
preventing this, by explicitly setting $ENV{PSQL_HISTORY} to "/dev/null"
if the calling script hasn't set some other value.  However, that could
fail if the user who runs the test habitually sets PSQL_HISTORY.

A messier but safer alternative would be to supply the -n switch by
default, with some way for 010_tab_completion.pl to override that.

Thoughts?

            regards, tom lane

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

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 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.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
-
 =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));

On 2023-12-22 Fr 17:11, Tom Lane wrote:
> 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.


Looks fine, after reading your original post I was thinking along the 
same lines.

You could shorten this

+    my $history_file = $params{history_file};
+    $history_file ||= '/dev/null';
+    $ENV{PSQL_HISTORY} = $history_file;

to just

      $ENV{PSQL_HISTORY} = $params{history_file} || '/dev/null';


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Andrew Dunstan <andrew@dunslane.net> writes:
> On 2023-12-22 Fr 17:11, Tom Lane wrote:
>> 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.

> Looks fine, after reading your original post I was thinking along the 
> same lines.

Thanks for reviewing.

> You could shorten this
> +    my $history_file = $params{history_file};
> +    $history_file ||= '/dev/null';
> +    $ENV{PSQL_HISTORY} = $history_file;
> to just
>       $ENV{PSQL_HISTORY} = $params{history_file} || '/dev/null';

OK.  I was unsure which way would be considered more readable,
but based on your advice I shortened it.

            regards, tom lane

> On 23 Dec 2023, at 17:52, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 
> Andrew Dunstan <andrew@dunslane.net> writes:
>> On 2023-12-22 Fr 17:11, Tom Lane wrote:
>>> 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.
> 
>> Looks fine, after reading your original post I was thinking along the 
>> same lines.
> 
> Thanks for reviewing.
> 
>> You could shorten this
>> +    my $history_file = $params{history_file};
>> +    $history_file ||= '/dev/null';
>> +    $ENV{PSQL_HISTORY} = $history_file;
>> to just
>>      $ENV{PSQL_HISTORY} = $params{history_file} || '/dev/null';
> 
> OK.  I was unsure which way would be considered more readable,
> but based on your advice I shortened it.

Sorry for jumping in late, only saw this now (ETOOMUCHCHRISTMASPREP) but the
committed patch looks good to me too.  Thanks!

--
Daniel Gustafsson