Re: [PATCH] pgpassfile connection option

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
Date: 2016-11-28 14:15:31
Message-ID: 6bef8549-aeb4-b97f-8a21-5477f486eb23@uni-muenster.de
Views: Raw Message | Whole Thread | Download mbox
Thread:
Lists: 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.

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

Attachment Content-Type Size
pgpassfile_v5.patch text/x-patch 9.2 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
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