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

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(dot)korotkov(at)postgrespro(dot)ru> wrote:
>
> Hi!
>
> On Sun, Aug 18, 2019 at 6:53 PM Nino Floris <mail(at)ninofloris(dot)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