[Tomas Vondra <tomas.vondra@enterprisedb.com>]

Hi,

While working on some BRIN code, I discovered a bug in handling NULL
values - when inserting a non-NULL value into a NULL-only range, we
reset the all_nulls flag but don't update the has_nulls flag. And
because of that we then fail to return the range for IS NULL ranges.

Reproducing this is trivial:

  create table t (a int);
  create index on t using brin (a);
  insert into t values (null);
  insert into t values (1);

  set enable_seqscan  = off;
  select * from t where a is null;

This should return 1 row, but actually it returns no rows.

Attached is a patch fixing this by properly updating the has_nulls flag.

I reproduced this all the way back to 9.5, so it's a long-standing bug.
It's interesting no one noticed / reported it so far, it doesn't seem
like a particularly rare corner case.


regards

-- 
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Attachments:

brin-hasnulls-fix.patch (text/x-patch, 4.6K) download
diff --git a/src/backend/access/brin/brin_bloom.c b/src/backend/access/brin/brin_bloom.c
index 6b0af7267d5..60315450b41 100644
--- a/src/backend/access/brin/brin_bloom.c
+++ b/src/backend/access/brin/brin_bloom.c
@@ -539,6 +539,7 @@ brin_bloom_add_value(PG_FUNCTION_ARGS)
 							BloomGetFalsePositiveRate(opts));
 		column->bv_values[0] = PointerGetDatum(filter);
 		column->bv_allnulls = false;
+		column->bv_hasnulls = true;
 		updated = true;
 	}
 	else
diff --git a/src/backend/access/brin/brin_inclusion.c b/src/backend/access/brin/brin_inclusion.c
index 4b02d374f23..e0f44d3e62c 100644
--- a/src/backend/access/brin/brin_inclusion.c
+++ b/src/backend/access/brin/brin_inclusion.c
@@ -164,6 +164,7 @@ brin_inclusion_add_value(PG_FUNCTION_ARGS)
 		column->bv_values[INCLUSION_UNMERGEABLE] = BoolGetDatum(false);
 		column->bv_values[INCLUSION_CONTAINS_EMPTY] = BoolGetDatum(false);
 		column->bv_allnulls = false;
+		column->bv_hasnulls = true;
 		new = true;
 	}
 
diff --git a/src/backend/access/brin/brin_minmax.c b/src/backend/access/brin/brin_minmax.c
index 9e8a8e056cc..8a5661a8952 100644
--- a/src/backend/access/brin/brin_minmax.c
+++ b/src/backend/access/brin/brin_minmax.c
@@ -90,6 +90,7 @@ brin_minmax_add_value(PG_FUNCTION_ARGS)
 		column->bv_values[0] = datumCopy(newval, attr->attbyval, attr->attlen);
 		column->bv_values[1] = datumCopy(newval, attr->attbyval, attr->attlen);
 		column->bv_allnulls = false;
+		column->bv_hasnulls = true;
 		PG_RETURN_BOOL(true);
 	}
 
diff --git a/src/backend/access/brin/brin_minmax_multi.c b/src/backend/access/brin/brin_minmax_multi.c
index 9a0bcf6698d..4e7119e2d78 100644
--- a/src/backend/access/brin/brin_minmax_multi.c
+++ b/src/backend/access/brin/brin_minmax_multi.c
@@ -2500,6 +2500,7 @@ brin_minmax_multi_add_value(PG_FUNCTION_ARGS)
 		MemoryContextSwitchTo(oldctx);
 
 		column->bv_allnulls = false;
+		column->bv_hasnulls = true;
 		modified = true;
 
 		column->bv_mem_value = PointerGetDatum(ranges);
diff --git a/src/test/regress/expected/brin.out b/src/test/regress/expected/brin.out
index 73fa38396e4..cc896c2d9d4 100644
--- a/src/test/regress/expected/brin.out
+++ b/src/test/regress/expected/brin.out
@@ -572,3 +572,39 @@ CREATE UNLOGGED TABLE brintest_unlogged (n numrange);
 CREATE INDEX brinidx_unlogged ON brintest_unlogged USING brin (n);
 INSERT INTO brintest_unlogged VALUES (numrange(0, 2^1000::numeric));
 DROP TABLE brintest_unlogged;
+-- test that we properly update has_nulls when inserting something into
+-- a range that only had NULLs before
+CREATE TABLE brintest_4 (a INT, b INT, c INT, d INET);
+CREATE INDEX brintest_4_idx ON brintest_4 USING brin (a, b int4_minmax_multi_ops, c int4_bloom_ops, d inet_inclusion_ops);
+-- insert a NULL value, so that we get an all-nulls range
+INSERT INTO brintest_4 VALUES (NULL, NULL, NULL, NULL);
+-- now insert a non-NULL value
+INSERT INTO brintest_4 VALUES (1, 1, 1, '127.0.0.1');
+-- see that we can still match the value when using the brin index
+SET enable_seqscan = off;
+SELECT * FROM brintest_4 WHERE a IS NULL;
+ a | b | c | d 
+---+---+---+---
+   |   |   | 
+(1 row)
+
+SELECT * FROM brintest_4 WHERE b IS NULL;
+ a | b | c | d 
+---+---+---+---
+   |   |   | 
+(1 row)
+
+SELECT * FROM brintest_4 WHERE c IS NULL;
+ a | b | c | d 
+---+---+---+---
+   |   |   | 
+(1 row)
+
+SELECT * FROM brintest_4 WHERE d IS NULL;
+ a | b | c | d 
+---+---+---+---
+   |   |   | 
+(1 row)
+
+DROP TABLE brintest_4;
+SET enable_seqscan = off;
diff --git a/src/test/regress/sql/brin.sql b/src/test/regress/sql/brin.sql
index e68e9e18df5..17a01a4b82f 100644
--- a/src/test/regress/sql/brin.sql
+++ b/src/test/regress/sql/brin.sql
@@ -515,3 +515,24 @@ CREATE UNLOGGED TABLE brintest_unlogged (n numrange);
 CREATE INDEX brinidx_unlogged ON brintest_unlogged USING brin (n);
 INSERT INTO brintest_unlogged VALUES (numrange(0, 2^1000::numeric));
 DROP TABLE brintest_unlogged;
+
+-- test that we properly update has_nulls when inserting something into
+-- a range that only had NULLs before
+CREATE TABLE brintest_4 (a INT, b INT, c INT, d INET);
+CREATE INDEX brintest_4_idx ON brintest_4 USING brin (a, b int4_minmax_multi_ops, c int4_bloom_ops, d inet_inclusion_ops);
+
+-- insert a NULL value, so that we get an all-nulls range
+INSERT INTO brintest_4 VALUES (NULL, NULL, NULL, NULL);
+
+-- now insert a non-NULL value
+INSERT INTO brintest_4 VALUES (1, 1, 1, '127.0.0.1');
+
+-- see that we can still match the value when using the brin index
+SET enable_seqscan = off;
+SELECT * FROM brintest_4 WHERE a IS NULL;
+SELECT * FROM brintest_4 WHERE b IS NULL;
+SELECT * FROM brintest_4 WHERE c IS NULL;
+SELECT * FROM brintest_4 WHERE d IS NULL;
+
+DROP TABLE brintest_4;
+SET enable_seqscan = off;