Horton

Thread

Commits

  1. vacuumdb: don't prompt for passwords over and over

  1. BUG #13741: vacuumdb does not accept valid password

    brown@fastmail.com — 2015-10-27T19:39:19Z

    The following bug has been logged on the website:

Bug reference:      13741
Logged by:          Eric Brown
Email address:      brown@fastmail.com
PostgreSQL version: 9.5beta1
Operating system:   CentOS 6.6
Description:        

I am trying to vacuum a database from a remote computer.  It appears that
vacuumdb is not accepting my password, though psql connects just fine.
______
browne1@reslin3 ~/Desktop> vacuumdb -z -j 24 -h reslin1.enhnet.org -p 5433  
                                                      127
vacuumdb: vacuuming database "browne1"
Password: 
Password: 
Password: 
vacuumdb: could not connect to database browne1: FATAL:  password
authentication failed for user "browne1"
browne1@reslin3 ~/Desktop> psql -h reslin1.enhnet.org -p 5433               
                                                        1
Password: 
psql (9.5beta1)
Type "help" for help.

browne1=# 
______

  2. Re: BUG #13741: vacuumdb does not accept valid password

    Haribabu Kommi <kommi.haribabu@gmail.com> — 2015-10-28T03:53:19Z

    On Wed, Oct 28, 2015 at 6:39 AM,  <brown@fastmail.com> wrote:
> The following bug has been logged on the website:
>
> Bug reference:      13741
> Logged by:          Eric Brown
> Email address:      brown@fastmail.com
> PostgreSQL version: 9.5beta1
> Operating system:   CentOS 6.6
> Description:
>
> I am trying to vacuum a database from a remote computer.  It appears that
> vacuumdb is not accepting my password, though psql connects just fine.
>
>______
> browne1@reslin3 ~/Desktop> vacuumdb -z -j 24 -h reslin1.enhnet.org -p 5433
>                                                       127
> vacuumdb: vacuuming database "browne1"
> Password:
> Password:
> Password:
> vacuumdb: could not connect to database browne1: FATAL:  password
> authentication failed for user "browne1"
> browne1@reslin3 ~/Desktop> psql -h reslin1.enhnet.org -p 5433
>                                                         1
> Password:
> psql (9.5beta1)
> Type "help" for help.
>
> browne1=#
> ______

No. vacuumdb is accepting your password correctly. But you gave the number
of client connections as 24, to connect all these 24 clients to the
database, it is requesting
the password to enter again.

you can try with vacuumb -z -j2 to verify the same.

I feel we need to accept the password once and reuse it for all client
connections.
Provided working password can be returned for that database.

Regards,
Hari Babu
Fujitsu Australia

  3. Re: BUG #13741: vacuumdb does not accept valid password

    Haribabu Kommi <kommi.haribabu@gmail.com> — 2015-10-28T05:07:20Z

    On Wed, Oct 28, 2015 at 2:53 PM, Haribabu Kommi
<kommi.haribabu@gmail.com> wrote:
> On Wed, Oct 28, 2015 at 6:39 AM,  <brown@fastmail.com> wrote:
>> The following bug has been logged on the website:
>>
>> Bug reference:      13741
>> Logged by:          Eric Brown
>> Email address:      brown@fastmail.com
>> PostgreSQL version: 9.5beta1
>> Operating system:   CentOS 6.6
>> Description:
>>
>> I am trying to vacuum a database from a remote computer.  It appears that
>> vacuumdb is not accepting my password, though psql connects just fine.
>>
>>______
>> browne1@reslin3 ~/Desktop> vacuumdb -z -j 24 -h reslin1.enhnet.org -p 5433
>>                                                       127
>> vacuumdb: vacuuming database "browne1"
>> Password:
>> Password:
>> Password:
>> vacuumdb: could not connect to database browne1: FATAL:  password
>> authentication failed for user "browne1"
>> browne1@reslin3 ~/Desktop> psql -h reslin1.enhnet.org -p 5433
>>                                                         1
>> Password:
>> psql (9.5beta1)
>> Type "help" for help.
>>
>> browne1=#
>> ______
>
> No. vacuumdb is accepting your password correctly. But you gave the number
> of client connections as 24, to connect all these 24 clients to the
> database, it is requesting
> the password to enter again.
>
> you can try with vacuumb -z -j2 to verify the same.
>
> I feel we need to accept the password once and reuse it for all client
> connections.
> Provided working password can be returned for that database.

