Horton

Thread

Commits

  1. Set readline-relevant ENV vars in interactive_psql(), not caller.

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-12-21T18:21:36Z

    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

  2. Re: authentication/t/001_password.pl trashes ~/.psql_history

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-12-22T22:11:36Z

    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

  3. Re: authentication/t/001_password.pl trashes ~/.psql_history

    Andrew Dunstan <andrew@dunslane.net> — 2023-12-23T13:20:07Z

    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

  4. Re: authentication/t/001_password.pl trashes ~/.psql_history

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-12-23T16:52:56Z

    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

  5. Re: authentication/t/001_password.pl trashes ~/.psql_history

    Daniel Gustafsson <daniel@yesql.se> — 2023-12-23T17:31:16Z

    
> 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