From: Nikolay Shaplov <dhyan@nataraj.su>
To: pgsql-hackers@lists.postgresql.org
Cc: Dmitry Belyavsky <beldmit@gmail.com>
Subject: Re: Ltree syntax improvement
Date: Tue, 05 Feb 2019 16:26:17 +0300
Message-ID: <30381329.7fST72pxSh@x200m>
User-Agent: KMail/5.2.3 (Linux/4.9.0-8-amd64; KDE/5.28.0; x86_64; ; )
In-Reply-To: 
 <CADqLbzLVkBuPX0812o+z=c3i6honszsZZ6VQOSKR3VPbB56P3w@mail.gmail.com>
References: 
 <CADqLbzLVkBuPX0812o+z=c3i6honszsZZ6VQOSKR3VPbB56P3w@mail.gmail.com>
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

=D0=92 =D0=BF=D0=B8=D1=81=D1=8C=D0=BC=D0=B5 =D0=BE=D1=82 =D0=B2=D1=82=D0=BE=
=D1=80=D0=BD=D0=B8=D0=BA, 29 =D1=8F=D0=BD=D0=B2=D0=B0=D1=80=D1=8F 2019 =D0=
=B3. 20:43:07 MSK =D0=BF=D0=BE=D0=BB=D1=8C=D0=B7=D0=BE=D0=B2=D0=B0=D1=82=D0=
=B5=D0=BB=D1=8C Dmitry=20
Belyavsky =D0=BD=D0=B0=D0=BF=D0=B8=D1=81=D0=B0=D0=BB:

> Please find attached the patch extending the sets of symbols allowed in
> ltree labels. The patch introduces 2 variants of escaping symbols, via
> backslashing separate symbols and via quoting the labels in whole for
> ltree, lquery and ltxtquery datatypes.

Hi!

Let's me join the review process...

I've just give a superficial look to the patch, read it through, and try  h=
ere=20
and there, so first I'll give feedback about exterior features of the patch.

So, the first question: why do we need all this? The answer seems to be=20
obvious, but it would be good say it explicitly. What problems would it sol=
ve?=20
Do these problems really exists?=20

The second question is about coding style. As far as I can see you've passe=
d=20
your code through pg_indent, but it does not solve all problems.

As far as I know, postgres commiters are quite strict about code width, the=
=20
code should not be wider that 80 col, except places were string constants a=
re=20
introduced (There you can legally exceed 80 col). So I would advice to use =
vim=20
with  tabstop=3D4  and colorcolumn=3D80 and make sure that new code text do=
es not=20
cross red vertical line.=20

Comments. There is no fixed coding style for comments in postgres. Usually =
this=20
means that it is better to follow coding in the code around. But ltree does=
=20
not have much comments, so I would suggest to use the best style I've seen =
in=20
the code
/*
 * [function name]
 * < tab ><tab> [Short description, what does it do]
 *=20
 * [long explanation how it works, several subparagraph if needed
 */
(Seen this in src/include/utils/rel.h)
or something like that.
At least it would be good to have explicit separation between descriptions =
and=20
explanation of how it works.

And about comment
/*
 * Function calculating bytes to escape
 * to_escape is an array of "special" 1-byte symbols
 * Behvaiour:
 * If there is no "special" symbols, return 0
 * If there are any special symbol, we need initial and final quote, so ret=
urn=20
2
 * If there are any quotes, we need to escape all of them and also initial =
and=20
final quote, so
 * return 2 + number of quotes
 */

It has some formatting. But it should not.  As far as I understand this=20
comment should be treated as single paragraph by reformatter, and refomatte=
d.=20
I do not understand why pg_indent have not done it. Documentation (https://
www.postgresql.org/docs/11/source-format.html) claims that if you want to=20
avoid reformatting, you should add "-------------" at the beginning and at =
the=20
end of the comment. So it would be good either remove this formatting, or a=
dd=20
"----" if you are sure you want to keep it.

Third part is about error messages.
=46irst I've seen errors like elog(ERROR, "internal error during splitting=
=20
levels");. I've never seen error like that in postgres. May be if this erro=
r=20
is in the part of the code, that should never be reached in real live, may =
be=20
it would be good to change it to Assert? Or if this code can be normally=20
reached, add some more explanation of what had happened...

About other error messages.

There are messages like=20
errmsg("syntax error"),
errdetail("Unexpected end of line.")));

or

errmsg("escaping syntax error")));


In postgres there is error message style guide=20
https://www.postgresql.org/docs/11/error-style-guide.html

Here I would expect messages like that

Primary:    Error parsing ltree path
Detail:     Curly quote is opened and not closed

Or something like that, I am not expert in English, but this way it would b=
e=20
better for sure. Key points are: Primary message should point to general ar=
ea=20
where error occurred (there can be a large SQL with a lot of things in it, =
so=20
it is good to point that problem is with ltree staff). And is is better to=
=20
word from the point of view of a user. What for you (and me) is unexpected =
end=20
of line, for him it is unclosed curly quote.=20

Also I am not sure, if it is easy, but if it is possible, it would be good =
to=20
move "^" symbol that points to the place of the error, to the place inside =
' '=20
where unclosed curly quote is located. As a user I would appreciate that.

So that's what I've seen while giving it superficial look...