Here I attached a patch that saves the password provided by the user
from the connectDatabase function and reuse it for connecting all clients
to the same database.

Regards,
Hari Babu
Fujitsu Australia

  4. Re: BUG #13741: vacuumdb does not accept valid password

    Haribabu Kommi <kommi.haribabu@gmail.com> — 2015-10-29T00:13:00Z

    On Wed, Oct 28, 2015 at 4:07 PM, Haribabu Kommi
<kommi.haribabu@gmail.com> wrote:
> Here I attached a patch that saves the password provided by the user
> from the connectDatabase function and reuse it for connecting all clients
> to the same database.

Attached a wrong patch, it is having some compilation problems,
Here is the new version with the fixed problems.

Regards,
Hari Babu
Fujitsu Australia

  5. Re: BUG #13741: vacuumdb does not accept valid password

    Michael Paquier <michael.paquier@gmail.com> — 2015-11-02T06:56:59Z

    On Thu, Oct 29, 2015 at 9:13 AM, Haribabu Kommi
<kommi.haribabu@gmail.com> wrote:
> On Wed, Oct 28, 2015 at 4:07 PM, Haribabu Kommi
> <kommi.haribabu@gmail.com> wrote:
>> Here I attached a patch that saves the password provided by the user
>> from the connectDatabase function and reuse it for connecting all clients
>> to the same database.
>
> Attached a wrong patch, it is having some compilation problems,
> Here is the new version with the fixed problems.

I have added an entry in this CF:
https://commitfest.postgresql.org/7/417/
Let's not lose track of this patch.
-- 
Michael

  6. Re: BUG #13741: vacuumdb does not accept valid password

    Michael Paquier <michael.paquier@gmail.com> — 2015-11-02T07:53:44Z

    On Mon, Nov 2, 2015 at 3:56 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Thu, Oct 29, 2015 at 9:13 AM, Haribabu Kommi
> <kommi.haribabu@gmail.com> wrote:
>> On Wed, Oct 28, 2015 at 4:07 PM, Haribabu Kommi
>> <kommi.haribabu@gmail.com> wrote:
>>> Here I attached a patch that saves the password provided by the user
>>> from the connectDatabase function and reuse it for connecting all clients
>>> to the same database.
>>
>> Attached a wrong patch, it is having some compilation problems,
>> Here is the new version with the fixed problems.
>
> I have added an entry in this CF:
> https://commitfest.postgresql.org/7/417/
> Let's not lose track of this patch.

Regarding this patch, wouldn't it be clearer to pass the password as a
variable of connectDatabase()? Then we could use NULL at the first
call of connectDatabase so as we enforce the prompt if requested by
the user. For successive calls of connectDatabase for each worker, we
then fetch the password from the parent connection using that:
if (PQconnectionUsedPassword(con))
    password = PQpass(conn);
And pass it as an argument of connectDatabase. In short, I think that
this approach would make a more portable routine because one could
enforce a password at the first call of connectDatabase() without
having to save it once.
-- 
Michael

  7. Re: BUG #13741: vacuumdb does not accept valid password

    Haribabu Kommi <kommi.haribabu@gmail.com> — 2015-11-02T11:10:59Z

    On Mon, Nov 2, 2015 at 6:53 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Mon, Nov 2, 2015 at 3:56 PM, Michael Paquier
> <michael.paquier@gmail.com> wrote:
>> On Thu, Oct 29, 2015 at 9:13 AM, Haribabu Kommi
>> <kommi.haribabu@gmail.com> wrote:
>>> On Wed, Oct 28, 2015 at 4:07 PM, Haribabu Kommi
>>> <kommi.haribabu@gmail.com> wrote:
>>>> Here I attached a patch that saves the password provided by the user
>>>> from the connectDatabase function and reuse it for connecting all clients
>>>> to the same database.
>>>
>>> Attached a wrong patch, it is having some compilation problems,
>>> Here is the new version with the fixed problems.
>>
>> I have added an entry in this CF:
>> https://commitfest.postgresql.org/7/417/
>> Let's not lose track of this patch.

Thanks for adding the patch to commitfest.

> Regarding this patch, wouldn't it be clearer to pass the password as a
> variable of connectDatabase()? Then we could use NULL at the first
> call of connectDatabase so as we enforce the prompt if requested by
> the user. For successive calls of connectDatabase for each worker, we
> then fetch the password from the parent connection using that:
> if (PQconnectionUsedPassword(con))
>     password = PQpass(conn);
> And pass it as an argument of connectDatabase. In short, I think that
> this approach would make a more portable routine because one could
> enforce a password at the first call of connectDatabase() without
> having to save it once.

