Date: Tue, 28 Jul 2015 15:39:42 -0300
From: Alvaro Herrera <alvherre@2ndquadrant.com>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Andreas Seltenreich <seltenreich@gmx.de>, pgsql-hackers@postgresql.org
Subject: Re: Failing assertions in indxpath.c, placeholder.c and brin_minmax.c
Message-ID: <20150728183942.GA2441@postgresql.org>
References: <871tfv3wor.fsf@ex.ansel.ydns.eu> <20646.1437919632@sss.pgh.pa.us>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="nFreZHaLTZJo0R7j"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20646.1437919632@sss.pgh.pa.us>
User-Agent: Mutt/1.5.23 (2014-03-12)
Precedence: bulk
Sender: pgsql-hackers-owner@postgresql.org


--nFreZHaLTZJo0R7j
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit

Tom Lane wrote:

> Bottom line is that somebody failed to consider the possibility of a
> null comparison value reaching the BRIN index lookup machinery.
> The code stanza that's failing supposes that only IS NULL or IS NOT NULL
> tests could have SK_ISNULL set, but that's just wrong.

I think the easiest way to solve this is to consider that all indexable
operators are strict, and have the function return false in that case.
The attached patch implements that.  (In a quick check, the only
non-strict operator in the regression database is <%(point,widget),
which seems okay to ignore given that the type itself is only part of
pg_regress.  I wonder what would happen if the regression tests defined
an index using that operator.)

What btree actually does is precompute a "qual_ok" property at
scan-restart time, which seems pretty clever (maybe too much).  I think
something like _bt_preprocess_keys should probably be applied to BRIN
scans someday.

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

--nFreZHaLTZJo0R7j
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="brin-nulls.patch"

diff --git a/src/backend/access/brin/brin_inclusion.c b/src/backend/access/brin/brin_inclusion.c
index 803b07f..926487e 100644
--- a/src/backend/access/brin/brin_inclusion.c
+++ b/src/backend/access/brin/brin_inclusion.c
@@ -276,8 +276,14 @@ brin_inclusion_consistent(PG_FUNCTION_ARGS)
 		 * For IS NOT NULL, we can only skip ranges that are known to have
 		 * only nulls.
 		 */
-		Assert(key->sk_flags & SK_SEARCHNOTNULL);
-		PG_RETURN_BOOL(!column->bv_allnulls);
+		if (key->sk_flags & SK_SEARCHNOTNULL)
+			PG_RETURN_BOOL(!column->bv_allnulls);
+
+		/*
+		 * Neither IS NULL nor IS NOT NULL was used; assume all indexable
+		 * operators are strict and return false.
+		 */
+		PG_RETURN_BOOL(false);
 	}
 
 	/* If it is all nulls, it cannot possibly be consistent. */
diff --git a/src/backend/access/brin/brin_minmax.c b/src/backend/access/brin/brin_minmax.c
index 7cd9888..2cc6e41 100644
--- a/src/backend/access/brin/brin_minmax.c
+++ b/src/backend/access/brin/brin_minmax.c
@@ -174,8 +174,14 @@ brin_minmax_consistent(PG_FUNCTION_ARGS)
 		 * For IS NOT NULL, we can only skip ranges that are known to have
 		 * only nulls.
 		 */
-		Assert(key->sk_flags & SK_SEARCHNOTNULL);
-		PG_RETURN_BOOL(!column->bv_allnulls);
+		if (key->sk_flags & SK_SEARCHNOTNULL)
+			PG_RETURN_BOOL(!column->bv_allnulls);
+
+		/*
+		 * Neither IS NULL nor IS NOT NULL was used; assume all indexable
+		 * operators are strict and return false.
+		 */
+		PG_RETURN_BOOL(false);
 	}
 
 	/* if the range is all empty, it cannot possibly be consistent */

--nFreZHaLTZJo0R7j
Content-Type: text/plain
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

--nFreZHaLTZJo0R7j--