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