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

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