Обсуждение: [PATCH] ltree, lquery, and ltxtquery binary protocol support

Hi,

Attached is a patch to support send/recv on ltree, lquery and ltxtquery.
I'm a contributor to the Npgsql .NET PostgreSQL driver and we'll be
the first to have official support once those ltree changes have been
released.
You can find the driver support work here, the tests verify a
roundtrip for each of the types is succesful.
https://github.com/NinoFloris/npgsql/tree/label-tree-support

Any feedback would be highly appreciated.

Greetings,
Nino Floris

Hi!

On Sun, Aug 18, 2019 at 6:53 PM Nino Floris <mail@ninofloris.com> wrote:
> Attached is a patch to support send/recv on ltree, lquery and ltxtquery.
> I'm a contributor to the Npgsql .NET PostgreSQL driver and we'll be
> the first to have official support once those ltree changes have been
> released.
> You can find the driver support work here, the tests verify a
> roundtrip for each of the types is succesful.
> https://github.com/NinoFloris/npgsql/tree/label-tree-support

Thank you for the patch.

My notes are following:
 * Your patch doesn't follow coding convention.  In particular, it
uses spaces for indentation instead of tabs.  Moreover, it changes
tags to spaces in places it doesn't touch.  As the result, patch is
"dirty".  Looks like your IDE isn't properly setup.  Please check your
patch doesn't contain unintended changes and follow coding convention.
It's also good practice to run pgindent over changed files before
sending patch.
 * We currently don't add new extension SQL-script for new extension
version (i.e. ltree--1.2.sql).  Instead, we provide just a migration
script (i.e. ltree--1.1--1.2.sql).  This simplifies testing of
extension migration – plain extension creation implies migration.
 * What is motivation for binary format for lquery and ltxtquery?  One
could transfer large datasets of ltree's.  But is it so for lquery and
ltxtquery, which are used just for querying.
 * Just send binary representation of datatype is not OK.  PostgreSQL
supports a lot of platforms with different byte ordering, alignment
and so on.  You basically need to send each particular field using
pq_send*() function.

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Hi Alexander,

Thanks for the initial review!

> * Your patch doesn't follow coding convention.  In particular, it
> uses spaces for indentation instead of tabs.  Moreover, it changes
> tags to spaces in places it doesn't touch.  As the result, patch is
> "dirty".  Looks like your IDE isn't properly setup.  Please check your
> patch doesn't contain unintended changes and follow coding convention.
> It's also good practice to run pgindent over changed files before
> sending patch.

Apologies, will fix, I indeed haven't seen the requirement being tabs.
Though not to sound pedantic I would expect an .editorconfig for such
(industry) non standard indenting as it's broadly supported and very
little effort to do so.
I will run pgindent, thanks for the pointer, that looks super useful.

 > * We currently don't add new extension SQL-script for new extension
> version (i.e. ltree--1.2.sql).  Instead, we provide just a migration
> script (i.e. ltree--1.1--1.2.sql).  This simplifies testing of
> extension migration – plain extension creation implies migration.

I wasn't sure how to add new methods to the type without doing a full
CREATE TYPE, as ALTER TYPE doesn't allow updates to functions. At that
point wouldn't it be better as a new version?
I checked some other extensions like hstore to find reference material
how to do a new CREATE TYPE, all did a full version bump.
Should I just do a DROP and CREATE instead in a migration?

>  * What is motivation for binary format for lquery and ltxtquery?  One
could transfer large datasets of ltree's.  But is it so for lquery and
ltxtquery, which are used just for querying.

Completeness, Npgsql (and other drivers like Ecto from Elixir and
probably many others as well) can't do any text fallback in the binary
protocol without manual configuration.
This is because these drivers don't know much (or anything) about the
SQL they send so it wouldn't know to apply it for which columns.
I believe there has been a proposal at some point to enhance the
binary protocol to additionally allow clients to specify text/binary
per data type instead of per column.
That would allow all these drivers to automate this, but I think it
never went anywhere.
As it stands it's not ergonomic to ask developers to setup this
metadata per query they write.

 * Just send binary representation of datatype is not OK.  PostgreSQL
