|From:||Julian Markwort <julian(dot)markwort(at)uni-muenster(dot)de>|
|To:||Mithun Cy <mithun(dot)cy(at)enterprisedb(dot)com>, Fabien COELHO <coelho(at)cri(dot)ensmp(dot)fr>|
|Cc:||PostgreSQL Developers <pgsql-hackers(at)postgresql(dot)org>|
|Subject:||Re: [PATCH] pgpassfile connection option|
|Views:||Raw Message | Whole Thread | Download mbox | Resend email|
Fabien Coelho wrote:
> A few detailed comments:
> Beware of spacing:
> . "if(" -> "if (" (2 instances)
> . " =='\0')" -> " == '\0')" (at least one instance)
> + if (conn->pgpassfile_used && conn->password_needed && conn->result &&
> + conn->pgpassfile && conn->pgpassfile!='\0' &&
> ISTM that if pgpassfile_used is true then pgpassfile is necessarily
> defined, so the second line tests are redundant? Or am I missing
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.
|Next Message||Paul Ramsey||2016-11-28 14:33:13||Re: User-defined Operator Pushdown and Collations|
|Previous Message||Pavel Stehule||2016-11-28 13:20:53||Re: Tackling JsonPath support|