pg.ddx.io pgsql-hackers@postgresql.org mailing list archive
help / color / mirror / Atom feedauthentication/t/001_password.pl trashes ~/.psql_history
5+ messages / 3 participants
[nested] [flat]
Commits from this thread:
2023-12-23 da44ff312e 11652f919d Set readline-relevant ENV vars in interactive_psql(), not caller.
* authentication/t/001_password.pl trashes ~/.psql_history
@ 2023-12-21 18:21 Tom Lane <tgl@sss.pgh.pa.us>
2023-12-22 22:11 ` Re: authentication/t/001_password.pl trashes ~/.psql_history Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 5+ messages in thread
From: Tom Lane @ 2023-12-21 18:21 UTC (permalink / raw)
To: pgsql-hackers@lists.postgresql.org
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
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: authentication/t/001_password.pl trashes ~/.psql_history
2023-12-21 18:21 authentication/t/001_password.pl trashes ~/.psql_history Tom Lane <tgl@sss.pgh.pa.us>
@ 2023-12-22 22:11 ` Tom Lane <tgl@sss.pgh.pa.us>
2023-12-23 13:20 ` Re: authentication/t/001_password.pl trashes ~/.psql_history Andrew Dunstan <andrew@dunslane.net>
0 siblings, 1 reply; 5+ messages in thread
From: Tom Lane @ 2023-12-22 22:11 UTC (permalink / raw)
To: pgsql-hackers@lists.postgresql.org; +Cc: Daniel Gustafsson <daniel@yesql.se>
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) download
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));
* Re: authentication/t/001_password.pl trashes ~/.psql_history
2023-12-21 18:21 authentication/t/001_password.pl trashes ~/.psql_history Tom Lane <tgl@sss.pgh.pa.us>
2023-12-22 22:11 ` Re: authentication/t/001_password.pl trashes ~/.psql_history Tom Lane <tgl@sss.pgh.pa.us>
@ 2023-12-23 13:20 ` Andrew Dunstan <andrew@dunslane.net>
2023-12-23 16:52 ` Re: authentication/t/001_password.pl trashes ~/.psql_history Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 5+ messages in thread
From: Andrew Dunstan @ 2023-12-23 13:20 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; pgsql-hackers@lists.postgresql.org; +Cc: Daniel Gustafsson <daniel@yesql.se>
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
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: authentication/t/001_password.pl trashes ~/.psql_history
2023-12-21 18:21 authentication/t/001_password.pl trashes ~/.psql_history Tom Lane <tgl@sss.pgh.pa.us>
2023-12-22 22:11 ` Re: authentication/t/001_password.pl trashes ~/.psql_history Tom Lane <tgl@sss.pgh.pa.us>
2023-12-23 13:20 ` Re: authentication/t/001_password.pl trashes ~/.psql_history Andrew Dunstan <andrew@dunslane.net>
@ 2023-12-23 16:52 ` Tom Lane <tgl@sss.pgh.pa.us>
2023-12-23 17:31 ` Re: authentication/t/001_password.pl trashes ~/.psql_history Daniel Gustafsson <daniel@yesql.se>
0 siblings, 1 reply; 5+ messages in thread
From: Tom Lane @ 2023-12-23 16:52 UTC (permalink / raw)
To: Andrew Dunstan <andrew@dunslane.net>; +Cc: pgsql-hackers@lists.postgresql.org, Daniel Gustafsson <daniel@yesql.se>
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
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: authentication/t/001_password.pl trashes ~/.psql_history
2023-12-21 18:21 authentication/t/001_password.pl trashes ~/.psql_history Tom Lane <tgl@sss.pgh.pa.us>
2023-12-22 22:11 ` Re: authentication/t/001_password.pl trashes ~/.psql_history Tom Lane <tgl@sss.pgh.pa.us>
2023-12-23 13:20 ` Re: authentication/t/001_password.pl trashes ~/.psql_history Andrew Dunstan <andrew@dunslane.net>
2023-12-23 16:52 ` Re: authentication/t/001_password.pl trashes ~/.psql_history Tom Lane <tgl@sss.pgh.pa.us>
@ 2023-12-23 17:31 ` Daniel Gustafsson <daniel@yesql.se>
0 siblings, 0 replies; 5+ messages in thread
From: Daniel Gustafsson @ 2023-12-23 17:31 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Andrew Dunstan <andrew@dunslane.net>; pgsql-hackers@lists.postgresql.org
> 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
^ permalink raw reply [nested|flat] 5+ messages in thread
end of thread, other threads:[~2023-12-23 17:31 UTC | newest]
Thread overview: 5+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2023-12-21 18:21 authentication/t/001_password.pl trashes ~/.psql_history Tom Lane <tgl@sss.pgh.pa.us>
2023-12-22 22:11 ` Tom Lane <tgl@sss.pgh.pa.us>
2023-12-23 13:20 ` Andrew Dunstan <andrew@dunslane.net>
2023-12-23 16:52 ` Tom Lane <tgl@sss.pgh.pa.us>
2023-12-23 17:31 ` Daniel Gustafsson <daniel@yesql.se>
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