supports a lot of platforms with different byte ordering, alignment
and so on.  You basically need to send each particular field using
pq_send*() function.

Oh my, I don't think I did? I copied what jsonb is doing,
specifically, sending the textual representation of the data type with
a version field prefixed.
(It is why I introduced deparse_ltree/lquery, to take the respective
structure and create a null terminated string of its textual form)
So there are no other fields besides version and the string
representation of the structure.
ltree, lquery, and ltxtquery all seem to have a lossless and compact
textual interpretation.
My motivation here has been "if it's good enough for jsonb it should
be good enough for a niche extension like ltree" especially as having
any binary support is better than not having it at all.
I can change it to anything you'd like to see instead but I would need
some pointers as to what that would be.

Again, thank you for taking up this patch to review.

Best regards,
Nino Floris

On Mon, Sep 9, 2019 at 11:38 PM Alexander Korotkov
<a.korotkov@postgrespro.ru> wrote:
>
> Hi!
>
> On Sun, Aug 18, 2019 at 6:53 PM Nino Floris <mail@ninofloris.com> wrote:
> > Attached is a patch to support send/recv on ltree, lquery and ltxtquery.
> > I'm a contributor to the Npgsql .NET PostgreSQL driver and we'll be
> > the first to have official support once those ltree changes have been
> > released.
> > You can find the driver support work here, the tests verify a
> > roundtrip for each of the types is succesful.
> > https://github.com/NinoFloris/npgsql/tree/label-tree-support
>
> Thank you for the patch.
>
> My notes are following:
>  * Your patch doesn't follow coding convention.  In particular, it
> uses spaces for indentation instead of tabs.  Moreover, it changes
> tags to spaces in places it doesn't touch.  As the result, patch is
> "dirty".  Looks like your IDE isn't properly setup.  Please check your
> patch doesn't contain unintended changes and follow coding convention.
> It's also good practice to run pgindent over changed files before
> sending patch.
>  * We currently don't add new extension SQL-script for new extension
> version (i.e. ltree--1.2.sql).  Instead, we provide just a migration
> script (i.e. ltree--1.1--1.2.sql).  This simplifies testing of
> extension migration – plain extension creation implies migration.
>  * What is motivation for binary format for lquery and ltxtquery?  One
> could transfer large datasets of ltree's.  But is it so for lquery and
> ltxtquery, which are used just for querying.
>  * Just send binary representation of datatype is not OK.  PostgreSQL
> supports a lot of platforms with different byte ordering, alignment
> and so on.  You basically need to send each particular field using
> pq_send*() function.
>
> ------
> Alexander Korotkov
> Postgres Professional: http://www.postgrespro.com
> The Russian Postgres Company

On Wed, Sep 11, 2019 at 9:45 PM Nino Floris <mail@ninofloris.com> wrote:
>  > * We currently don't add new extension SQL-script for new extension
> > version (i.e. ltree--1.2.sql).  Instead, we provide just a migration
> > script (i.e. ltree--1.1--1.2.sql).  This simplifies testing of
> > extension migration – plain extension creation implies migration.
>
> I wasn't sure how to add new methods to the type without doing a full
> CREATE TYPE, as ALTER TYPE doesn't allow updates to functions. At that
> point wouldn't it be better as a new version?
> I checked some other extensions like hstore to find reference material
> how to do a new CREATE TYPE, all did a full version bump.
> Should I just do a DROP and CREATE instead in a migration?

Oh, I appears we didn't add send/recv to any pluggable datatype one we
get extension system.

Ideally we need to adjust ALTER TYPE command so that it could add
send/recv functions to an existing type.

I see couple other options:
1) Write migration script, which directly updates pg_type.
2) Add ltree--1.2.sql and advise users to recreate extension to get
binary protocol support.
But both these options are cumbersome.

What do you think about writing patch for ALTER TYPE?