Thanks for the review.
Here I attached modified patch that gets the password from parent connection
if exists and use it for the child connection also.


Regards,
Hari Babu
Fujitsu Australia

  8. Re: BUG #13741: vacuumdb does not accept valid password

    Michael Paquier <michael.paquier@gmail.com> — 2015-11-02T13:04:50Z

    On Mon, Nov 2, 2015 at 8:10 PM, Haribabu Kommi wrote:
> Here I attached modified patch that gets the password from parent connection
> if exists and use it for the child connection also.

Meh? Why the parent connection? You could simply have the password as
an argument of connectDatabase, per se the attached. That looks just
more simple.
-- 
Michael

  9. Re: BUG #13741: vacuumdb does not accept valid password

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2015-11-02T19:26:26Z

    Michael Paquier wrote:
> On Mon, Nov 2, 2015 at 8:10 PM, Haribabu Kommi wrote:
> > Here I attached modified patch that gets the password from parent connection
> > if exists and use it for the child connection also.
> 
> Meh? Why the parent connection? You could simply have the password as
> an argument of connectDatabase, per se the attached. That looks just
> more simple.

Thanks!  This is almost there, but there's an important missing piece --
vacuumdb --all will ask you for a password for each database.  I think
vacuum_one_database needs to receive a password from its caller too.

(Also, please use pg_strdup.)

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  10. Re: BUG #13741: vacuumdb does not accept valid password

    Haribabu Kommi <kommi.haribabu@gmail.com> — 2015-11-02T21:10:12Z

    On Tue, Nov 3, 2015 at 6:26 AM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
> Michael Paquier wrote:
>> On Mon, Nov 2, 2015 at 8:10 PM, Haribabu Kommi wrote:
>> > Here I attached modified patch that gets the password from parent connection
>> > if exists and use it for the child connection also.
>>
>> Meh? Why the parent connection? You could simply have the password as
>> an argument of connectDatabase, per se the attached. That looks just
>> more simple.
>
> Thanks!  This is almost there, but there's an important missing piece --
> vacuumdb --all will ask you for a password for each database.  I think
> vacuum_one_database needs to receive a password from its caller too.

Here I attached a separate patch to handle the reuse of password for
vacuumdb -all
case. The same behavior exists in all supported branches.

> (Also, please use pg_strdup.)

The simple_prompt function in connectDatabase allocates memory for password
using malloc. Because of this reason he used strdup instead of pstrdup to avoid
adding a separate code to handle both the cases.

Regards,
Hari Babu
Fujitsu Australia

  11. Re: BUG #13741: vacuumdb does not accept valid password

    Michael Paquier <michael.paquier@gmail.com> — 2015-11-03T13:06:18Z

    On Tue, Nov 3, 2015 at 6:10 AM, Haribabu Kommi <kommi.haribabu@gmail.com> wrote:
> On Tue, Nov 3, 2015 at 6:26 AM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
>> Michael Paquier wrote:
>>> On Mon, Nov 2, 2015 at 8:10 PM, Haribabu Kommi wrote:
>>> > Here I attached modified patch that gets the password from parent connection
>>> > if exists and use it for the child connection also.
>>>
>>> Meh? Why the parent connection? You could simply have the password as
>>> an argument of connectDatabase, per se the attached. That looks just
>>> more simple.
>>
>> Thanks!  This is almost there, but there's an important missing piece --
>> vacuumdb --all will ask you for a password for each database.  I think
>> vacuum_one_database needs to receive a password from its caller too.

Argh, right.

> Here I attached a separate patch to handle the reuse of password for
> vacuumdb -all
> case. The same behavior exists in all supported branches.

Sure. Still you don't actually need a double pointer as you do. You
could just reuse the password from the connection obtained via
connectMaintenanceDatabase and pass the password from this connection
as the argument to vacuum_one_database. Something like the attached
seems more elegant IMO.

>> (Also, please use pg_strdup.)
> The simple_prompt function in connectDatabase allocates memory for password
> using malloc. Because of this reason he used strdup instead of pstrdup to avoid
> adding a separate code to handle both the cases.

Yep. That's the deal.
-- 
Michael

  12. Re: BUG #13741: vacuumdb does not accept valid password

    Haribabu Kommi <kommi.haribabu@gmail.com> — 2015-11-04T00:24:12Z

    On Wed, Nov 4, 2015 at 12:06 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Tue, Nov 3, 2015 at 6:10 AM, Haribabu Kommi <kommi.haribabu@gmail.com> wrote:
>> Here I attached a separate patch to handle the reuse of password for
>> vacuumdb -all
>> case. The same behavior exists in all supported branches.
>
> Sure. Still you don't actually need a double pointer as you do. You
> could just reuse the password from the connection obtained via
> connectMaintenanceDatabase and pass the password from this connection
> as the argument to vacuum_one_database. Something like the attached
> seems more elegant IMO.

Why I used a double pointer is to support the scenario like the following.
- There is no password requirement for Postgres, template1 and
maintenance db that is provided by the user.
- But there is a password requirement for user databases.
- If user doesn't provide the password during connection to
Maintenance database, later it prompts for
password while connecting to user database.
- Without the double pointer, further on for every database, it
prompts for the password and also
   the case of --analyze-in-stages prompts for password for the all the stages.


Regards,
Hari Babu
Fujitsu Australia

  13. Re: BUG #13741: vacuumdb does not accept valid password

    Haribabu Kommi <kommi.haribabu@gmail.com> — 2015-11-04T00:36:42Z

    On Wed, Nov 4, 2015 at 11:24 AM, Haribabu Kommi
<kommi.haribabu@gmail.com> wrote:
> On Wed, Nov 4, 2015 at 12:06 AM, Michael Paquier
> <michael.paquier@gmail.com> wrote:
>> On Tue, Nov 3, 2015 at 6:10 AM, Haribabu Kommi <kommi.haribabu@gmail.com> wrote:
>>> Here I attached a separate patch to handle the reuse of password for
>>> vacuumdb -all
>>> case. The same behavior exists in all supported branches.
>>
>> Sure. Still you don't actually need a double pointer as you do. You
>> could just reuse the password from the connection obtained via
>> connectMaintenanceDatabase and pass the password from this connection
>> as the argument to vacuum_one_database. Something like the attached
>> seems more elegant IMO.
>
> Why I used a double pointer is to support the scenario like the following.
> - There is no password requirement for Postgres, template1 and
> maintenance db that is provided by the user.
> - But there is a password requirement for user databases.
> - If user doesn't provide the password during connection to
> Maintenance database, later it prompts for
> password while connecting to user database.
> - Without the double pointer, further on for every database, it
> prompts for the password and also
>    the case of --analyze-in-stages prompts for password for the all the stages.

And one more thing, the vacuumdb password behavior is present in back branches
also, is it worth back patching the vacuumdb fix to all supported
branches and apply
the jobs connection fix only to master and 9.5?


Regards,
Hari Babu
Fujitsu Australia

  14. Re: BUG #13741: vacuumdb does not accept valid password

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2015-11-04T02:16:18Z

    Haribabu Kommi wrote:
> On Wed, Nov 4, 2015 at 11:24 AM, Haribabu Kommi
> <kommi.haribabu@gmail.com> wrote:

> And one more thing, the vacuumdb password behavior is present in back branches
> also, is it worth back patching the vacuumdb fix to all supported
> branches and apply
> the jobs connection fix only to master and 9.5?

Given the lack of complaints, I doubt it's worth the destabilization
risk.  Let's just patch 9.5 and be done with it.

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  15. Re: BUG #13741: vacuumdb does not accept valid password

    Michael Paquier <michael.paquier@gmail.com> — 2015-11-04T07:26:58Z

    On Wed, Nov 4, 2015 at 9:24 AM, Haribabu Kommi wrote:
> On Wed, Nov 4, 2015 at 12:06 AM, Michael Paquier wrote:
> Why I used a double pointer is to support the scenario like the following.
> - There is no password requirement for Postgres, template1 and
> maintenance db that is provided by the user.
> - But there is a password requirement for user databases.
> - If user doesn't provide the password during connection to
> Maintenance database, later it prompts for
> password while connecting to user database.
> - Without the double pointer, further on for every database, it
> prompts for the password and also
>    the case of --analyze-in-stages prompts for password for the all the stages.

Yeah, I thought a bit about that while writing the last patch but
that's quite narrow, no? So I didn't think it was worth doing, the
approach to request a password only after connecting to the
maintenance database instead of relying on the first one given out by
user looked more simple and solid to me, and is able to cover all the
test scenarios either way as one can specify the maintenance database
he wishes to connect to.
Thoughts from other folks?
-- 
Michael

  16. Re: BUG #13741: vacuumdb does not accept valid password

    Michael Paquier <michael.paquier@gmail.com> — 2015-11-04T07:30:01Z

    On Wed, Nov 4, 2015 at 11:16 AM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:
