CUBE seems a bit confused about ORDER BY
Hi,
I've noticed this suspicious behavior of "cube" data type with ORDER BY,
which I believe is a bug in the extension (or the GiST index support).
The following example comes directly from regression tests added by
33bd250f (so CC Teodor and Stas, who are mentioned in the commit).
This query should produce results with ordering "ascending by 2nd
coordinate or upper right corner". To make it clear, I've added the
"c~>4" expression to the query, otherwise it's right from the test.
test=# SELECT c~>4 "c~>4", * FROM test_cube ORDER BY c~>4 LIMIT 15;
c~>4 | c
------+---------------------------
50 | (30333, 50),(30273, 6)
75 | (43301, 75),(43227, 43)
142 | (19650, 142),(19630, 51)
160 | (2424, 160),(2424, 81)
171 | (3449, 171),(3354, 108)
155 | (18037, 155),(17941, 109)
208 | (28511, 208),(28479, 114)
217 | (19946, 217),(19941, 118)
191 | (16906, 191),(16816, 139)
187 | (759, 187),(662, 163)
266 | (22684, 266),(22656, 181)
255 | (24423, 255),(24360, 213)
249 | (45989, 249),(45910, 222)
377 | (11399, 377),(11360, 294)
389 | (12162, 389),(12103, 309)
(15 rows)
As you can see, it's not actually sorted by the c~>4 coordinate (but by
c~>2, which it the last number).
Moreover, disabling index scans fixes the ordering:
test=# set enable_indexscan = off;
SET
test=# SELECT c~>4, * FROM test_cube ORDER BY c~>4 LIMIT 15; --
ascending by 2nd coordinate or upper right corner
?column? | c
----------+---------------------------
50 | (30333, 50),(30273, 6)
75 | (43301, 75),(43227, 43)
142 | (19650, 142),(19630, 51)
155 | (18037, 155),(17941, 109)
160 | (2424, 160),(2424, 81)
171 | (3449, 171),(3354, 108)
187 | (759, 187),(662, 163)
191 | (16906, 191),(16816, 139)
208 | (28511, 208),(28479, 114)
217 | (19946, 217),(19941, 118)
249 | (45989, 249),(45910, 222)
255 | (24423, 255),(24360, 213)
266 | (22684, 266),(22656, 181)
367 | (31018, 367),(30946, 333)
377 | (11399, 377),(11360, 294)
(15 rows)
Seems like a bug somewhere in gist_cube_ops, I guess?
regards
--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Hi!
On Fri, Oct 20, 2017 at 12:52 AM, Tomas Vondra <tomas.vondra@2ndquadrant.com
wrote:
I've noticed this suspicious behavior of "cube" data type with ORDER BY,
which I believe is a bug in the extension (or the GiST index support).
The following example comes directly from regression tests added by
33bd250f (so CC Teodor and Stas, who are mentioned in the commit).This query should produce results with ordering "ascending by 2nd
coordinate or upper right corner". To make it clear, I've added the
"c~>4" expression to the query, otherwise it's right from the test.test=# SELECT c~>4 "c~>4", * FROM test_cube ORDER BY c~>4 LIMIT 15;
c~>4 | c
------+---------------------------
50 | (30333, 50),(30273, 6)
75 | (43301, 75),(43227, 43)
142 | (19650, 142),(19630, 51)
160 | (2424, 160),(2424, 81)
171 | (3449, 171),(3354, 108)
155 | (18037, 155),(17941, 109)
208 | (28511, 208),(28479, 114)
217 | (19946, 217),(19941, 118)
191 | (16906, 191),(16816, 139)
187 | (759, 187),(662, 163)
266 | (22684, 266),(22656, 181)
255 | (24423, 255),(24360, 213)
249 | (45989, 249),(45910, 222)
377 | (11399, 377),(11360, 294)
389 | (12162, 389),(12103, 309)
(15 rows)As you can see, it's not actually sorted by the c~>4 coordinate (but by
c~>2, which it the last number).Moreover, disabling index scans fixes the ordering:
test=# set enable_indexscan = off;
SET
test=# SELECT c~>4, * FROM test_cube ORDER BY c~>4 LIMIT 15; --
ascending by 2nd coordinate or upper right corner
?column? | c
----------+---------------------------
50 | (30333, 50),(30273, 6)
75 | (43301, 75),(43227, 43)
142 | (19650, 142),(19630, 51)
155 | (18037, 155),(17941, 109)
160 | (2424, 160),(2424, 81)
171 | (3449, 171),(3354, 108)
187 | (759, 187),(662, 163)
191 | (16906, 191),(16816, 139)
208 | (28511, 208),(28479, 114)
217 | (19946, 217),(19941, 118)
249 | (45989, 249),(45910, 222)
255 | (24423, 255),(24360, 213)
266 | (22684, 266),(22656, 181)
367 | (31018, 367),(30946, 333)
377 | (11399, 377),(11360, 294)
(15 rows)Seems like a bug somewhere in gist_cube_ops, I guess?
+1,
that definitely looks like a bug. Thank you for reporting!
I'll take a look on it in couple days.
------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
On Fri, Oct 20, 2017 at 1:01 AM, Alexander Korotkov <
a.korotkov@postgrespro.ru> wrote:
On Fri, Oct 20, 2017 at 12:52 AM, Tomas Vondra <
tomas.vondra@2ndquadrant.com> wrote:I've noticed this suspicious behavior of "cube" data type with ORDER BY,
which I believe is a bug in the extension (or the GiST index support).
The following example comes directly from regression tests added by
33bd250f (so CC Teodor and Stas, who are mentioned in the commit).This query should produce results with ordering "ascending by 2nd
coordinate or upper right corner". To make it clear, I've added the
"c~>4" expression to the query, otherwise it's right from the test.test=# SELECT c~>4 "c~>4", * FROM test_cube ORDER BY c~>4 LIMIT 15;
c~>4 | c
------+---------------------------
50 | (30333, 50),(30273, 6)
75 | (43301, 75),(43227, 43)
142 | (19650, 142),(19630, 51)
160 | (2424, 160),(2424, 81)
171 | (3449, 171),(3354, 108)
155 | (18037, 155),(17941, 109)
208 | (28511, 208),(28479, 114)
217 | (19946, 217),(19941, 118)
191 | (16906, 191),(16816, 139)
187 | (759, 187),(662, 163)
266 | (22684, 266),(22656, 181)
255 | (24423, 255),(24360, 213)
249 | (45989, 249),(45910, 222)
377 | (11399, 377),(11360, 294)
389 | (12162, 389),(12103, 309)
(15 rows)As you can see, it's not actually sorted by the c~>4 coordinate (but by
c~>2, which it the last number).Moreover, disabling index scans fixes the ordering:
test=# set enable_indexscan = off;
SET
test=# SELECT c~>4, * FROM test_cube ORDER BY c~>4 LIMIT 15; --
ascending by 2nd coordinate or upper right corner
?column? | c
----------+---------------------------
50 | (30333, 50),(30273, 6)
75 | (43301, 75),(43227, 43)
142 | (19650, 142),(19630, 51)
155 | (18037, 155),(17941, 109)
160 | (2424, 160),(2424, 81)
171 | (3449, 171),(3354, 108)
187 | (759, 187),(662, 163)
191 | (16906, 191),(16816, 139)
208 | (28511, 208),(28479, 114)
217 | (19946, 217),(19941, 118)
249 | (45989, 249),(45910, 222)
255 | (24423, 255),(24360, 213)
266 | (22684, 266),(22656, 181)
367 | (31018, 367),(30946, 333)
377 | (11399, 377),(11360, 294)
(15 rows)Seems like a bug somewhere in gist_cube_ops, I guess?
+1,
that definitely looks like a bug. Thank you for reporting!
I'll take a look on it in couple days.
I've reviewed code of ~> operator and its KNN-GiST support. Unfortunately,
it appears that it's broken in design... The explanation is above.
We've following implementation of ~> operator.
if (coord <= DIM(cube))
{
if (IS_POINT(cube))
PG_RETURN_FLOAT8(cube->x[coord - 1]);
else
PG_RETURN_FLOAT8(Min(cube->x[coord - 1],
cube->x[coord - 1 + DIM(cube)]));
}
else
{
if (IS_POINT(cube))
PG_RETURN_FLOAT8(cube->x[(coord - 1) % DIM(cube)]);
else
PG_RETURN_FLOAT8(Max(cube->x[coord - 1],
cube->x[coord - 1 - DIM(cube)]));
}
Thus, for cube of N dimensions [1; N - 1] are lower bounds and [N; 2*N - 1]
are upper bounds. However, we might have indexed cubes of different
dimensions. For example, for cube of 2 dimensions "cube ~> 3" selects
upper bound of 1st dimension. For cube of 3 dimensions "cube ~> 3" selects
lower bound of 3rd dimension.
Therefore, despite ~> operator was specially invented to be supported by
KNN-GIST, it can't serve this way.
Regarding particular case discovered by Tomas, I think the error is in the
GiST supporting code.
if (strategy == CubeKNNDistanceCoord)
{
int coord = PG_GETARG_INT32(1);
if (DIM(cube) == 0)
retval = 0.0;
else if (IS_POINT(cube))
retval = cube->x[(coord - 1) % DIM(cube)];
else
retval = Min(cube->x[(coord - 1) % DIM(cube)],
cube->x[(coord - 1) % DIM(cube) + DIM(cube)]);
}
g_cube_distance() always returns lower bound of cube. It should return
upper bound for coord > DIM(cube).
It would be also nice to provide descending ordering using KNN-GiST. It
would be especially effective for upper bound. Since, KNN-GiST doesn't
support explicit descending ordering, it might be useful to make ~>
operator return negative of coordinate when negative argument is provided.
For instance, '(1,2), (3,4)'::cube ~> -1 return -1.
I'd like to propose following way to resolve design issue. cube ~> (2*N -
1) should return lower bound of Nth coordinate of the cube while cube ~>
2*N should return upper bound of Nth coordinate. Then it would be
independent on number of coordinates in particular cube. For sure, it
would be user-visible incompatible change. But I think there is not so
many users of this operator yet. Also, current behavior of ~> seems quite
useless.
Any thoughts?
------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
On Sun, Oct 29, 2017 at 1:30 AM, Alexander Korotkov <
a.korotkov@postgrespro.ru> wrote:
On Fri, Oct 20, 2017 at 1:01 AM, Alexander Korotkov <
a.korotkov@postgrespro.ru> wrote:
I've reviewed code of ~> operator and its KNN-GiST support.
Unfortunately, it appears that it's broken in design... The explanation is
above.We've following implementation of ~> operator.
if (coord <= DIM(cube))
{
if (IS_POINT(cube))
PG_RETURN_FLOAT8(cube->x[coord - 1]);
else
PG_RETURN_FLOAT8(Min(cube->x[coord - 1],
cube->x[coord - 1 + DIM(cube)]));
}
else
{
if (IS_POINT(cube))
PG_RETURN_FLOAT8(cube->x[(coord - 1) % DIM(cube)]);
else
PG_RETURN_FLOAT8(Max(cube->x[coord - 1],
cube->x[coord - 1 - DIM(cube)]));
}Thus, for cube of N dimensions [1; N - 1] are lower bounds and [N; 2*N -
1] are upper bounds. However, we might have indexed cubes of different
dimensions. For example, for cube of 2 dimensions "cube ~> 3" selects
upper bound of 1st dimension. For cube of 3 dimensions "cube ~> 3" selects
lower bound of 3rd dimension.Therefore, despite ~> operator was specially invented to be supported by
KNN-GIST, it can't serve this way.Regarding particular case discovered by Tomas, I think the error is in the
GiST supporting code.if (strategy == CubeKNNDistanceCoord)
{
int coord = PG_GETARG_INT32(1);
if (DIM(cube) == 0)
retval = 0.0;
else if (IS_POINT(cube))
retval = cube->x[(coord - 1) % DIM(cube)];
else
retval = Min(cube->x[(coord - 1) % DIM(cube)],
cube->x[(coord - 1) % DIM(cube) + DIM(cube)]);
}g_cube_distance() always returns lower bound of cube. It should return
upper bound for coord > DIM(cube).It would be also nice to provide descending ordering using KNN-GiST. It
would be especially effective for upper bound. Since, KNN-GiST doesn't
support explicit descending ordering, it might be useful to make ~>
operator return negative of coordinate when negative argument is provided.
For instance, '(1,2), (3,4)'::cube ~> -1 return -1.I'd like to propose following way to resolve design issue. cube ~> (2*N -
1) should return lower bound of Nth coordinate of the cube while cube ~>
2*N should return upper bound of Nth coordinate. Then it would be
independent on number of coordinates in particular cube. For sure, it
would be user-visible incompatible change. But I think there is not so
many users of this operator yet. Also, current behavior of ~> seems quite
useless.
Thus, I heard no objection to my approach. Attached patch changes ~>
operator in the proposed way and fixes KNN-GiST search for that. I'm going
to register this patch to the nearest commitfest.
------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
Attachments:
cube-knn-fix-1.patchapplication/octet-stream; name=cube-knn-fix-1.patchDownload+1140-727
On 10/30/2017 03:04 PM, Alexander Korotkov wrote:
On Sun, Oct 29, 2017 at 1:30 AM, Alexander Korotkov
<a.korotkov@postgrespro.ru <mailto:a.korotkov@postgrespro.ru>> wrote:
...
Thus, I heard no objection to my approach. Attached patch changes ~>
operator in the proposed way and fixes KNN-GiST search for that. I'm
going to register this patch to the nearest commitfest.
Seems fine to me, although perhaps it should be split into two parts.
One with the cube_coord_llur fixes, and then g_cube_distance changes
adding support for negative coordinates.
regards
--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Hi, Tomas!
On Mon, Nov 20, 2017 at 4:09 AM, Tomas Vondra <tomas.vondra@2ndquadrant.com>
wrote:
Seems fine to me, although perhaps it should be split into two parts.
One with the cube_coord_llur fixes, and then g_cube_distance changes
adding support for negative coordinates.
Thank you for your feedback. I'll split this patch into two.
------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
On Mon, Nov 20, 2017 at 1:59 PM, Alexander Korotkov <
a.korotkov@postgrespro.ru> wrote:
On Mon, Nov 20, 2017 at 4:09 AM, Tomas Vondra <
tomas.vondra@2ndquadrant.com> wrote:Seems fine to me, although perhaps it should be split into two parts.
One with the cube_coord_llur fixes, and then g_cube_distance changes
adding support for negative coordinates.Thank you for your feedback. I'll split this patch into two.
Please, find two patches attached.
------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
On Tue, Nov 21, 2017 at 7:07 AM, Alexander Korotkov
<a.korotkov@postgrespro.ru> wrote:
On Mon, Nov 20, 2017 at 1:59 PM, Alexander Korotkov
<a.korotkov@postgrespro.ru> wrote:On Mon, Nov 20, 2017 at 4:09 AM, Tomas Vondra
<tomas.vondra@2ndquadrant.com> wrote:Seems fine to me, although perhaps it should be split into two parts.
One with the cube_coord_llur fixes, and then g_cube_distance changes
adding support for negative coordinates.Thank you for your feedback. I'll split this patch into two.
Please, find two patches attached.
This got no reviews, so moved to next CF.
--
Michael
On 11/29/2017 06:13 AM, Michael Paquier wrote:
On Tue, Nov 21, 2017 at 7:07 AM, Alexander Korotkov
<a.korotkov@postgrespro.ru> wrote:On Mon, Nov 20, 2017 at 1:59 PM, Alexander Korotkov
<a.korotkov@postgrespro.ru> wrote:On Mon, Nov 20, 2017 at 4:09 AM, Tomas Vondra
<tomas.vondra@2ndquadrant.com> wrote:Seems fine to me, although perhaps it should be split into two parts.
One with the cube_coord_llur fixes, and then g_cube_distance changes
adding support for negative coordinates.Thank you for your feedback. I'll split this patch into two.
Please, find two patches attached.
This got no reviews, so moved to next CF.
That is somewhat inaccurate, I believe. I won't claim it received enough
reviews, but it certainly received some. And splitting it into two does
not invalidate that, I guess.
regards
--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Wed, Nov 29, 2017 at 1:30 PM, Tomas Vondra <tomas.vondra@2ndquadrant.com>
wrote:
On 11/29/2017 06:13 AM, Michael Paquier wrote:
On Tue, Nov 21, 2017 at 7:07 AM, Alexander Korotkov
<a.korotkov@postgrespro.ru> wrote:On Mon, Nov 20, 2017 at 1:59 PM, Alexander Korotkov
<a.korotkov@postgrespro.ru> wrote:On Mon, Nov 20, 2017 at 4:09 AM, Tomas Vondra
<tomas.vondra@2ndquadrant.com> wrote:Seems fine to me, although perhaps it should be split into two parts.
One with the cube_coord_llur fixes, and then g_cube_distance changes
adding support for negative coordinates.Thank you for your feedback. I'll split this patch into two.
Please, find two patches attached.
This got no reviews, so moved to next CF.
That is somewhat inaccurate, I believe. I won't claim it received enough
reviews, but it certainly received some. And splitting it into two does
not invalidate that, I guess.
Sure, patch got some review. I've no objection against moving this to the
next commitfest though.
Since, these patches include bug fix, it's possible that someone will
commit it before next commitfest.
------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
29 нояб. 2017 г., в 15:59, Alexander Korotkov <a.korotkov@postgrespro.ru> написал(а):
Sure, patch got some review. I've no objection against moving this to the next commitfest though.
Since, these patches include bug fix, it's possible that someone will commit it before next commitfest.
Hi!
I've took a glance at the patch, here's what catches my eye in comments: corrdinate, dimenstions, descinding, stoty.
I'll try to provide meaningful review next week.
Best regards, Andrey Borodin.
Hi!
On Wed, Nov 29, 2017 at 2:32 PM, Andrey Borodin <x4mmm@yandex-team.ru>
wrote:
29 нояб. 2017 г., в 15:59, Alexander Korotkov <a.korotkov@postgrespro.ru>
написал(а):Sure, patch got some review. I've no objection against moving this to the
next commitfest though.
Since, these patches include bug fix, it's possible that someone will
commit it before next commitfest.I've took a glance at the patch, here's what catches my eye in
comments: corrdinate, dimenstions, descinding, stoty.
Thank you for catching these typos. Rebased patchset with fixes typos is
attached.
I'll try to provide meaningful review next week.
Cool, thanks.
------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
On Wed, Nov 29, 2017 at 7:59 PM, Alexander Korotkov
<a.korotkov@postgrespro.ru> wrote:
Sure, patch got some review. I've no objection against moving this to the
next commitfest though.
Please note that as this is qualified as a bug fix, I was not going to
mark it as returned with feedback or such. We want to keep track of
fixes that are bugs and potentially need back-patching.
Since, these patches include bug fix, it's possible that someone will commit
it before next commitfest.
Sure. Committers are free to commit patches they see suited for
integration even out of the CF time.
--
Michael
On Wed, Nov 29, 2017 at 3:10 PM, Alexander Korotkov <
a.korotkov@postgrespro.ru> wrote:
I'll try to provide meaningful review next week.
Cool, thanks.
Andrey Borodin complained through Telegram that he can't apply my patches
using "git apply". After investigation of this problem, it appears that
the reason is that I still use git context diff setup according to our wiki
instruction (https://wiki.postgresql.org/wiki/Working_with_Git). I've look
around some threads and realized that I'm probably the only guy who still
sends context diff patches.
Attached patchset in universal diff format.
------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
The following review has been posted through the commitfest application:
make installcheck-world: tested, passed
Implements feature: tested, passed
Spec compliant: tested, passed
Documentation: tested, passed
Hi! I've reviewed the patch.
Patch works as expected, documented and tested.
The code does not use LL_COORD and UR_COORD used all around. But the code still is easily readable.
LL_COORD and UR_COORD are always combined with Min or Max function, so LL (Lower Left) and UR (Upper Right) are somewhat irrelevant names...
BTW if we swap implementation of these macro-funcs and fix cube_out, almost all tests pass again. This is evidence of good compatibility and shows that compatibility overhead is added everywhere.
I think that this patch is ready for committer.
Best regards, Andrey Borodin.
The new status of this patch is: Ready for Committer
Hi!
On Fri, Dec 8, 2017 at 11:21 AM, Andrey Borodin <x4mmm@yandex-team.ru>
wrote:
The following review has been posted through the commitfest application:
make installcheck-world: tested, passed
Implements feature: tested, passed
Spec compliant: tested, passed
Documentation: tested, passedHi! I've reviewed the patch.
Patch works as expected, documented and tested.
Thank you for reviewing this.
The code does not use LL_COORD and UR_COORD used all around. But the code
still is easily readable.LL_COORD and UR_COORD are always combined with Min or Max function, so LL
(Lower Left) and UR (Upper Right) are somewhat irrelevant names...
BTW if we swap implementation of these macro-funcs and fix cube_out,
almost all tests pass again. This is evidence of good compatibility and
shows that compatibility overhead is added everywhere.
Yes, the thing is that we change behavior of existing ~> operator. In
general, this is not good idea because it could affect existing users whose
already use this operator. Typically in such situation, we could leave
existing operator as is, and invent new operator with new behavior.
However, in this particular case, I think there are reasons to make an
exception to the rules. The reasons are following:
1) The ~> operator was designed especially for knn gist.
2) Knn gist support for current behavior is broken by design and can't be
fixed. Most we can do to fix existing ~> operator behavior as is to drop
knn gist support. But then ~> operator would be left useless.
3) It doesn't seems that ~> operator have many users yet, because an error
wasn't reported during whole release cycle.
I hope these reasons justify altering behavior of existing operator as an
exception to the rules. Another question is whether we should backpatch
it. But I think we could leave this decision to committer.
I think that this patch is ready for committer.
Good.
------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
Yes, the thing is that we change behavior of existing ~> operator.О©╫ In general,
this is not good idea because it could affect existing users whose already use
this operator.О©╫ Typically in such situation, we could leave existing operator as
is, and invent new operator with new behavior.О©╫ However, in this particular
case, I think there are reasons to make an exception to the rules.О©╫ The reasons
are following:
1) The ~> operator was designed especially for knn gist.
2) Knn gist support for current behavior is broken by design and can't be
fixed.О©╫ Most we can do to fix existing ~> operator behavior as is to drop knn
gist support.О©╫ But then ~> operator would be left useless.
3) It doesn't seems that ~> operator have many users yet, because an error
wasn't reported during whole release cycle.I hope these reasons justify altering behavior of existing operator as an
exception to the rules.О©╫ Another question is whether we should backpatch it.
But I think we could leave this decision to committer.I think that this patch is ready for committer.
I'm agree with changing behavior of existing ~> operator, but is any objection
here? Current implementation is not fixable and I hope that users which use this
operator will understand why we change it. Fortunately, the fix doesn't require
changes in system catalog.
The single question here is about index over expression with this operator, they
will need to reindex, which should be noted in release notes.
--
Teodor Sigaev E-mail: teodor@sigaev.ru
WWW: http://www.sigaev.ru/
On Tue, Dec 12, 2017 at 3:49 PM, Teodor Sigaev <teodor@sigaev.ru> wrote:
Yes, the thing is that we change behavior of existing ~> operator. In
general, this is not good idea because it could affect existing users whose
already use this operator. Typically in such situation, we could leave
existing operator as is, and invent new operator with new behavior.
However, in this particular case, I think there are reasons to make an
exception to the rules. The reasons are following:
1) The ~> operator was designed especially for knn gist.
2) Knn gist support for current behavior is broken by design and can't be
fixed. Most we can do to fix existing ~> operator behavior as is to drop
knn gist support. But then ~> operator would be left useless.
3) It doesn't seems that ~> operator have many users yet, because an
error wasn't reported during whole release cycle.I hope these reasons justify altering behavior of existing operator as an
exception to the rules. Another question is whether we should backpatch
it. But I think we could leave this decision to committer.I think that this patch is ready for committer.
I'm agree with changing behavior of existing ~> operator, but is any
objection here? Current implementation is not fixable and I hope that users
which use this operator will understand why we change it. Fortunately, the
fix doesn't require changes in system catalog.The single question here is about index over expression with this
operator, they will need to reindex, which should be noted in release notes.
Yes. I bet only few users have built indexes over ~> operator if any. Ask
them to reindex in the release notes seems OK for me.
------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
On 12/12/2017 01:52 PM, Alexander Korotkov wrote:
On Tue, Dec 12, 2017 at 3:49 PM, Teodor Sigaev <teodor@sigaev.ru
<mailto:teodor@sigaev.ru>> wrote:Yes, the thing is that we change behavior of existing ~>
operator. In general, this is not good idea because it could
affect existing users whose already use this operator.
Typically in such situation, we could leave existing operator as
is, and invent new operator with new behavior. However, in this
particular case, I think there are reasons to make an exception
to the rules. The reasons are following:
1) The ~> operator was designed especially for knn gist.
2) Knn gist support for current behavior is broken by design and
can't be fixed. Most we can do to fix existing ~> operator
behavior as is to drop knn gist support. But then ~> operator
would be left useless.
3) It doesn't seems that ~> operator have many users yet,
because an error wasn't reported during whole release cycle.I hope these reasons justify altering behavior of existing
operator as an exception to the rules. Another question is
whether we should backpatch it. But I think we could leave this
decision to committer.I think that this patch is ready for committer.
I'm agree with changing behavior of existing ~> operator, but is any
objection here? Current implementation is not fixable and I hope
that users which use this operator will understand why we change it.
Fortunately, the fix doesn't require changes in system catalog.The single question here is about index over expression with this
operator, they will need to reindex, which should be noted in
release notes.Yes. I bet only few users have built indexes over ~> operator if any.
Ask them to reindex in the release notes seems OK for me.
Is there a good way to detect such cases? Either in pg_upgrade, so that
we can print warnings, or at least manually (which would be suitable for
release notes).
regards
--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Yes.О©╫ I bet only few users have built indexes over ~> operator if any.
Ask them to reindex in the release notes seems OK for me.Is there a good way to detect such cases? Either in pg_upgrade, so that
we can print warnings, or at least manually (which would be suitable for
release notes).
Hmm, suppose, fix should be backpatched (because now it's unusable) and
pg_upgrade should not do anything. Just add release note to 10.0 and 11.0
Oh, check expression is affected too, users will need to reinsert data.
--
Teodor Sigaev E-mail: teodor@sigaev.ru
WWW: http://www.sigaev.ru/
