Re: Ltree syntax improvement

Hi!

I have looked at the patch and found some problems.

1. I fixed some bugs (fixed patch with additional test cases is attached):

-- NULL 'lptr' pointer dereference at lquery_in()
=# SELECT '*'::lquery;
-- crash

-- '|' after '*{n}' is wrongly handled (LQPRS_WAITEND state)
=# SELECT '*{1}|2'::lquery;
WARNING: problem in alloc set MessageContext: req size > alloc size for chunk 0x1c2d508 in block 0x1c2bc58
WARNING: problem in alloc set MessageContext: req size > alloc size for chunk 0x1c2d508 in block 0x1c2bc58
lquery
-------------
*{1}.*{,48}
(1 row)

-- wrong handling of trailing whitespace
=# SELECT '"a" '::ltree;
ERROR: name of level is empty
LINE 1: SELECT '"a" '::ltree;
^
DETAIL: Name length is 0 in position 4.

=# SELECT '"a" '::lquery;
ERROR: name of level is empty
LINE 1: SELECT '"a" '::lquery;
^
DETAIL: Name length is 0 in position 4.

-- backslashes are not escaped in ltree_out()/lquery_out(),
-- which is not consistent with ltree_in()/lquery_in()
=# SELECT '"\\"'::ltree;
ltree
-------
"\"
(1 row)

=# SELECT '"\\"'::lquery;
lquery
--------
"\"
(1 row)

=# SELECT '"\\"'::ltree::text::ltree;
ERROR: syntax error
DETAIL: Unexpected end of line.

=# SELECT '"\\"'::lquery::text::lquery;
ERROR: syntax error
DETAIL: Unexpected end of line.

2. There are inconsistencies in whitespace handling before and after *, @, %, {}
(I have not fixed this because I'm not sure how it is supposed to work):

-- whitespace before '*' is not ignored
=# SELECT '"a" *'::lquery;
lquery
--------
"a\""*
(1 row)

=# SELECT 'a *'::lquery;
lquery
--------
"a "*
(1 row)

-- whitespace after '*' and '{}' is disallowed
=# SELECT 'a* .b'::lquery;
ERROR: syntax error at position 2
LINE 1: SELECT 'a* .b'::lquery;
^

=# SELECT 'a* |b'::lquery;
ERROR: syntax error at position 2
LINE 1: SELECT 'a* |b'::lquery;
^

=# SELECT '*{1} .a'::lquery;
ERROR: syntax error at position 4
LINE 1: SELECT '*{1} .a'::lquery;
^

-- but the whitespace after levels without '*(at)%{}' is allowed
=# SELECT 'a |b'::lquery;
lquery
--------
a|b
(1 row)

3. Empty level names between '!' and '|' are allowed. This behavior can be
seen on master, so it seems that we cannot fix it now:

-- master
=# SELECT '!|a'::lquery;
lquery
--------
!|a
(1 row)

-- patched
=# SELECT '!|a'::lquery;
lquery
--------
!""|a
(1 row)

-- empty level names in other places are disallowed
=# SELECT '!a|'::lquery;
ERROR: syntax error
LINE 1: SELECT '!a|'::lquery;
^
DETAIL: Unexpected end of line.

=# SELECT '|a'::lquery;
ERROR: syntax error at position 0
LINE 1: SELECT '|a'::lquery;
^

4. It looks strange to me that leading and trailing unquoted whitespace is
ignored, but the internal whitespace is treated like a quoted:

=# SELECT ' a b . c d '::lquery;
lquery
-----------------
"a b"."c d"
(1 row)

I would prefer unquoted unescaped whitespace to be a delimiter always.

5. It seems wrong to me that ltree and lquery have different special character
sets now. This leads to the fact that arbitrary ltree text cannot be used
directly as lquery text, as it seemed to be before the syntax improvements:

=# SELECT 'a|b'::ltree::text::lquery;
lquery
--------
a|b
(1 row)

=# SELECT '"a|b"'::ltree::text::lquery;
lquery
--------
a|b
(1 row)

=# SELECT '"a|b"'::lquery;
lquery
--------
"a|b"
(1 row)

There might not be a problem if we had ltree::lquery cast.

Also I think that text[]::ltree/ltree::text[] casts for ltree
construction/deconstruction from text level names can be very useful.

6. ltree and escpecially lquery parsing code still look too complicated for me,
and I believe that the bugs described above are a direct consequence of this.
So the code needs to be refactored, maybe even without using of state machines.

On 11.07.2019 20:49, Dmitry Belyavsky wrote:
>
> On Thu, Jul 11, 2019 at 11:20 AM Thomas Munro <thomas(dot)munro(at)gmail(dot)com
> <mailto:thomas(dot)munro(at)gmail(dot)com>> wrote:
>
> On Wed, Jul 10, 2019 at 7:40 AM Dmitry Belyavsky
> <beldmit(at)gmail(dot)com <mailto:beldmit(at)gmail(dot)com>> wrote:
> > [ltree_20190709.diff]
>
> You need to update contrib/ltree_plpython/expected/ltree_plpython.out,
> otherwise check-world fails when built with Python support. The good
> news is that it looks like it fails because you fixed something!
> (Though I didn't check the details).
>
>  CREATE FUNCTION test2() RETURNS ltree
>  LANGUAGE plpythonu
>  TRANSFORM FOR TYPE ltree
>  AS $$
>  return ['foo', 'bar', 'baz']
>  $$;
>  -- plpython to ltree is not yet implemented, so this will fail,
>  -- because it will try to parse the Python list as an ltree input
>  -- string.
>  SELECT test2();
> -ERROR:  syntax error at position 0
> -CONTEXT:  while creating return value
> -PL/Python function "test2"
> +          test2
> +-------------------------
> + "['foo', 'bar', 'baz']"
> +(1 row)
> +
>
>
> See attached. I'm not familiar enough with python so I just removed
> the failing tests.
> If the main patch is accepted, the ltree_python extension should be
> redesigned, I think...
>
7. ltree_plpython test does not fail now because Python list is converted to a
text and then to a ltree, and the textual representation of a Python list has
become a valid ltree text:

SELECT $$['foo', 'bar', 'baz']$$::ltree;
ltree
-------------------------
"['foo', 'bar', 'baz']"
(1 row)

So Python lists can be now successfully converted to ltrees without a transform,
but the result is not that is expected ('foo.bar.baz'::ltree).

--
Nikita Glukhov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company