From e00c0b4414e0bfb1d8405e67de510b549fc8deed Mon Sep 17 00:00:00 2001 From: Shihao Date: Wed, 23 Sep 2026 21:23:52 -0400 Subject: [PATCH v4 1/2] Fix BRIN box_inclusion_ops hiding rows next to a NaN box bound_box() lets a NaN coordinate into the range summary, and no box operator matches a NaN bound, so BRIN skipped the whole range. Add a mergeable support function, box_mergeable(), that rejects boxes with a NaN coordinate. A range holding one is then marked unmergeable and always scanned. BRIN also checks the first value of a range now, which it used to copy into the summary unchecked. At scan time, a summary that is not mergeable even with itself is treated as unmergeable. So indexes built before this fix return correct results without a REINDEX. This needs a catalog change, so it is for master only. Author: Kirill Reshke Author: Shihao Zhong Reported-by: Ke Reported-by: Andrey Borodin Discussion: https://postgr.es/m/19705-548fda77321e062d@postgresql.org --- doc/src/sgml/brin.sgml | 6 +- src/backend/access/brin/brin_inclusion.c | 22 +++++++ src/backend/utils/adt/geo_ops.c | 19 ++++++ src/include/catalog/pg_amproc.dat | 2 + src/include/catalog/pg_proc.dat | 3 + src/test/regress/expected/brin.out | 73 ++++++++++++++++++++++++ src/test/regress/expected/geometry.out | 13 +++++ src/test/regress/sql/brin.sql | 45 +++++++++++++++ src/test/regress/sql/geometry.sql | 4 ++ 9 files changed, 185 insertions(+), 2 deletions(-) diff --git a/doc/src/sgml/brin.sgml b/doc/src/sgml/brin.sgml index 64fb520db7e..2b3e41056b1 100644 --- a/doc/src/sgml/brin.sgml +++ b/doc/src/sgml/brin.sgml @@ -1187,8 +1187,10 @@ typedef struct BrinOpcInfo Support function numbers 12 and 14 are provided to support irregularities of built-in data types. Function number 12 - is used to support network addresses from different families which - are not mergeable. Function number 14 is used to support + is used to support network addresses from different families, and + boxes with NaN coordinates, which are not mergeable. A value that is + not mergeable even with itself makes its block range always match. + Function number 14 is used to support empty ranges. Function number 13 is an optional but recommended one, which allows the new value to be checked before it is passed to the union function. As the BRIN framework can shortcut diff --git a/src/backend/access/brin/brin_inclusion.c b/src/backend/access/brin/brin_inclusion.c index 5a2058d9aad..4dddc45f03d 100644 --- a/src/backend/access/brin/brin_inclusion.c +++ b/src/backend/access/brin/brin_inclusion.c @@ -191,8 +191,19 @@ brin_inclusion_add_value(PG_FUNCTION_ARGS) PG_RETURN_BOOL(false); } + /* + * A new union is not checked for mergeability below, so check the value + * against itself. A box with a NaN coordinate fails this. + */ if (new) + { + finfo = inclusion_get_procinfo(bdesc, attno, PROCNUM_MERGEABLE, true); + if (finfo != NULL && + !DatumGetBool(FunctionCall2Coll(finfo, colloid, newval, newval))) + column->bv_values[INCLUSION_UNMERGEABLE] = BoolGetDatum(true); + PG_RETURN_BOOL(true); + } /* Check if the new value is already contained. */ finfo = inclusion_get_procinfo(bdesc, attno, PROCNUM_CONTAINS, true); @@ -274,6 +285,17 @@ brin_inclusion_consistent(PG_FUNCTION_ARGS) subtype = key->sk_subtype; query = key->sk_argument; unionval = column->bv_values[INCLUSION_UNION]; + + /* + * Likewise if the union is not mergeable even with itself. An index + * built before the opclass had a mergeable function can hold such a union + * without the flag, like a box union with NaN bounds. + */ + finfo = inclusion_get_procinfo(bdesc, attno, PROCNUM_MERGEABLE, true); + if (finfo != NULL && + !DatumGetBool(FunctionCall2Coll(finfo, colloid, unionval, unionval))) + PG_RETURN_BOOL(true); + switch (key->sk_strategy) { /* diff --git a/src/backend/utils/adt/geo_ops.c b/src/backend/utils/adt/geo_ops.c index 73324b91fe5..66bb58fb7d6 100644 --- a/src/backend/utils/adt/geo_ops.c +++ b/src/backend/utils/adt/geo_ops.c @@ -4427,6 +4427,25 @@ boxes_bound_box(PG_FUNCTION_ARGS) PG_RETURN_BOX_P(container); } +/* + * Can the two boxes be merged into one bounding box? + * + * Not if either has a NaN coordinate: the bounding box would have NaN + * bounds too, and no box operator matches those. This is the mergeable + * support function of BRIN box_inclusion_ops. + */ +Datum +box_mergeable(PG_FUNCTION_ARGS) +{ + BOX *box1 = PG_GETARG_BOX_P(0), + *box2 = PG_GETARG_BOX_P(1); + + PG_RETURN_BOOL(!(isnan(box1->high.x) || isnan(box1->high.y) || + isnan(box1->low.x) || isnan(box1->low.y) || + isnan(box2->high.x) || isnan(box2->high.y) || + isnan(box2->low.x) || isnan(box2->low.y))); +} + /*********************************************************************** ** diff --git a/src/include/catalog/pg_amproc.dat b/src/include/catalog/pg_amproc.dat index 4a1efdbc899..db24e42e795 100644 --- a/src/include/catalog/pg_amproc.dat +++ b/src/include/catalog/pg_amproc.dat @@ -2033,6 +2033,8 @@ amproc => 'brin_inclusion_union' }, { amprocfamily => 'brin/box_inclusion_ops', amproclefttype => 'box', amprocrighttype => 'box', amprocnum => '11', amproc => 'bound_box' }, +{ amprocfamily => 'brin/box_inclusion_ops', amproclefttype => 'box', + amprocrighttype => 'box', amprocnum => '12', amproc => 'box_mergeable' }, { amprocfamily => 'brin/box_inclusion_ops', amproclefttype => 'box', amprocrighttype => 'box', amprocnum => '13', amproc => 'box_contain' }, diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat index f46427258e3..bbbce58a962 100644 --- a/src/include/catalog/pg_proc.dat +++ b/src/include/catalog/pg_proc.dat @@ -2140,6 +2140,9 @@ { oid => '4067', descr => 'bounding box of two boxes', proname => 'bound_box', prorettype => 'box', proargtypes => 'box box', prosrc => 'boxes_bound_box' }, +{ oid => '8400', descr => 'can two boxes be merged into a single summary', + proname => 'box_mergeable', prorettype => 'bool', proargtypes => 'box box', + prosrc => 'box_mergeable' }, { oid => '981', descr => 'box diagonal', proname => 'diagonal', prorettype => 'lseg', proargtypes => 'box', prosrc => 'box_diagonal' }, diff --git a/src/test/regress/expected/brin.out b/src/test/regress/expected/brin.out index e1db2280cf9..445efddd8f6 100644 --- a/src/test/regress/expected/brin.out +++ b/src/test/regress/expected/brin.out @@ -589,3 +589,76 @@ CREATE INDEX brin_insert_optimization_idx ON brin_insert_optimization USING brin UPDATE brin_insert_optimization SET a = a; REINDEX INDEX CONCURRENTLY brin_insert_optimization_idx; DROP TABLE brin_insert_optimization; +-- a box with a NaN coordinate must not hide the other rows of its page +-- range (bug #19705) +CREATE TABLE brin_box_nan (v box); +INSERT INTO brin_box_nan SELECT box '(0,0),(1,1)' FROM generate_series(1, 100); +INSERT INTO brin_box_nan VALUES (box '(NaN,NaN),(0,0)'); +CREATE INDEX brin_box_nan_idx ON brin_box_nan USING brin (v); +SET enable_seqscan = off; +SELECT count(*) FROM brin_box_nan WHERE v && box '(-2,-2),(2,2)'; + count +------- + 100 +(1 row) + +SELECT count(*) FROM brin_box_nan WHERE v @> point '(0.5,0.5)'; + count +------- + 100 +(1 row) + +SELECT count(*) FROM brin_box_nan WHERE v ~= box '(0,0),(1,1)'; + count +------- + 100 +(1 row) + +-- the NaN row is found too: unmergeable ranges are always scanned +SELECT count(*) FROM brin_box_nan WHERE v ~= box '(NaN,NaN),(0,0)'; + count +------- + 1 +(1 row) + +-- also when it is the only value in its range +TRUNCATE brin_box_nan; +INSERT INTO brin_box_nan VALUES (box '(NaN,NaN),(0,0)'); +REINDEX INDEX brin_box_nan_idx; +SELECT count(*) FROM brin_box_nan WHERE v ~= box '(NaN,NaN),(0,0)'; + count +------- + 1 +(1 row) + +RESET enable_seqscan; +DROP TABLE brin_box_nan; +-- An index built before box_inclusion_ops had a mergeable function can +-- hold a NaN union. Mimic one with an opclass that gets the function +-- only after the build. +CREATE OPERATOR FAMILY brin_box_nan_ops USING brin; +CREATE OPERATOR CLASS brin_box_nan_ops FOR TYPE box USING brin + FAMILY brin_box_nan_ops AS + OPERATOR 3 &&, + FUNCTION 1 brin_inclusion_opcinfo(internal), + FUNCTION 2 brin_inclusion_add_value(internal, internal, internal, internal), + FUNCTION 3 brin_inclusion_consistent(internal, internal, internal), + FUNCTION 4 brin_inclusion_union(internal, internal, internal), + FUNCTION 11 bound_box(box, box); +CREATE TABLE brin_box_nan (v box); +INSERT INTO brin_box_nan SELECT box '(0,0),(1,1)' FROM generate_series(1, 100); +INSERT INTO brin_box_nan VALUES (box '(NaN,NaN),(0,0)'); +CREATE INDEX brin_box_nan_idx ON brin_box_nan USING brin (v brin_box_nan_ops); +ALTER OPERATOR FAMILY brin_box_nan_ops USING brin + ADD FUNCTION 12 (box, box) box_mergeable(box, box); +\c - +SET enable_seqscan = off; +SELECT count(*) FROM brin_box_nan WHERE v && box '(-2,-2),(2,2)'; + count +------- + 100 +(1 row) + +RESET enable_seqscan; +DROP TABLE brin_box_nan; +DROP OPERATOR FAMILY brin_box_nan_ops USING brin; diff --git a/src/test/regress/expected/geometry.out b/src/test/regress/expected/geometry.out index 1d168b21cbc..9d03c4718a9 100644 --- a/src/test/regress/expected/geometry.out +++ b/src/test/regress/expected/geometry.out @@ -5321,3 +5321,16 @@ SELECT * FROM pg_input_error_info('(1,2),-1', 'circle'); invalid input syntax for type circle: "(1,2),-1" | | | 22P02 (1 row) +-- box_mergeable() rejects NaN coordinates (BRIN box_inclusion_ops) +SELECT box_mergeable(box '(0,0),(1,1)', box '(NaN,3),(0,2)'); + box_mergeable +--------------- + f +(1 row) + +SELECT box_mergeable(box '(0,0),(1,1)', box '(2,3),(0,2)'); + box_mergeable +--------------- + t +(1 row) + diff --git a/src/test/regress/sql/brin.sql b/src/test/regress/sql/brin.sql index 7ea97f47c8d..33bfef7a5e6 100644 --- a/src/test/regress/sql/brin.sql +++ b/src/test/regress/sql/brin.sql @@ -534,3 +534,48 @@ CREATE INDEX brin_insert_optimization_idx ON brin_insert_optimization USING brin UPDATE brin_insert_optimization SET a = a; REINDEX INDEX CONCURRENTLY brin_insert_optimization_idx; DROP TABLE brin_insert_optimization; + +-- a box with a NaN coordinate must not hide the other rows of its page +-- range (bug #19705) +CREATE TABLE brin_box_nan (v box); +INSERT INTO brin_box_nan SELECT box '(0,0),(1,1)' FROM generate_series(1, 100); +INSERT INTO brin_box_nan VALUES (box '(NaN,NaN),(0,0)'); +CREATE INDEX brin_box_nan_idx ON brin_box_nan USING brin (v); +SET enable_seqscan = off; +SELECT count(*) FROM brin_box_nan WHERE v && box '(-2,-2),(2,2)'; +SELECT count(*) FROM brin_box_nan WHERE v @> point '(0.5,0.5)'; +SELECT count(*) FROM brin_box_nan WHERE v ~= box '(0,0),(1,1)'; +-- the NaN row is found too: unmergeable ranges are always scanned +SELECT count(*) FROM brin_box_nan WHERE v ~= box '(NaN,NaN),(0,0)'; +-- also when it is the only value in its range +TRUNCATE brin_box_nan; +INSERT INTO brin_box_nan VALUES (box '(NaN,NaN),(0,0)'); +REINDEX INDEX brin_box_nan_idx; +SELECT count(*) FROM brin_box_nan WHERE v ~= box '(NaN,NaN),(0,0)'; +RESET enable_seqscan; +DROP TABLE brin_box_nan; + +-- An index built before box_inclusion_ops had a mergeable function can +-- hold a NaN union. Mimic one with an opclass that gets the function +-- only after the build. +CREATE OPERATOR FAMILY brin_box_nan_ops USING brin; +CREATE OPERATOR CLASS brin_box_nan_ops FOR TYPE box USING brin + FAMILY brin_box_nan_ops AS + OPERATOR 3 &&, + FUNCTION 1 brin_inclusion_opcinfo(internal), + FUNCTION 2 brin_inclusion_add_value(internal, internal, internal, internal), + FUNCTION 3 brin_inclusion_consistent(internal, internal, internal), + FUNCTION 4 brin_inclusion_union(internal, internal, internal), + FUNCTION 11 bound_box(box, box); +CREATE TABLE brin_box_nan (v box); +INSERT INTO brin_box_nan SELECT box '(0,0),(1,1)' FROM generate_series(1, 100); +INSERT INTO brin_box_nan VALUES (box '(NaN,NaN),(0,0)'); +CREATE INDEX brin_box_nan_idx ON brin_box_nan USING brin (v brin_box_nan_ops); +ALTER OPERATOR FAMILY brin_box_nan_ops USING brin + ADD FUNCTION 12 (box, box) box_mergeable(box, box); +\c - +SET enable_seqscan = off; +SELECT count(*) FROM brin_box_nan WHERE v && box '(-2,-2),(2,2)'; +RESET enable_seqscan; +DROP TABLE brin_box_nan; +DROP OPERATOR FAMILY brin_box_nan_ops USING brin; diff --git a/src/test/regress/sql/geometry.sql b/src/test/regress/sql/geometry.sql index c3ea368da5e..994797c4d24 100644 --- a/src/test/regress/sql/geometry.sql +++ b/src/test/regress/sql/geometry.sql @@ -529,3 +529,7 @@ SELECT pg_input_is_valid('(1', 'circle'); SELECT * FROM pg_input_error_info('1,', 'circle'); SELECT pg_input_is_valid('(1,2),-1', 'circle'); SELECT * FROM pg_input_error_info('(1,2),-1', 'circle'); + +-- box_mergeable() rejects NaN coordinates (BRIN box_inclusion_ops) +SELECT box_mergeable(box '(0,0),(1,1)', box '(NaN,3),(0,2)'); +SELECT box_mergeable(box '(0,0),(1,1)', box '(2,3),(0,2)'); -- 2.37.1 (Apple Git-137.1)