> Haribabu Kommi wrote:
>> On Wed, Nov 4, 2015 at 11:24 AM, Haribabu Kommi
>> <kommi.haribabu@gmail.com> wrote:
>
>> And one more thing, the vacuumdb password behavior is present in back branches
>> also, is it worth back patching the vacuumdb fix to all supported
>> branches and apply
>> the jobs connection fix only to master and 9.5?
>
> Given the lack of complaints, I doubt it's worth the destabilization
> risk.  Let's just patch 9.5 and be done with it.

Fine for me. --all is supported for ages.
-- 
Michael

  17. Re: BUG #13741: vacuumdb does not accept valid password

    Michael Paquier <michael.paquier@gmail.com> — 2015-11-06T06:53:47Z

    On Wed, Nov 4, 2015 at 4:30 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Wed, Nov 4, 2015 at 11:16 AM, Alvaro Herrera
> <alvherre@2ndquadrant.com> wrote:
>> Haribabu Kommi wrote:
>>> On Wed, Nov 4, 2015 at 11:24 AM, Haribabu Kommi
>>> <kommi.haribabu@gmail.com> wrote:
>>
>>> And one more thing, the vacuumdb password behavior is present in back branches
>>> also, is it worth back patching the vacuumdb fix to all supported
>>> branches and apply
>>> the jobs connection fix only to master and 9.5?
>>
>> Given the lack of complaints, I doubt it's worth the destabilization
>> risk.  Let's just patch 9.5 and be done with it.
>
> Fine for me. --all is supported for ages.

OK, so attached is a patch aimed at master and 9.5. I reused the
suggestion of Haribabu to not rely completely on the maintenance
database after testing with a couple of database, some of them using
md5 and others trust. That's just more portable this way, and user
just needs to specify the password once to be done even with vacuumdb
--all.
Thoughts?
-- 
Michael

  18. Re: BUG #13741: vacuumdb does not accept valid password

    Haribabu Kommi <kommi.haribabu@gmail.com> — 2015-11-12T06:03:17Z

    On Fri, Nov 6, 2015 at 5:53 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Wed, Nov 4, 2015 at 4:30 PM, Michael Paquier
> <michael.paquier@gmail.com> wrote:
>> On Wed, Nov 4, 2015 at 11:16 AM, Alvaro Herrera
>> <alvherre@2ndquadrant.com> wrote:
>>> Haribabu Kommi wrote:
>>>> On Wed, Nov 4, 2015 at 11:24 AM, Haribabu Kommi
>>>> <kommi.haribabu@gmail.com> wrote:
>>>
>>>> And one more thing, the vacuumdb password behavior is present in back branches
>>>> also, is it worth back patching the vacuumdb fix to all supported
>>>> branches and apply
>>>> the jobs connection fix only to master and 9.5?
>>>
>>> Given the lack of complaints, I doubt it's worth the destabilization
>>> risk.  Let's just patch 9.5 and be done with it.
>>
>> Fine for me. --all is supported for ages.
>
> OK, so attached is a patch aimed at master and 9.5. I reused the
> suggestion of Haribabu to not rely completely on the maintenance
> database after testing with a couple of database, some of them using
> md5 and others trust. That's just more portable this way, and user
> just needs to specify the password once to be done even with vacuumdb
> --all.
> Thoughts?

Thanks for the patch. Yes, reusing the password helps for --analyze-in-stages
for single database case also along with -all.

Regards,
Hari Babu
Fujitsu Australia

  19. Re: BUG #13741: vacuumdb does not accept valid password

    Fujii Masao <masao.fujii@gmail.com> — 2015-11-12T16:44:35Z

    On Fri, Nov 6, 2015 at 3:53 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Wed, Nov 4, 2015 at 4:30 PM, Michael Paquier
> <michael.paquier@gmail.com> wrote:
>> On Wed, Nov 4, 2015 at 11:16 AM, Alvaro Herrera
>> <alvherre@2ndquadrant.com> wrote:
>>> Haribabu Kommi wrote:
>>>> On Wed, Nov 4, 2015 at 11:24 AM, Haribabu Kommi
>>>> <kommi.haribabu@gmail.com> wrote:
>>>
>>>> And one more thing, the vacuumdb password behavior is present in back branches
>>>> also, is it worth back patching the vacuumdb fix to all supported
>>>> branches and apply
>>>> the jobs connection fix only to master and 9.5?
>>>
>>> Given the lack of complaints, I doubt it's worth the destabilization
>>> risk.  Let's just patch 9.5 and be done with it.
>>
>> Fine for me. --all is supported for ages.
>
> OK, so attached is a patch aimed at master and 9.5. I reused the
> suggestion of Haribabu to not rely completely on the maintenance
> database after testing with a couple of database, some of them using
> md5 and others trust. That's just more portable this way, and user
> just needs to specify the password once to be done even with vacuumdb
> --all.
> Thoughts?

