Re: [PATCH] pgpassfile connection option

Fabien Coelho wrote:
> A few detailed comments:
>
> Beware of spacing:
>  . "if(" -> "if (" (2 instances)
>  . " =='\0')" -> " == '\0')" (at least one instance)
>
> Elsewhere:
>
>  + if (conn->pgpassfile_used && conn->password_needed && conn->result &&
>  +     conn->pgpassfile && conn->pgpassfile[0]!='\0' &&
>
> ISTM that if pgpassfile_used is true then pgpassfile is necessarily 
> defined, so the second line tests are redundant? Or am I missing 
> something? 
I've adressed those spacing errors.
You are right, if pgpassfile_used is true, it SHOULD be defined, I just 
like to be careful whenever I'm working with strings. But I guess in 
this scenario I can trust the caller and omit those checks.


On 11/22/2016 07:04 AM, Mithun Cy wrote:
>  > Independently of your patch, while testing I concluded that the 
> multi-host feature and documentation should be improved:
>  > - If a connection fails, it does not say for which host/port.
>
> Thanks I will re-test same.
>
> > In fact they are tried in turn if the network connection fails, but not
> > if the connection fails for some other reason such as the auth.
> > The documentation is not precise enough.
>
> If we fail due to authentication, then I think we should notify the 
> client instead of trying next host. I think it is expected genuine 
> user have right credentials for making the connection. So it's better 
> if we notify same to client immediately rather than silently ignoring 
> them. Otherwise the host node which the client failed to connect will 
> be permanently unavailable until client corrects itself. But I agree 
> documentation and error message as you said need improvements. I will 
> try to correct same
I agree with those criticisms of the multi-host feature and notifying 
the client in case of an authentification error rather than trying other 
hosts seems sensible to me.
But I think fixes for those should be part of different patches, as this 
patch's aim was only to expand the existing pgpassfile functionality to 
be used with a parameter.

regards,
Julian