Content-Type: text/plain;
	charset=us-ascii
Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3776.700.51\))
Subject: Re: [PATCH] Add sortsupport for range types and btree_gist
From: "Andrey M. Borodin" <x4mmm@yandex-team.ru>
In-Reply-To: <61128550659807032dd29c8c8a71674c818cbbb6.camel@oopsware.de>
Date: Mon, 11 Nov 2024 23:03:36 +0500
Cc: jian he <jian.universality@gmail.com>,
 PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Content-Transfer-Encoding: quoted-printable
Message-Id: <7095B5FA-FADB-4524-B858-ECD14998710B@yandex-team.ru>
References: <437ccbcf-8f80-2919-411d-a3af88becf6c@cybertec.at>
 <77f4bacbeb9d223f90b3a0bd55ef7d65407bfddd.camel@oopsware.de>
 <CACJufxHvXHNi_9roSo2gLJVCSep1EgD-z=-ey6K_Xg96PHs9+g@mail.gmail.com>
 <CACJufxEVYNCQJEnRr6jpdrNSb9hH6YkavxrLabrRuCWoeTDHjQ@mail.gmail.com>
 <2f2e88bf1d44d06c1baf27bbf3db880f42a5cb87.camel@oopsware.de>
 <CACJufxHSsRk9egTjZWYXK4mJF8rsoV0XuRuw4uCxymoeeYBAJw@mail.gmail.com>
 <31f23e46f71b744fb639967169d7df4c748110af.camel@oopsware.de>
 <a380cfd805c37c2aa70b1083d45820e1a7cbe759.camel@oopsware.de>
 <7C9400DF-66FE-4308-BAD8-92FE3D0E55C6@yandex-team.ru>
 <0d9fbd59de15e2fc987a850c3d1f8a79a6ede825.camel@oopsware.de>
 <19c62e62c79b727d2c9a82eaeecfea5017001099.camel@oopsware.de>
 <4BADF8DD-A4D3-43AA-AA08-037F6FFA40BC@yandex-team.ru>
 <61128550659807032dd29c8c8a71674c818cbbb6.camel@oopsware.de>
To: Bernd Helmle <mailings@oopsware.de>
Archived-At: <https://www.postgresql.org/message-id/7095B5FA-FADB-4524-B858-ECD14998710B%40yandex-team.ru>
Precedence: bulk


> On 11 Nov 2024, at 21:41, Bernd Helmle <mailings@oopsware.de> wrote:
>=20
> Updated and rebased patches attached.

Hi Bernd!

Some nitpicking:
0. postgres % git apply ~/Downloads/v7.3-Add-GIST-sortsupport-*
/Users/x4mmm/Downloads/v7.3-Add-GIST-sortsupport-btree-gist.patch:19: =
space before tab in indent.
    oid oid_buffering timestamp timestamp_buffering timestamptz \
/Users/x4mmm/Downloads/v7.3-Add-GIST-sortsupport-btree-gist.patch:20: =
space before tab in indent.
    timestamptz_buffering time time_buffering timetz timetz_buffering =
date \
/Users/x4mmm/Downloads/v7.3-Add-GIST-sortsupport-btree-gist.patch:21: =
space before tab in indent.
    date_buffering interval interval_buffering macaddr macaddr_buffering =
\
/Users/x4mmm/Downloads/v7.3-Add-GIST-sortsupport-btree-gist.patch:22: =
space before tab in indent.
    macaddr8 macaddr8_buffering inet inet_buffering cidr cidr_buffering =
text \
/Users/x4mmm/Downloads/v7.3-Add-GIST-sortsupport-btree-gist.patch:23: =
space before tab in indent.
    text_buffering varchar varchar_buffering char char_buffering \
warning: 5 lines add whitespace errors.

1. I'm mostly seening patches formatted with `git patch-format` rather =
than diffs as patches...

2. Changes in contrib/btree_gist/btree_gist.c seem unnecessary.

3. Why do you move "typedef struct int32key" to btree_gist.h, but do not =
need to move all other keys?

4. These ifdedfs are not needed, just do INJECTION_POINT()
#ifdef USE_INJECTION_POINTS
INJECTION_POINT("btree-gist-sorted-build");
#endif

Also, there are 61 occurance of this code. Why not just 1 in =
gist_indexsortbuild() ?

Thanks!


Best regards, Andrey Borodin.