authentication/t/001_password.pl trashes ~/.psql_history

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

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