[PATCH] ltree, lquery, and ltxtquery binary protocol support

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH] ltree, lquery, and ltxtquery binary protocol support

Nino Floris
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

0001-Implements-ltree-lquery-and-ltxtquery-binary-protoco.patch (82K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] ltree, lquery, and ltxtquery binary protocol support

Alexander Korotkov
Hi!

On Sun, Aug 18, 2019 at 6:53 PM Nino Floris <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] ltree, lquery, and ltxtquery binary protocol support

Nino Floris
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
<[hidden email]> wrote:

>
> Hi!
>
> On Sun, Aug 18, 2019 at 6:53 PM Nino Floris <[hidden email]> 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



Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] ltree, lquery, and ltxtquery binary protocol support

Alexander Korotkov
On Wed, Sep 11, 2019 at 9:45 PM Nino Floris <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] ltree, lquery, and ltxtquery binary protocol support

Alvaro Herrera-9
In reply to this post by Nino Floris
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