Re: [PATCH] pgpassfile connection option - Mailing list pgsql-hackers

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.

 > 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