ISTM that the attached simpler patch can fix the problem.
But maybe I'm missing something...

Regards,

-- 
Fujii Masao

  20. Re: BUG #13741: vacuumdb does not accept valid password

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2015-11-12T17:03:03Z

    Fujii Masao wrote:

> ISTM that the attached simpler patch can fix the problem.
> But maybe I'm missing something...

Hmm, this is a very good and simple idea that looks like it should do
the trick.  I would just add a comment explaining why we're using a
static.

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  21. Re: BUG #13741: vacuumdb does not accept valid password

    Tom Lane <tgl@sss.pgh.pa.us> — 2015-11-12T19:41:03Z

    Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> Fujii Masao wrote:
>> ISTM that the attached simpler patch can fix the problem.
>> But maybe I'm missing something...

> Hmm, this is a very good and simple idea that looks like it should do
> the trick.  I would just add a comment explaining why we're using a
> static.

NAK.  This changes the behavior of connectDatabase() for *all* users
of that function, not only vacuumdb.  But the proposed behavioral
change is only appropriate for calling programs in which only a single
host/port/database target is used per execution.  In other contexts,
reusing the prior password is not just inappropriate but could actually
create security issues.  (It's possible that this behavior would be okay
for all existing callers, but that doesn't mean we should put in a
security gotcha for future uses.)

We could make this approach work if connectDatabase() remembered all
the parameters internally, and only tried to reuse the password when
they all match.  Or maybe it'd be better to alter the API so the caller
can say whether to try to reuse a saved password or not.  But I'm not sure
whether either of those answers is cleaner than the previous patch.