> >  * What is motivation for binary format for lquery and ltxtquery?  One
> could transfer large datasets of ltree's.  But is it so for lquery and
> ltxtquery, which are used just for querying.
>
> Completeness, Npgsql (and other drivers like Ecto from Elixir and
> probably many others as well) can't do any text fallback in the binary
> protocol without manual configuration.
> This is because these drivers don't know much (or anything) about the
> SQL they send so it wouldn't know to apply it for which columns.
> I believe there has been a proposal at some point to enhance the
> binary protocol to additionally allow clients to specify text/binary
> per data type instead of per column.
> That would allow all these drivers to automate this, but I think it
> never went anywhere.
> As it stands it's not ergonomic to ask developers to setup this
> metadata per query they write.
>
>  * Just send binary representation of datatype is not OK.  PostgreSQL
> supports a lot of platforms with different byte ordering, alignment
> and so on.  You basically need to send each particular field using
> pq_send*() function.
>
> Oh my, I don't think I did? I copied what jsonb is doing,
> specifically, sending the textual representation of the data type with
> a version field prefixed.
> (It is why I introduced deparse_ltree/lquery, to take the respective
> structure and create a null terminated string of its textual form)
> So there are no other fields besides version and the string
> representation of the structure.
> ltree, lquery, and ltxtquery all seem to have a lossless and compact
> textual interpretation.
> My motivation here has been "if it's good enough for jsonb it should
> be good enough for a niche extension like ltree" especially as having
> any binary support is better than not having it at all.
> I can change it to anything you'd like to see instead but I would need
> some pointers as to what that would be.

Oh, sorry.  I didn't notice you send textual representation for ltree,
lquery, and ltxtquery.  And the point is not performance for these
datatypes, but completeness.  Now I got it, then it looks OK.

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Hello, can you please post an updated version of this patch?  Note that
Travis has a small complaint:

gcc -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Werror=vla -Wendif-labels
-Wmissing-format-attribute-Wformat-security -fno-strict-aliasing -fwrapv -fexcess-precision=standard -g -O2 -Wall
-Werror-fPIC -DLOWER_NODE -I. -I. -I../../src/include -I/usr/include/x86_64-linux-gnu -D_GNU_SOURCE   -c -o crc32.o
crc32.c
crc32.c: In function ‘ltree_crc32_sz’:
crc32.c:26:12: error: initialization discards ‘const’ qualifier from pointer target type
[-Werror=discarded-qualifiers]
  char    *p = buf;
            ^
cc1: all warnings being treated as errors
<builtin>: recipe for target 'crc32.o' failed
make[4]: *** [crc32.o] Error 1
make[4]: Leaving directory '/home/travis/build/postgresql-cfbot/postgresql/contrib/ltree'

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

Alright, as usual life got in the way. I've attached a new version of
the patch with pgindent changes.

> What do you think about writing patch for ALTER TYPE?
I'd rather not :$

> 1) Write migration script, which directly updates pg_type.
This sounds like the best option right now, if we don't want people to
do manual migrations to ltree 1.2.
How would I best go at this?

> Travis has a small complaint:
Should be fixed!

Nino Floris

On Wed, Sep 25, 2019 at 10:31 PM Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:
>
> Hello, can you please post an updated version of this patch?  Note that
> Travis has a small complaint:
>
> gcc -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Werror=vla -Wendif-labels
-Wmissing-format-attribute-Wformat-security -fno-strict-aliasing -fwrapv -fexcess-precision=standard -g -O2 -Wall
-Werror-fPIC -DLOWER_NODE -I. -I. -I../../src/include -I/usr/include/x86_64-linux-gnu -D_GNU_SOURCE   -c -o crc32.o
crc32.c
> crc32.c: In function ‘ltree_crc32_sz’:
> crc32.c:26:12: error: initialization discards ‘const’ qualifier from pointer target type
[-Werror=discarded-qualifiers]
>   char    *p = buf;
>             ^
> cc1: all warnings being treated as errors
> <builtin>: recipe for target 'crc32.o' failed
> make[4]: *** [crc32.o] Error 1
> make[4]: Leaving directory '/home/travis/build/postgresql-cfbot/postgresql/contrib/ltree'
>
> --
> Álvaro Herrera                https://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Mon, Nov 11, 2019 at 03:44:54PM +0100, Nino Floris wrote:
> Alright, as usual life got in the way. I've attached a new version of
> the patch with pgindent changes.
>
> > What do you think about writing patch for ALTER TYPE?
> I'd rather not :$
>
> > 1) Write migration script, which directly updates pg_type.
> This sounds like the best option right now, if we don't want people to
> do manual migrations to ltree 1.2.
> How would I best go at this?
>
> > Travis has a small complaint:
> Should be fixed!

