Обсуждение: libpq parameter parsing problem

On 07/01/2020 15:17, Jobin Augustine wrote:
> Hello hackers,
> User can pass session-level settings as a parameter in the connection 
> string like:
> psql "host=localhost user=postgres options='-c synchronous_commit=off'"
> Which sets the synchronous_commit off for the session.
> 
> However, URI spec is not allowing it,
> psql postgresql://postgres@localhost:5432/postgres?options="-c 
> synchronous_commit=off"
> psql: error: could not connect to server: extra key/value separator "=" 
> in URI query parameter: "options"

The "-c synchronous_commit=off" string is part of the value for the 
"options" keyword. So it needs to be URI encoded:

psql 
"postgresql://postgres@localhost:5432/postgres?options=-c%20synchronous_commit%3Doff"

- Heikki

On 08/01/2020 10:48, Jobin Augustine wrote:
> Thanks Alex and Heikki,
> 
> Encoding works and it solved the problem. I find the documentation is 
> not very clear about it.
> Heikki's single line comment was more clear to me than documentation :)
> A statement like  "PostgreSQL URI string need to be encoded with 
> Percent-encoding <https://en.wikipedia.org/wiki/Percent-encoding> " 
> would have been much better

If you could propose a concrete patch to add something like that to the 
docs, that'd be great. I think adding an example connection URI like 
that in the docs would be really helpful.

- Heikki

On Wed, Jan 08, 2020 at 11:06:35AM +0200, Heikki Linnakangas wrote:
> If you could propose a concrete patch to add something like that to the
> docs, that'd be great. I think adding an example connection URI like that in
> the docs would be really helpful.

+1.
--
Michael

On Mon, Jan 13, 2020 at 08:07:06PM +0530, Jobin Augustine wrote:
> Sorry for the late reply. Attaching a patch.

>     <para>
> -    Percent-encoding may be used to include symbols with special meaning in any
> -    of the <acronym>URI</acronym> parts, e.g. replace <literal>=</literal> with
> -    <literal>%3D</literal>.
> -
> +    Connection <acronym>URI</acronym> need to be encoded with
> +    <ulink url="https://en.wikipedia.org/wiki/Percent-encoding">Percent-encoding</ulink>
> +    if it includes symbols with special meaning in any of its parts.
> +    For example:
> +<programlisting>
> +postgresql://postgres@localhost:5432/postgres?options=-c%20synchronous_commit%3Doff%20-c%20geqo%3Doff
> +</programlisting>
> +    where all <literal>=</literal> are replaced with <literal>%3D</literal> and
> +    space character with <literal>%20</literal>
>     </para>

The reference to wikipedia is nice to have.  A small nit from me is
that I would group the last sentence with the "For example", to give:
"Here is an example where all = are replaced.."

The first sentence sounds good to me.
--
Michael

On Tue, Jan 14, 2020 at 12:01:43PM +0530, Jobin Augustine wrote:
> Yes, agree, readability is better with that modification.
> Attaching a modified patch.

(Please always top-post, this is the style of the PostgreSQL community
mailing lists).  The new patch looks good to me, let's see if others
have extra thoughts to share.  If not, I'll try to commit it.
--
Michael

On 2020-Jan-14, Michael Paquier wrote:

> On Tue, Jan 14, 2020 at 12:01:43PM +0530, Jobin Augustine wrote:
> > Yes, agree, readability is better with that modification.
> > Attaching a modified patch.
> 
> (Please always top-post, this is the style of the PostgreSQL community
> mailing lists).  The new patch looks good to me, let's see if others
> have extra thoughts to share.  If not, I'll try to commit it.

The thought I had when I first saw this was that it would be better to
have a minimal, coherent set of useful examples all together in a
subsection called Examples.  But then I realized that random examples
are already scattered here and there in the libpq page, so while I still
think that that would be better, I no longer think it's this patch's
responsibility to make it so.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Tue, Jan 14, 2020 at 06:52:07PM -0700, David G. Johnston wrote:
> I would probably choose to move the example for the options parameters to
> the "Parameter Key Words" options section:

I think that this would be inconsistent with the rest, as that's a URI
and all the other examples are there.  I agree with the feeling of
Alvaro upthread that we could do a better effort with the handling of
the examples in this section, but it is quite unclear to me if that
would actually bring more clarity to the whole, and that's not really
the job of this patch.

