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:
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