The latest patch provided fails to apply for me on HEAD.  Please
provide a rebase.  For now I am bumping this patch to next CF with
"waiting on author" as status.
--
Michael

On Fri, Nov 29, 2019 at 11:29:03AM +0900, Michael Paquier wrote:
>On Mon, Nov 11, 2019 at 03:44:54PM +0100, Nino Floris wrote:
>> Alright, as usual life got in the way. I've attached a new version of
>> the patch with pgindent changes.
>>
>> > What do you think about writing patch for ALTER TYPE?
>> I'd rather not :$
>>
>> > 1) Write migration script, which directly updates pg_type.
>> This sounds like the best option right now, if we don't want people to
>> do manual migrations to ltree 1.2.
>> How would I best go at this?
>>
>> > Travis has a small complaint:
>> Should be fixed!
>
>The latest patch provided fails to apply for me on HEAD.  Please
>provide a rebase.  For now I am bumping this patch to next CF with
>"waiting on author" as status.

Nino, any plans to submit a rebased/fixed patch, so that people can
review it? Not sure if this needs a simple rebase or something more
complex, all I know is cputube can't apply it.

regards

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

Attached is the new patch rebased onto master.

Best regards,
Nino Floris

On Thu, Jan 16, 2020 at 11:00 PM Tomas Vondra
<tomas.vondra@2ndquadrant.com> wrote:
>
> On Fri, Nov 29, 2019 at 11:29:03AM +0900, Michael Paquier wrote:
> >On Mon, Nov 11, 2019 at 03:44:54PM +0100, Nino Floris wrote:
> >> Alright, as usual life got in the way. I've attached a new version of
> >> the patch with pgindent changes.
> >>
> >> > What do you think about writing patch for ALTER TYPE?
> >> I'd rather not :$
> >>
> >> > 1) Write migration script, which directly updates pg_type.
> >> This sounds like the best option right now, if we don't want people to
> >> do manual migrations to ltree 1.2.
> >> How would I best go at this?
> >>
> >> > Travis has a small complaint:
> >> Should be fixed!
> >
> >The latest patch provided fails to apply for me on HEAD.  Please
> >provide a rebase.  For now I am bumping this patch to next CF with
> >"waiting on author" as status.
>
> Nino, any plans to submit a rebased/fixed patch, so that people can
> review it? Not sure if this needs a simple rebase or something more
> complex, all I know is cputube can't apply it.
>
> regards
>
> --
> Tomas Vondra                  http://www.2ndQuadrant.com
> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Nino Floris <mail@ninofloris.com> writes:
> Attached is the new patch rebased onto master.

The approach of simply providing a 1.2 installation script, with no
upgrade path from 1.1, is surely not going to be acceptable to anyone
who's using ltree in production.

Fortunately for the odds of getting this patch accepted, we just
pushed an ALTER TYPE improvement that will solve your problem [1].
Please replace ltree--1.2.sql with an upgrade script that uses
that, and resubmit.

            regards, tom lane

[1] https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=fe30e7ebfa3846416f1adeb7cf611006513a4ee0

I wrote:
> Fortunately for the odds of getting this patch accepted, we just
> pushed an ALTER TYPE improvement that will solve your problem [1].
> Please replace ltree--1.2.sql with an upgrade script that uses
> that, and resubmit.

I decided it would be a shame for this to miss v13, seeing that
(a) it'd provide real-world use of that ALTER TYPE code, and
(b) we already bumped ltree's extension version for v13.

So I went ahead and rebased this, reviewed and pushed it.  The
main non-cosmetic thing I changed, other than using ALTER TYPE,
was that you had the output functions looking like this:

    pq_sendtext(&buf, res, pg_mbstrlen(res));

That's just wrong; it should be plain strlen().  Did you copy
that coding from someplace?  I could not find any similar
occurrences in our code tree.

            regards, tom lane