Re: PQHost() undefined behavior if connecting string contains both host and hostaddr types

From: "David G(dot) Johnston" <david(dot)g(dot)johnston(at)gmail(dot)com>
To: Michael Paquier <michael(at)paquier(dot)xyz>
Cc: Haribabu Kommi <kommi(dot)haribabu(at)gmail(dot)com>, Kyotaro HORIGUCHI <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp>, Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com>, Pg Hackers <pgsql-hackers(at)postgresql(dot)org>, Michael Paquier <michael(dot)paquier(at)gmail(dot)com>, Robert Haas <robertmhaas(at)gmail(dot)com>
Subject: Re: PQHost() undefined behavior if connecting string contains both host and hostaddr types
Date: 2018-03-27 04:03:17
Message-ID: CAKFQuwYZyw6gYEN2Ki6MW_jhLE4Lb+wXYRVwzrWeT4H-wfM_4w@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On Mon, Mar 26, 2018 at 8:24 PM, Michael Paquier <michael(at)paquier(dot)xyz>
wrote:

> On Tue, Mar 27, 2018 at 11:43:27AM +1100, Haribabu Kommi wrote:
> > Patch attached with the above behavior along with other comments from
> > upthread.
>
> Thanks for the updated version.
>
> The function changes look logically good to me.
>
> + <para>
> + The <function>PQhost</function> function returns NULL when the
> + input conn parameter is NULL or an empty string if conn cannot be
> evaluated.
> + Applications of this function must carefully evaluate the return
> value.
> + </para>
>
> - "Applications of this function must carefully evaluate the return
> value" is rather vague, so I would append to the end "depending on the
> state of the connection involved."
> The same applies to PQport() for consistency.
>
> Perhaps the documentation should mention as well that making the
> difference between those different values is particularly relevant when
> using multiple-value strings? I would rather add one paragraph on the
> matter than nothing. I really think that we have been lacking clarity
> in the documentation for those APIs for too long, and people always
> argue about what they should do. If we have a base documented, then we
> can more easily argue for the future as well, and things are clear to
> the user.
>

"depending on the state of the connection" doesn't move the goal-posts that
far though...and "Applications of this function" would be better written as
"Callers of this function" if left in place.

In any case something like the following framework seems more useful to the
reader who doesn't want to scan the source code for the PQhost/PQport
functions.

The PQhost function returns NULL when the input conn parameter is NULL or
an empty string if conn cannot be evaluated. Otherwise, the return value
depends on the state of the conn: specifically (translate code to
documentation here). Furthermore, if both host and hostaddr properties
exist on conn the return value will contain only the host.

I'm undecided on the need for a <note> element but would lean against it in
favor of the above, slightly longer, paragraph.

And yes, while I'm not sure right now what the multi-value condition logic
results in it should be mentioned - at least if the goal of the docs is to
be a sufficient resource for using these functions. In particular what
happens when the connection is inactive (unless that falls under "cannot be
evaluated"...).

David J.

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Pavel Stehule 2018-03-27 04:10:59 Re: idea - custom menu
Previous Message Thomas Munro 2018-03-27 04:01:36 Re: [HACKERS] pg_serial early wraparound