Re: WIP: Fix parallel workers connection bug in pg_dump (Bug #13727)

On Nov 1, 2015 5:04 PM, "Marko Tiikkaja" <marko(at)joh(dot)to> wrote:
>
> On 10/25/15 10:55 PM, Zeus Kronion wrote:
>>
>> Parallel workers were failing to connect to the database when running
>> pg_dump with a connection string. The first of the following two commands
>> runs without errors, while the second one fails:
>> pg_dump "postgres://my-user:my-password(at)my(dot)hostname(dot)com:5432/my-db" -Fd
-f
>> my-dump
>> pg_dump "postgres://my-user:my-password(at)my(dot)hostname(dot)com:5432/my-db" -Fd
>> --jobs=9 -f my-dump
>>
>> The error message:
>> pg_dump: [parallel archiver] connection to database "my-db" failed:
>> fe_sendauth: no password supplied
>>
>> The password is not being stored correctly in the PGconn object when
>> connecting with a connection string.
>
>
> Yeah, the current code is definitely broken for this case. However, I
don't feel like this patch is quite there yet, either. _connectDB has
similar logic in it which might be hit in case e.g. a a user's HBA is
changed from a non-password-requiring method to a password-requiring one
after the one or more connections has been initiated. That one needs
changing as well.

I wasn't aware of that case. Should be an easy fix to make this weekend.

> However, I don't quite like the way the password cache is kept up to date
in the old *or* the new code. It seems to me that it should instead look
like:
>
> if (PQconnectionUsedPassword(AH->connection))
> AH->savedPassword = PQpass(AH->connection);
>
> What do you think?

I don't understand why this logic is preferable. Is your concern that
AH->savedPassword may contain a password even when none is needed? Or is
the change simply meant to give the reader a better sense of what is
actually going on?

-CS