> Also, regardless of where it is placed having both the username and
> database both be named "postgres" in an example just adds unnecessary
> mental effort to understanding the example.  One that none of the existing
> examples do.  Name your user "user" and database "mydb" unless, as with the
> desire to include a space, there is a meaningful reason not to.

Good point.  Using "mydb" or "user" instead of "postgres" in the new
example would be less confusing.  Another question, would be it better
to use "5433" instead of "5432" for the port number.  That's a nit,
but as we are on that stuff let's be right..
--
Michael

On Tue, Jan 14, 2020 at 08:54:41PM -0700, David G. Johnston wrote:
> My rationale is more since none of the other options have structural parts
> that require escaping, and rarely do the values themselves require
> escaping, that tossing that single example for a seldom-used option into
> the middle of the "usage examples" section doesn't really fit.  What the
> example does is clarify a specific combination of factors, URI and
> "options", that require special attention.  I'd rather bury that special
> case in the documentation for options then explain it in detail in the
> generic URI section - the structural elements involved are already
> mentioned in the options section and this just clarifies how they are
> written in the URI situation.  Its not a strong opinion but I don't think
> adding it there while leaving the other common compound usage examples as a
> whole above is a misplacement - "options" is special and can very well have
> special treatment.  It will be found by those that need to know about it.

Fair point.  Now, replacing a special character applies to more than
"options", because it can be used for any values.  For example:
postgresql:///mydb?host=localhost&application_name=hoge%20%3D%20foo
(This generates "hoge = foo" as application_name as you can guess.)

And the part of the docs for connection URIs describes only how to
percent-encode a path.
--
Michael

On 2020-01-14 07:03, Michael Paquier wrote:
>>      <para>
>> -    Percent-encoding may be used to include symbols with special meaning in any
>> -    of the <acronym>URI</acronym> parts, e.g. replace <literal>=</literal> with
>> -    <literal>%3D</literal>.
>> -
>> +    Connection <acronym>URI</acronym> need to be encoded with
>> +    <ulink url="https://en.wikipedia.org/wiki/Percent-encoding">Percent-encoding</ulink>
>> +    if it includes symbols with special meaning in any of its parts.
>> +    For example:
>> +<programlisting>
>> +postgresql://postgres@localhost:5432/postgres?options=-c%20synchronous_commit%3Doff%20-c%20geqo%3Doff
>> +</programlisting>
>> +    where all <literal>=</literal> are replaced with <literal>%3D</literal> and
>> +    space character with <literal>%20</literal>
>>      </para>
> 
> The reference to wikipedia is nice to have.

Let's please not put links to Wikipedia into the main body of the 
documentation.  If people want to look up something, they know where to 
find it.

Links to authoritative sources (perhaps an RFC in this case) would be 
better.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Wed, Jan 15, 2020 at 04:42:28PM +0100, Peter Eisentraut wrote:
> Let's please not put links to Wikipedia into the main body of the
> documentation.  If people want to look up something, they know where to find
> it.

That's not really a new concept:
$ cd doc/ && git grep wikipedia\.org | wc -l
55

> Links to authoritative sources (perhaps an RFC in this case) would be
> better.

No objections to your suggestion in this case though.  I can see
Section 2.1 from RFC3986 for that:
https://tools.ietf.org/html/rfc3986#section-2.1

So, gathering all the feedback and my own tweaks, I finish with the
attached (merely simplified).
--
Michael

On Thu, Jan 16, 2020 at 09:17:23AM +0100, Oleksandr Shulgin wrote:
> At the same time, we could still be more specific if we would say
> "delimiters" instead of the generic "special meaning".  Should we then
> provide an exhaustive list of delimiters or is it clear enough like that?
> For example, the whitespace doesn't need to be percent-encoded (it doesn't
> hurt as you might be able to spare the quoting if using it as an argument
> to a shell command), while the "equal sign", when used in the query string
> part, does need encoding.

This term comes from you, as of 2d612ab, and that does not look like
something to change because reserved characters have "sometimes" a
special meaning in this context.  A reference to the RFC is sufficient
IMO, readers could always look at that reference for a precise list
and that would bloat the docs more than necessary.
--
Michael

On Fri, Jan 17, 2020 at 09:05:27AM +0100, Oleksandr Shulgin wrote:
> Yeah, the fact that a delimiter in one context may perfectly be ignored in
> the other is what makes it tricky to describe, with a non-zero likelihood
> of making it more confusing. :-/
>
> Fine by me to keep it like that, though I'm obviously biased as I never had
> this question in the first place.

OK, and committed.
--
Michael