(BTW, I notice that pg_dumpall.c has a version of connectDatabase in
which the "static" trick is already being used, sans any documentation.
That's okay for pg_dumpall, but might be an issue if anyone copies-and-
pastes that version somewhere else ... and in any case it's fair to ask
why that version hasn't been merged with common.c.)

			regards, tom lane

  22. Re: BUG #13741: vacuumdb does not accept valid password

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2015-11-12T21:08:50Z

    Michael Paquier wrote:

> OK, so attached is a patch aimed at master and 9.5. I reused the
> suggestion of Haribabu to not rely completely on the maintenance
> database after testing with a couple of database, some of them using
> md5 and others trust. That's just more portable this way, and user
> just needs to specify the password once to be done even with vacuumdb
> --all.

Thanks, pushed a slightly tweaked version.  If you can please verify
that I didn't break anything, it'd be great.

FWIW I think the mode that generated the most connections is -jN --all
--analyze-in-stages.  For additional annoyance, one sets up the
"postgres" database with trust auth and others with md5.  All the cases
I tried only get a single prompt now.

Thanks Eric for reporting the problem and Haribabu and Michael for
patching.

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  23. Re: BUG #13741: vacuumdb does not accept valid password

    Tom Lane <tgl@sss.pgh.pa.us> — 2015-11-12T21:17:10Z

    Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> Thanks, pushed a slightly tweaked version.

?  No push visible from here ...

			regards, tom lane

  24. Re: BUG #13741: vacuumdb does not accept valid password

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2015-11-12T21:23:05Z

    Tom Lane wrote:
> Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> > Thanks, pushed a slightly tweaked version.
> 
> ?  No push visible from here ...

Uh, I had only made the dry-run one.  Did the real one now.

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  25. Re: BUG #13741: vacuumdb does not accept valid password

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2015-11-12T21:26:27Z

    Tom Lane wrote:

> NAK.  This changes the behavior of connectDatabase() for *all* users
> of that function, not only vacuumdb.  But the proposed behavioral
> change is only appropriate for calling programs in which only a single
> host/port/database target is used per execution.  In other contexts,
> reusing the prior password is not just inappropriate but could actually
> create security issues.  (It's possible that this behavior would be okay
> for all existing callers, but that doesn't mean we should put in a
> security gotcha for future uses.)
> 
> We could make this approach work if connectDatabase() remembered all
> the parameters internally, and only tried to reuse the password when
> they all match.  Or maybe it'd be better to alter the API so the caller
> can say whether to try to reuse a saved password or not.  But I'm not sure
> whether either of those answers is cleaner than the previous patch.

Thanks for the input.  I decided to push what we had because it's less
invasive in terms of API definition.  If we want to change in the
direction suggested by Masao-san, we can still do it, but perhaps only
in master -- maybe we would like to have both pg_dump and
src/bin/scripts compile a single source code file instead of having two
copies of essentially the same routine, for instance.

> (BTW, I notice that pg_dumpall.c has a version of connectDatabase in
> which the "static" trick is already being used, sans any documentation.
> That's okay for pg_dumpall, but might be an issue if anyone copies-and-
> pastes that version somewhere else ... and in any case it's fair to ask
> why that version hasn't been merged with common.c.)

We'd have to have a file common to both subdirs that only contains that
routine, I think.  We now have pg_dump/common.c as well ...  Not
something I want to propose for 9.5.

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  26. Re: BUG #13741: vacuumdb does not accept valid password

    Tom Lane <tgl@sss.pgh.pa.us> — 2015-11-12T21:51:22Z

    Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> Thanks for the input.  I decided to push what we had because it's less
> invasive in terms of API definition.

I dunno, this might be easier for the callers that don't want password
re-use, but it seems quite horrid for ones that do.  The changes to
vacuumdb.c are, frankly, seriously ugly; and they require vacuumdb.c
to know a lot more than before about password handling.

Other notes are that the strdup() call should surely be pg_strdup(),
and the mix of free() and pg_free() is at best unsightly.

On the whole I don't think this was ready to push.

The place I was thinking we might end up was something like Fujii-san's
patch plus a new bool parameter "allow_password_reuse", which could be
passed as false in cases where the old behavior seems preferable.

			regards, tom lane

  27. Re: BUG #13741: vacuumdb does not accept valid password

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2015-11-12T21:58:41Z

    Tom Lane wrote:
> Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> > Thanks for the input.  I decided to push what we had because it's less
> > invasive in terms of API definition.
> 
> I dunno, this might be easier for the callers that don't want password
> re-use, but it seems quite horrid for ones that do.  The changes to
> vacuumdb.c are, frankly, seriously ugly; and they require vacuumdb.c
> to know a lot more than before about password handling.

Yes, it is ugly.

> Other notes are that the strdup() call should surely be pg_strdup(),
> and the mix of free() and pg_free() is at best unsightly.

I had the same comment, but it turns out that free and strdup must be
used instead because sprompt.c uses malloc in the equivalent places; and
we can't use pg_malloc there because it's in src/port which isn't
allowed to use fe_memutils.  We would have to move it from src/port to
src/common first.

> The place I was thinking we might end up was something like Fujii-san's
> patch plus a new bool parameter "allow_password_reuse", which could be
> passed as false in cases where the old behavior seems preferable.

Hm, we can try that.

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  28. Re: BUG #13741: vacuumdb does not accept valid password

    Michael Paquier <michael.paquier@gmail.com> — 2015-11-14T13:39:28Z

    On Fri, Nov 13, 2015 at 6:58 AM, Alvaro Herrera wrote:
> Tom Lane wrote:
>> Alvaro Herrera <alvherre@2ndquadrant.com> writes:
>> The place I was thinking we might end up was something like Fujii-san's
>> patch plus a new bool parameter "allow_password_reuse", which could be
>> passed as false in cases where the old behavior seems preferable.
>
> Hm, we can try that.

That's neater. See for example the attached.
-- 
Michael

  29. Re: BUG #13741: vacuumdb does not accept valid password

    Haribabu Kommi <kommi.haribabu@gmail.com> — 2015-11-16T02:41:31Z

    On Sun, Nov 15, 2015 at 12:39 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Fri, Nov 13, 2015 at 6:58 AM, Alvaro Herrera wrote:
>> Tom Lane wrote:
>>> Alvaro Herrera <alvherre@2ndquadrant.com> writes:
>>> The place I was thinking we might end up was something like Fujii-san's
>>> patch plus a new bool parameter "allow_password_reuse", which could be
>>> passed as false in cases where the old behavior seems preferable.
>>
>> Hm, we can try that.
>
> That's neater. See for example the attached.

Yes, it is much cleaner than the previous version.
I reviewed and tested the same. It is working for all scenarios.

Regards,
Hari Babu
Fujitsu Australia

  30. Re: BUG #13741: vacuumdb does not accept valid password

    Michael Paquier <michael.paquier@gmail.com> — 2015-12-23T13:52:20Z

    On Mon, Nov 16, 2015 at 11:41 AM, Haribabu Kommi
<kommi.haribabu@gmail.com> wrote:
> On Sun, Nov 15, 2015 at 12:39 AM, Michael Paquier
> <michael.paquier@gmail.com> wrote:
>> On Fri, Nov 13, 2015 at 6:58 AM, Alvaro Herrera wrote:
>>> Tom Lane wrote:
>>>> Alvaro Herrera <alvherre@2ndquadrant.com> writes:
>>>> The place I was thinking we might end up was something like Fujii-san's
>>>> patch plus a new bool parameter "allow_password_reuse", which could be
>>>> passed as false in cases where the old behavior seems preferable.
>>>
>>> Hm, we can try that.
>>
>> That's neater. See for example the attached.
>
> Yes, it is much cleaner than the previous version.
> I reviewed and tested the same. It is working for all scenarios.

This patch has fallen in the void and it simplifies greatly vacuumdb.c
and its password-related routines. I have added it to the next CF to
not forget about it:
https://commitfest.postgresql.org/8/449/
-- 
Michael

  31. Re: BUG #13741: vacuumdb does not accept valid password

    Tom Lane <tgl@sss.pgh.pa.us> — 2015-12-23T16:44:53Z

    Michael Paquier <michael.paquier@gmail.com> writes:
> On Mon, Nov 16, 2015 at 11:41 AM, Haribabu Kommi
> <kommi.haribabu@gmail.com> wrote:
>> On Sun, Nov 15, 2015 at 12:39 AM, Michael Paquier
>> <michael.paquier@gmail.com> wrote:
>>> That's neater. See for example the attached.

>> Yes, it is much cleaner than the previous version.
>> I reviewed and tested the same. It is working for all scenarios.

> This patch has fallen in the void and it simplifies greatly vacuumdb.c
> and its password-related routines. I have added it to the next CF to
> not forget about it:

Sorry for having lost track of this.  I think if we're going to do this,
we should do it now not later, because it essentially reverts the
connectDatabase() API change made in 83dec5a71 in favor of a different API
change with the same purpose.  Now, that doesn't matter if no third-party
code is using connectDatabase(), but I have a suspicion that there
probably is some.  Any such users will not thank us for whacking
connectDatabase's API around in 9.5 and then whacking it around
differently in 9.6.

In short, I think we should apply this now and back-patch into 9.5,
like the prior patch, even though we're post-RC1.  The reduction in
cross-version API churn is worth it.

If no objections, I'll go do that shortly.

			regards, tom lane

  32. Re: BUG #13741: vacuumdb does not accept valid password

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2015-12-23T16:55:44Z

    Tom Lane wrote:

> Sorry for having lost track of this.  I think if we're going to do this,
> we should do it now not later, because it essentially reverts the
> connectDatabase() API change made in 83dec5a71 in favor of a different API
> change with the same purpose.  Now, that doesn't matter if no third-party
> code is using connectDatabase(), but I have a suspicion that there
> probably is some.  Any such users will not thank us for whacking
> connectDatabase's API around in 9.5 and then whacking it around
> differently in 9.6.
> 
> In short, I think we should apply this now and back-patch into 9.5,
> like the prior patch, even though we're post-RC1.  The reduction in
> cross-version API churn is worth it.
> 
> If no objections, I'll go do that shortly.

No objection here.

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  33. Re: [BUGS] BUG #13741: vacuumdb does not accept valid password

    Michael Paquier <michael.paquier@gmail.com> — 2015-12-24T00:21:46Z

    On Thu, Dec 24, 2015 at 1:55 AM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:
> Tom Lane wrote:
>
>> Sorry for having lost track of this.  I think if we're going to do this,
>> we should do it now not later, because it essentially reverts the
>> connectDatabase() API change made in 83dec5a71 in favor of a different API
>> change with the same purpose.  Now, that doesn't matter if no third-party
>> code is using connectDatabase(), but I have a suspicion that there
>> probably is some.  Any such users will not thank us for whacking
>> connectDatabase's API around in 9.5 and then whacking it around
>> differently in 9.6.
>>
>> In short, I think we should apply this now and back-patch into 9.5,
>> like the prior patch, even though we're post-RC1.  The reduction in
>> cross-version API churn is worth it.
>>
>> If no objections, I'll go do that shortly.
>
> No objection here.

Thanks for taking care of that!
-- 
Michael