Content-Type: text/plain;
	charset=utf-8
Mime-Version: 1.0 (Mac OS X Mail 11.5 \(3445.9.1\))
Subject: Re: [HACKERS] [PATCH] kNN for SP-GiST
From: Mark Dilger <hornschnorter@gmail.com>
In-Reply-To: 
 <CAPpHfdsqYeG4COHZfe_pn1=x0nTC41g-z7qfnse3qt4Lme+XFA@mail.gmail.com>
Date: Thu, 27 Sep 2018 10:28:16 -0700
Cc: Andrey Borodin <x4mmm@yandex-team.ru>,
 Nikita Glukhov <n.gluhov@postgrespro.ru>,
 David Steele <david@pgmasters.net>,
 Andres Freund <andres@anarazel.de>,
 pgsql-hackers <pgsql-hackers@postgresql.org>
Content-Transfer-Encoding: quoted-printable
Message-Id: <B2AEFCD0-836D-4654-9D59-3DF616E0A6F3@gmail.com>
References: <570825e8-47d0-4732-2bf6-88d67d2d51c8@postgrespro.ru>
 <b985b66a-240c-03f0-df66-faa8b045551b@postgrespro.ru>
 <CAPpHfdvLXtuPNNR2w0PXuTkuR=ssySSgEPrjuExjq05OM0=LBw@mail.gmail.com>
 <43c859f9-cb0d-1bd2-ba9f-c5e5b61185cc@postgrespro.ru>
 <20180302063548.7ojyn56xd4ptc36v@alap3.anarazel.de>
 <e8a7852a-e049-207f-d745-9a0b0e80d89c@pgmasters.net>
 <6a09b16a-0b93-6fb1-c14a-7797477c5f38@postgrespro.ru>
 <CAPpHfdu6Wm4DSAp8Pvwq0uo7fCSzsbrNy7x2v5EKK_g4Nkjx1Q@mail.gmail.com>
 <c6957e2c-3052-4fce-55ac-2cf7bcfd0dd9@postgrespro.ru>
 <01432E05-D1AD-4D80-A7B4-149A299E906B@yandex-team.ru>
 <13EDC8D7-A7DA-47ED-80B8-B48E01023633@yandex-team.ru>
 <8d429bed-1a7f-d592-7e13-03e99defde03@postgrespro.ru>
 <5A8DAE0D-E4E8-48A6-BC8C-048A240875A0@yandex-team.ru>
 <944a7113-1886-ee4e-b8e6-df3e4dbca02e@postgrespro.ru>
 <F6E32265-5E11-461A-83C1-1927B28E8600@yandex-team.ru>
 <CAPpHfdstcdyKr49qvLkVmcAd_GyKB-Umk+Qx6-g5Ni_fB8w1mA@mail.gmail.com>
 <CAPpHfdvGtEzwRHqOUM67ixvqLo+ZbGhV+KEk_YMLZZM_WTCy0g@mail.gmail.com>
 <CAPpHfduBO0VQxoL2+E7zqWitD7dLStpD-UJSLZPVRO0P3fDi9g@mail.gmail.com>
 <CAPpHfdsDCPJ0v+MEhSsZmtt38KW3eYqXG0oodRv0OmieXQiiRw@mail.gmail.com>
 <CAPpHfdscCV83E0Ks4Hk=jEYQLA4sL--W_5ty=TT_xyZ0DL88_A@mail.gmail.com>
 <CAPpHfdtLbjKiPLgPHM25HGMs096bvmJDmT5E_s38f_2EQTfjpA@mail.gmail.com>
 <C4994532-08F0-4993-8B95-30FDEE2B0975@yandex-team.ru>
 <CAPpHfdsqYeG4COHZfe_pn1=x0nTC41g-z7qfnse3qt4Lme+XFA@mail.gmail.com>
To: Alexander Korotkov <a.korotkov@postgrespro.ru>
Precedence: bulk


> On Sep 18, 2018, at 3:58 PM, Alexander Korotkov =
<a.korotkov@postgrespro.ru> wrote:
>=20
> On Mon, Sep 17, 2018 at 12:42 PM Andrey Borodin <x4mmm@yandex-team.ru> =
wrote:
>>> 17 =D1=81=D0=B5=D0=BD=D1=82. 2018 =D0=B3., =D0=B2 2:03, Alexander =
Korotkov <a.korotkov@postgrespro.ru> =D0=BD=D0=B0=D0=BF=D0=B8=D1=81=D0=B0=D0=
=BB(=D0=B0):
>>>=20
>>> Also, it appears to me that it's OK to be a single patch
>>=20
>> +1, ISTM that these 6 patches represent atomic unit of work.
>=20
> Thank you, pushed.

One note on this commit,

in my fork, I have converted BoxPGetDatum from a macro to a static =
inline function,
and it shows a thinko in your commit, namely that in->leafDatum is of =
type (BOX *),
but is actually (as the name implies) already a Datum:

spgquadtreeproc.c:469:27: error: incompatible integer to pointer =
conversion passing 'Datum' (aka 'unsigned long') to parameter of type =
'BOX *' [-Werror,-Wint-conversion]
                                                                         =
                               BoxPGetDatum(in->leafDatum), true,

I'm not claiming this creates any active bug, just that it would make =
more sense to
handle the typing cleanly.  Perhaps the following:

diff --git a/src/backend/access/spgist/spgquadtreeproc.c =
b/src/backend/access/spgist/spgquadtreeproc.c
index dee438a307..90cc776899 100644
--- a/src/backend/access/spgist/spgquadtreeproc.c
+++ b/src/backend/access/spgist/spgquadtreeproc.c
@@ -465,8 +465,7 @@ spg_quad_leaf_consistent(PG_FUNCTION_ARGS)
=20
        if (res && in->norderbys > 0)
                /* ok, it passes -> let's compute the distances */
-               out->distances =3D spg_key_orderbys_distances(
-                                                                        =
                               BoxPGetDatum(in->leafDatum), true,
+               out->distances =3D =
spg_key_orderbys_distances(in->leafDatum, true,
                                                                         =
                               in->orderbys, in->norderbys);

mark=