Thread: BUG #13741: vacuumdb does not accept valid password
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=#
______
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
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
Attachment
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
Attachment
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
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
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
Attachment
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
Attachment
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
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
Attachment
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
Attachment
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
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
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
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
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
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
Attachment
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
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
Attachment
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
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
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
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> Thanks, pushed a slightly tweaked version.
? No push visible from here ...
regards, tom lane
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
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
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
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
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
Attachment
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
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
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
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
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