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.korot...@postgrespro.ru> wrote: > > Hi! > > On Sun, Aug 18, 2019 at 6:53 PM Nino Floris <m...@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
