Thread: [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
Attachment
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
Attachment
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
Attachment
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
Attachment
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
Hi Tom,
Thanks a lot for pushing this through.
In complete agreement on fixing mbstrlen, it would clearly have lead to cut off string sends, or worse (does the binary protocol use null terminated strings, or are they length prefixed?). Apologies anyways, it's been a while so I don't know how it may have ended up there, thanks for catching it.
Even though it was a bit bumpy, and vastly different from my usual github contribution flow, I've had a good time contributing my first patch!
Best regards,
Nino Floris
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