Re: Ltree syntax improvement

On 25.03.2020 2:08, Tom Lane wrote:

Nikita Glukhov <n.gluhov@postgrespro.ru> writes:

Attached new version of the patch.

I spent a little bit of time looking through this, and have a few
comments:

* You have a lot of places where tests for particular ASCII characters
are done like this:
if ((charlen == 1) && t_iseq(src, '\\'))

This is a tedious and expensive way to spell
if (*src == '\\')

because charlen is necessarily 1 if you are looking at an ASCII character;
there is no allowed backend encoding in which an ASCII character can be
the first byte of a multibyte character.  Aside from the direct
simplifications of the tests that this makes possible, I see some places
where you'd not have to pass charlen around, either.

All unnecessary checks of charlen were removed, but t_iseq() were left for 
consistency.

* I spent a fair amount of time thinking that a lot of the added code
was wrong because it was only considering escaping and not
double-quoting.  I eventually concluded that the idea is to convert
double-quoting to a pure escape-based representation during input
and store it that way.  However, I don't really see why that is either
necessary or a good idea --- the internal storage already has a length
counter for each label.  So I think what you really ought to be doing
here is simplifying out both quotes and escapes during ltree_in
and just storing the notionally-represented string internally.
(If I've misunderstood what the plan is, well the utter lack of
documentation in the patch isn't helping.)

ltree_in() removes quotes and escapes before storing strings (see 
copy_unescaped()), just as you suggest.

ltree_out() adds escapes and quotes if necessary (see copy_escaped(), 
extra_bytes_for_escaping()).

I have refactored code a bit, removed duplicated code, fixed several 
bugs in reallocation of output strings, and added some comments.

* The added test cases seem a bit excessive and repetitive.

I have removed some tests that have become redundant after changes in
parsing.