From 09e12b211a9de3bab76aa1513f122bed0de628cb Mon Sep 17 00:00:00 2001 From: Shihao Date: Wed, 23 Sep 2026 21:23:52 -0400 Subject: [PATCH v5 2/3] Fix GiST box indexes hiding rows next to a NaN box GiST union keys kept NaN coordinates, and no box comparison is true against a NaN bound. So an internal key covering a NaN box hid its whole subtree from searches, and KNN scans could return rows from it too late. Map NaNs to infinities when building union keys. Internal keys that still have a NaN, from indexes built before this fix, now match every search and get distance zero. So those indexes return correct results without a REINDEX, only less selectively. Also stop internal pages from pruning ~= when the query has a NaN, since box_same() matches it. For points, ~= also missed NaN queries at the leaf level, because it compared with FPeq() while point_eq() compares exactly when there is a NaN. Make the index agree with point_eq(). Author: Kirill Reshke Author: Shihao Zhong Reported-by: Ke Reported-by: Andrey Borodin Discussion: https://postgr.es/m/19705-548fda77321e062d@postgresql.org Backpatch-through: 14 --- src/backend/access/gist/gistproc.c | 139 ++++++++++++++++++++++++----- src/test/regress/expected/gist.out | 73 +++++++++++++++ src/test/regress/sql/gist.sql | 35 ++++++++ 3 files changed, 225 insertions(+), 22 deletions(-) diff --git a/src/backend/access/gist/gistproc.c b/src/backend/access/gist/gistproc.c index f1044f49d6c..0f405d4450f 100644 --- a/src/backend/access/gist/gistproc.c +++ b/src/backend/access/gist/gistproc.c @@ -43,6 +43,45 @@ static bool gist_bbox_zorder_abbrev_abort(int memtupcount, SortSupport ssup); /* Minimum accepted ratio of split */ #define LIMIT_RATIO 0.3 +/* + * Does the box have a NaN coordinate? + * + * No box comparison is true against a NaN, so a union key with one would + * hide everything below it. Union keys map NaNs to infinities instead (see + * adjustBox), but indexes built before that was done can still have NaN + * internal keys, and searches must descend into those. + */ +static inline bool +box_has_nan(const BOX *box) +{ + return isnan(box->high.x) || isnan(box->high.y) || + isnan(box->low.x) || isnan(box->low.y); +} + +/* Is the entry an internal key with a NaN coordinate? */ +static inline bool +nan_internal_key(const GISTENTRY *entry) +{ + return !GIST_LEAF(entry) && box_has_nan(DatumGetBoxP(entry->key)); +} + +/* float8_max() and float8_min(), but a NaN gives the matching infinity */ +static inline float8 +bound_max(float8 val1, float8 val2) +{ + if (isnan(val1) || isnan(val2)) + return get_float8_infinity(); + return float8_max(val1, val2); +} + +static inline float8 +bound_min(float8 val1, float8 val2) +{ + if (isnan(val1) || isnan(val2)) + return -get_float8_infinity(); + return float8_min(val1, val2); +} + /************************************************** * Box ops @@ -126,6 +165,10 @@ gist_box_consistent(PG_FUNCTION_ARGS) if (DatumGetBoxP(entry->key) == NULL || query == NULL) PG_RETURN_BOOL(false); + /* A NaN internal key tells us nothing, see box_has_nan() */ + if (nan_internal_key(entry)) + PG_RETURN_BOOL(true); + /* * if entry is not leaf, use rtree_internal_consistent, else use * gist_box_leaf_consistent @@ -141,19 +184,31 @@ gist_box_consistent(PG_FUNCTION_ARGS) } /* - * Increase BOX b to include addon. + * Increase BOX b to include addon. A NaN in either box grows b to the + * matching infinity. */ static void adjustBox(BOX *b, const BOX *addon) { - if (float8_lt(b->high.x, addon->high.x)) - b->high.x = addon->high.x; - if (float8_gt(b->low.x, addon->low.x)) - b->low.x = addon->low.x; - if (float8_lt(b->high.y, addon->high.y)) - b->high.y = addon->high.y; - if (float8_gt(b->low.y, addon->low.y)) - b->low.y = addon->low.y; + b->high.x = bound_max(b->high.x, addon->high.x); + b->low.x = bound_min(b->low.x, addon->low.x); + b->high.y = bound_max(b->high.y, addon->high.y); + b->low.y = bound_min(b->low.y, addon->low.y); +} + +/* Copy a BOX, mapping NaNs to infinities. */ +static void +snapBox(BOX *b, const BOX *box) +{ + *b = *box; + if (isnan(b->high.x)) + b->high.x = get_float8_infinity(); + if (isnan(b->high.y)) + b->high.y = get_float8_infinity(); + if (isnan(b->low.x)) + b->low.x = -get_float8_infinity(); + if (isnan(b->low.y)) + b->low.y = -get_float8_infinity(); } /* @@ -174,7 +229,7 @@ gist_box_union(PG_FUNCTION_ARGS) numranges = entryvec->n; pageunion = palloc_object(BOX); cur = DatumGetBoxP(entryvec->vector[0].key); - memcpy(pageunion, cur, sizeof(BOX)); + snapBox(pageunion, cur); for (i = 1; i < numranges; i++) { @@ -239,7 +294,7 @@ fallbackSplit(GistEntryVector *entryvec, GIST_SPLITVEC *v) if (unionL == NULL) { unionL = palloc_object(BOX); - *unionL = *cur; + snapBox(unionL, cur); } else adjustBox(unionL, cur); @@ -252,7 +307,7 @@ fallbackSplit(GistEntryVector *entryvec, GIST_SPLITVEC *v) if (unionR == NULL) { unionR = palloc_object(BOX); - *unionR = *cur; + snapBox(unionR, cur); } else adjustBox(unionR, cur); @@ -526,7 +581,7 @@ gist_box_picksplit(PG_FUNCTION_ARGS) { box = DatumGetBoxP(entryvec->vector[i].key); if (i == FirstOffsetNumber) - context.boundingBox = *box; + snapBox(&context.boundingBox, box); else adjustBox(&context.boundingBox, box); } @@ -715,7 +770,7 @@ gist_box_picksplit(PG_FUNCTION_ARGS) if (v->spl_nleft > 0) \ adjustBox(leftBox, box); \ else \ - *leftBox = *(box); \ + snapBox(leftBox, box); \ v->spl_left[v->spl_nleft++] = off; \ } while(0) @@ -724,7 +779,7 @@ gist_box_picksplit(PG_FUNCTION_ARGS) if (v->spl_nright > 0) \ adjustBox(rightBox, box); \ else \ - *rightBox = *(box); \ + snapBox(rightBox, box); \ v->spl_right[v->spl_nright++] = off; \ } while(0) @@ -959,6 +1014,13 @@ rtree_internal_consistent(BOX *key, BOX *query, StrategyNumber strategy) { bool retval; + /* + * A NaN query can still match with ~=, since box_same() treats NaNs as + * equal, but box_contain() never matches it. + */ + if (strategy == RTSameStrategyNumber && box_has_nan(query)) + return true; + switch (strategy) { case RTLeftStrategyNumber: @@ -1077,6 +1139,10 @@ gist_poly_consistent(PG_FUNCTION_ARGS) if (DatumGetBoxP(entry->key) == NULL || query == NULL) PG_RETURN_BOOL(false); + /* A NaN internal key tells us nothing, see box_has_nan() */ + if (nan_internal_key(entry)) + PG_RETURN_BOOL(true); + /* * Since the operators require recheck anyway, we can just use * rtree_internal_consistent even at leaf nodes. (This works in part @@ -1147,6 +1213,10 @@ gist_circle_consistent(PG_FUNCTION_ARGS) if (DatumGetBoxP(entry->key) == NULL || query == NULL) PG_RETURN_BOOL(false); + /* A NaN internal key tells us nothing, see box_has_nan() */ + if (nan_internal_key(entry)) + PG_RETURN_BOOL(true); + /* * Since the operators require recheck anyway, we can just use * rtree_internal_consistent even at leaf nodes. (This works in part @@ -1307,7 +1377,20 @@ gist_point_consistent_internal(StrategyNumber strategy, result = FPlt(key->low.y, query->y); break; case RTSameStrategyNumber: - if (isLeaf) + + /* + * point_eq() compares exactly when there is a NaN, so a NaN query + * matches points with the same NaN coordinates. Internal keys + * cannot tell us where those are, so search everything below. + */ + if (isnan(query->x) || isnan(query->y)) + { + /* key.high must equal key.low, so we can disregard it */ + result = !isLeaf || + (float8_eq(key->low.x, query->x) && + float8_eq(key->low.y, query->y)); + } + else if (isLeaf) { /* key.high must equal key.low, so we can disregard it */ result = (FPeq(key->low.x, query->x) && @@ -1345,6 +1428,10 @@ gist_point_consistent(PG_FUNCTION_ARGS) bool result; StrategyNumber strategyGroup; + /* A NaN internal key tells us nothing, see box_has_nan() */ + if (nan_internal_key(entry)) + PG_RETURN_BOOL(true); + /* * We have to remap these strategy numbers to get this klugy * classification logic to work. @@ -1465,9 +1552,13 @@ gist_point_distance(PG_FUNCTION_ARGS) switch (strategyGroup) { case PointStrategyNumberGroup: - distance = computeDistance(GIST_LEAF(entry), - DatumGetBoxP(entry->key), - PG_GETARG_POINT_P(1)); + /* A NaN internal key tells us nothing, see box_has_nan() */ + if (nan_internal_key(entry)) + distance = 0.0; + else + distance = computeDistance(GIST_LEAF(entry), + DatumGetBoxP(entry->key), + PG_GETARG_POINT_P(1)); break; default: elog(ERROR, "unrecognized strategy number: %d", strategy); @@ -1487,9 +1578,13 @@ gist_bbox_distance(GISTENTRY *entry, Datum query, StrategyNumber strategy) switch (strategyGroup) { case PointStrategyNumberGroup: - distance = computeDistance(false, - DatumGetBoxP(entry->key), - DatumGetPointP(query)); + /* A NaN internal key tells us nothing, see box_has_nan() */ + if (nan_internal_key(entry)) + distance = 0.0; + else + distance = computeDistance(false, + DatumGetBoxP(entry->key), + DatumGetPointP(query)); break; default: elog(ERROR, "unrecognized strategy number: %d", strategy); diff --git a/src/test/regress/expected/gist.out b/src/test/regress/expected/gist.out index ac79f94aa80..174f8b4251f 100644 --- a/src/test/regress/expected/gist.out +++ b/src/test/regress/expected/gist.out @@ -463,3 +463,76 @@ create index gist_tbl_box_index on gist_tbl using gist (b); insert into gist_tbl select box(point(0.05*i, 0.05*i)) from generate_series(0,10) as i; drop table gist_tbl; +-- a box with a NaN coordinate must not poison the union keys (bug #19705) +create table gist_nan_tbl (b box); +insert into gist_nan_tbl select box '(1,1),(2,2)' from generate_series(1, 1000); +insert into gist_nan_tbl values (box '(NaN,NaN),(0,0)'); +create index gist_nan_tbl_index on gist_nan_tbl using gist (b); +-- also insert rows after the build, to exercise the insertion path +insert into gist_nan_tbl select box '(3,3),(4,4)' from generate_series(1, 1000); +insert into gist_nan_tbl values (box '(NaN,NaN),(0,0)'); +set enable_seqscan = off; +set enable_bitmapscan = off; +select count(*) from gist_nan_tbl where b && box(point(0,0), point(5,5)); + count +------- + 2000 +(1 row) + +select count(*) from gist_nan_tbl where b <@ box(point(0,0), point(5,5)); + count +------- + 2000 +(1 row) + +select count(*) from gist_nan_tbl where b @> point '(1.5,1.5)'; + count +------- + 1000 +(1 row) + +select count(*) from gist_nan_tbl where b ~= box '(NaN,NaN),(0,0)'; + count +------- + 2 +(1 row) + +reset enable_seqscan; +reset enable_bitmapscan; +drop table gist_nan_tbl; +-- point ~= matches NaN coordinates exactly, the index must agree +create table gist_nan_point_tbl (p point); +insert into gist_nan_point_tbl + select point(i % 100, i / 100) from generate_series(0, 9999) i; +insert into gist_nan_point_tbl + values ('(NaN,NaN)'), ('(NaN,3)'), ('(3,NaN)'), ('(NaN,NaN)'); +create index gist_nan_point_tbl_index on gist_nan_point_tbl using gist (p); +set enable_seqscan = off; +set enable_bitmapscan = off; +select count(*) from gist_nan_point_tbl where p ~= point '(NaN,NaN)'; + count +------- + 2 +(1 row) + +select count(*) from gist_nan_point_tbl where p ~= point '(NaN,3)'; + count +------- + 1 +(1 row) + +select count(*) from gist_nan_point_tbl where p ~= point '(3,NaN)'; + count +------- + 1 +(1 row) + +select count(*) from gist_nan_point_tbl where p ~= point '(3,3)'; + count +------- + 1 +(1 row) + +reset enable_seqscan; +reset enable_bitmapscan; +drop table gist_nan_point_tbl; diff --git a/src/test/regress/sql/gist.sql b/src/test/regress/sql/gist.sql index 57dcc082450..654318fb4db 100644 --- a/src/test/regress/sql/gist.sql +++ b/src/test/regress/sql/gist.sql @@ -236,3 +236,38 @@ create index gist_tbl_box_index on gist_tbl using gist (b); insert into gist_tbl select box(point(0.05*i, 0.05*i)) from generate_series(0,10) as i; drop table gist_tbl; + +-- a box with a NaN coordinate must not poison the union keys (bug #19705) +create table gist_nan_tbl (b box); +insert into gist_nan_tbl select box '(1,1),(2,2)' from generate_series(1, 1000); +insert into gist_nan_tbl values (box '(NaN,NaN),(0,0)'); +create index gist_nan_tbl_index on gist_nan_tbl using gist (b); +-- also insert rows after the build, to exercise the insertion path +insert into gist_nan_tbl select box '(3,3),(4,4)' from generate_series(1, 1000); +insert into gist_nan_tbl values (box '(NaN,NaN),(0,0)'); +set enable_seqscan = off; +set enable_bitmapscan = off; +select count(*) from gist_nan_tbl where b && box(point(0,0), point(5,5)); +select count(*) from gist_nan_tbl where b <@ box(point(0,0), point(5,5)); +select count(*) from gist_nan_tbl where b @> point '(1.5,1.5)'; +select count(*) from gist_nan_tbl where b ~= box '(NaN,NaN),(0,0)'; +reset enable_seqscan; +reset enable_bitmapscan; +drop table gist_nan_tbl; + +-- point ~= matches NaN coordinates exactly, the index must agree +create table gist_nan_point_tbl (p point); +insert into gist_nan_point_tbl + select point(i % 100, i / 100) from generate_series(0, 9999) i; +insert into gist_nan_point_tbl + values ('(NaN,NaN)'), ('(NaN,3)'), ('(3,NaN)'), ('(NaN,NaN)'); +create index gist_nan_point_tbl_index on gist_nan_point_tbl using gist (p); +set enable_seqscan = off; +set enable_bitmapscan = off; +select count(*) from gist_nan_point_tbl where p ~= point '(NaN,NaN)'; +select count(*) from gist_nan_point_tbl where p ~= point '(NaN,3)'; +select count(*) from gist_nan_point_tbl where p ~= point '(3,NaN)'; +select count(*) from gist_nan_point_tbl where p ~= point '(3,3)'; +reset enable_seqscan; +reset enable_bitmapscan; +drop table gist_nan_point_tbl; -- 2.43.0