Subject: [PATCH 2/8] Move IS NOT NULL checks to bringetbitmap

From: Tomas Vondra <[email-masked]>
Date: Sun, 9 Jun 2019 22:05:39 +0200

---
 src/backend/access/brin/brin.c           | 116 ++++++++++++++++++++---
 src/backend/access/brin/brin_inclusion.c |  62 +-----------
 src/backend/access/brin/brin_minmax.c    |  62 +-----------
 3 files changed, 109 insertions(+), 131 deletions(-)

diff --git a/src/backend/access/brin/brin.c b/src/backend/access/brin/brin.c
index 6777d48faf..caf7b62688 100644
--- a/src/backend/access/brin/brin.c
+++ b/src/backend/access/brin/brin.c
@@ -389,8 +389,10 @@ bringetbitmap(IndexScanDesc scan, TIDBitmap *tbm)
 	BrinMemTuple *dtup;
 	BrinTuple  *btup = NULL;
 	Size		btupsz = 0;
-	ScanKey	  **keys;
-	int		   *nkeys;
+	ScanKey	  **keys,
+			  **nullkeys;
+	int		   *nkeys,
+			   *nnullkeys;
 	int			keyno;
 
 	opaque = (BrinOpaque *) scan->opaque;
@@ -415,10 +417,13 @@ bringetbitmap(IndexScanDesc scan, TIDBitmap *tbm)
 
 	/*
 	 * Make room for per-attribute lists of scan keys that we'll pass to the
-	 * consistent support procedure.
+	 * consistent support procedure. We keep null and regular keys separate,
+	 * so that we can easily pass regular keys to the consistent function.
 	 */
 	keys = palloc0(sizeof(ScanKey *) * bdesc->bd_tupdesc->natts);
+	nullkeys = palloc0(sizeof(ScanKey *) * bdesc->bd_tupdesc->natts);
 	nkeys = palloc0(sizeof(int) * bdesc->bd_tupdesc->natts);
+	nnullkeys = palloc0(sizeof(int) * bdesc->bd_tupdesc->natts);
 
 	/*
 	 * Preprocess the scan keys - split them into per-attribute arrays.
@@ -440,14 +445,12 @@ bringetbitmap(IndexScanDesc scan, TIDBitmap *tbm)
 							  keyattno - 1)->attcollation));
 
 		/* First time we see this index attribute, so init as needed. */
-		if (!keys[keyattno-1])
+		if (consistentFn[keyattno - 1].fn_oid == InvalidOid)
 		{
 			FmgrInfo   *tmp;
 
-			keys[keyattno-1] = palloc0(sizeof(ScanKey) * scan->numberOfKeys);
-
-			/* First time this column, so look up consistent function */
-			Assert(consistentFn[keyattno - 1].fn_oid == InvalidOid);
+			Assert((keys[keyattno - 1] == NULL) && (nkeys[keyattno - 1] == 0));
+			Assert((nullkeys[keyattno - 1] == NULL) && (nnullkeys[keyattno - 1] == 0));
 
 			tmp = index_getprocinfo(idxRel, keyattno,
 									BRIN_PROCNUM_CONSISTENT);
@@ -455,9 +458,23 @@ bringetbitmap(IndexScanDesc scan, TIDBitmap *tbm)
 						   CurrentMemoryContext);
 		}
 
-		/* Add key to the per-attribute array. */
-		keys[keyattno - 1][nkeys[keyattno - 1]] = key;
-		nkeys[keyattno - 1]++;
+		/* Add key to the proper per-attribute array. */
+		if (key->sk_flags & SK_ISNULL)
+		{
+			if (!nullkeys[keyattno - 1])
+				nullkeys[keyattno - 1] = palloc0(sizeof(ScanKey) * scan->numberOfKeys);
+
+			nullkeys[keyattno - 1][nnullkeys[keyattno - 1]] = key;
+			nnullkeys[keyattno - 1]++;
+		}
+		else
+		{
+			if (!keys[keyattno - 1])
+				keys[keyattno - 1] = palloc0(sizeof(ScanKey) * scan->numberOfKeys);
+
+			keys[keyattno - 1][nkeys[keyattno - 1]] = key;
+			nkeys[keyattno - 1]++;
+		}
 	}
 
 	/* allocate an initial in-memory tuple, out of the per-range memcxt */
@@ -544,6 +561,83 @@ bringetbitmap(IndexScanDesc scan, TIDBitmap *tbm)
 					Assert((nkeys[attno - 1] > 0) &&
 						   (nkeys[attno - 1] <= scan->numberOfKeys));
 
+					/*
+					 * First check if there are any IS [NOT] NULL scan keys, and
+					 * if we're violating them. In that case we can terminate
+					 * early, without invoking the support function.
+					 *
+					 * As there may be more keys, we can only detemine mismatch
+					 * within this loop.
+					 */
+					for (keyno = 0; (keyno < nnullkeys[attno - 1]); keyno++)
+					{
+						ScanKey	key = nullkeys[attno - 1][keyno];
+
+						Assert(key->sk_attno == bval->bv_attno);
+
+						/* interrupt the loop as soon as we find a mismatch */
+						if (!addrange)
+							break;
+
+						/* handle IS NULL/IS NOT NULL tests */
+						if (key->sk_flags & SK_ISNULL)
+						{
+							/* IS NULL scan key, but range has no NULLs */
+							if (key->sk_flags & SK_SEARCHNULL)
+							{
+								if (!bval->bv_allnulls && !bval->bv_hasnulls)
+									addrange = false;
+
+								continue;
+							}
+
+							/*
+							 * For IS NOT NULL, we can only skip ranges that are
+							 * known to have only nulls.
+							 */
+							if (key->sk_flags & SK_SEARCHNOTNULL)
+							{
+								if (bval->bv_allnulls)
+									addrange = false;
+
+								continue;
+							}
+
+							/*
+							 * Neither IS NULL nor IS NOT NULL was used; assume all
+							 * indexable operators are strict and thus return false
+							 * with NULL value in the scan key.
+							 */
+							addrange = false;
+						}
+					}
+
+					/*
+					 * If any of the IS [NOT] NULL keys failed, the page range as
+					 * a whole can't pass. So terminate the loop.
+					 */
+					if (!addrange)
+						break;
+
+					/*
+					 * So either there are no IS [NOT] NULL keys, or all passed. If
+					 * there are no regular scan keys, we're done - the page range
+					 * matches. If there are regular keys, but the page range is
+					 * marked as 'all nulls' it can't possibly pass (we're assuming
+					 * the operators are strict).
+					 */
+
+					/* No regular scan keys - page range as a whole passes. */
+					if (!nkeys[attno - 1])
+						continue;
+
+					/* If it is all nulls, it cannot possibly be consistent. */
+					if (bval->bv_allnulls)
+					{
+						addrange = false;
+						break;
+					}
+
 					/*
 					 * Check whether the scan key is consistent with the page
 					 * range values; if so, have the pages in the range added
diff --git a/src/backend/access/brin/brin_inclusion.c b/src/backend/access/brin/brin_inclusion.c
index 8968886ff5..22edc6b46f 100644
--- a/src/backend/access/brin/brin_inclusion.c
+++ b/src/backend/access/brin/brin_inclusion.c
@@ -265,63 +265,6 @@ brin_inclusion_consistent(PG_FUNCTION_ARGS)
 	Oid			colloid = PG_GET_COLLATION();
 	int			keyno;
 	bool		matches;
-	bool		regular_keys = false;
-
-	/*
-	 * First check if there are any IS NULL scan keys, and if we're
-	 * violating them. In that case we can terminate early, without
-	 * inspecting the ranges.
-	 */
-	for (keyno = 0; keyno < nkeys; keyno++)
-	{
-		ScanKey	key = keys[keyno];
-
-		Assert(key->sk_attno == column->bv_attno);
-
-		/* handle IS NULL/IS NOT NULL tests */
-		if (key->sk_flags & SK_ISNULL)
-		{
-			if (key->sk_flags & SK_SEARCHNULL)
-			{
-				if (column->bv_allnulls || column->bv_hasnulls)
-					continue;	/* this key is fine, continue */
-
-				PG_RETURN_BOOL(false);
-			}
-
-			/*
-			 * For IS NOT NULL, we can only skip ranges that are known to have
-			 * only nulls.
-			 */
-			if (key->sk_flags & SK_SEARCHNOTNULL)
-			{
-				if (column->bv_allnulls)
-					PG_RETURN_BOOL(false);
-
-				continue;
-			}
-
-			/*
-			 * Neither IS NULL nor IS NOT NULL was used; assume all indexable
-			 * operators are strict and return false.
-			 */
-			PG_RETURN_BOOL(false);
-		}
-		else
-			/* note we have regular (non-NULL) scan keys */
-			regular_keys = true;
-	}
-
-	/*
-	 * If the page range is all nulls, it cannot possibly be consistent if
-	 * there are some regular scan keys.
-	 */
-	if (column->bv_allnulls && regular_keys)
-		PG_RETURN_BOOL(false);
-
-	/* If there are no regular keys, the page range is considered consistent. */
-	if (!regular_keys)
-		PG_RETURN_BOOL(true);
 
 	/* It has to be checked, if it contains elements that are not mergeable. */
 	if (DatumGetBool(column->bv_values[INCLUSION_UNMERGEABLE]))
@@ -333,9 +276,8 @@ brin_inclusion_consistent(PG_FUNCTION_ARGS)
 	{
 		ScanKey		key = keys[keyno];
 
-		/* ignore IS NULL/IS NOT NULL tests handled above */
-		if (key->sk_flags & SK_ISNULL)
-			continue;
+		/* NULL keys are handled and filtered-out in bringetbitmap */
+		Assert(!(key->sk_flags & SK_ISNULL));
 
 		matches = inclusion_consistent_key(bdesc, column, key, colloid);
 
diff --git a/src/backend/access/brin/brin_minmax.c b/src/backend/access/brin/brin_minmax.c
index 1219a3a2ab..7a7bd21cec 100644
--- a/src/backend/access/brin/brin_minmax.c
+++ b/src/backend/access/brin/brin_minmax.c
@@ -153,63 +153,6 @@ brin_minmax_consistent(PG_FUNCTION_ARGS)
 	Oid			colloid = PG_GET_COLLATION();
 	Datum		matches;
 	int			keyno;
-	bool		regular_keys = false;
-
-	/*
-	 * First check if there are any IS NULL scan keys, and if we're
-	 * violating them. In that case we can terminate early, without
-	 * inspecting the ranges.
-	 */
-	for (keyno = 0; keyno < nkeys; keyno++)
-	{
-		ScanKey	key = keys[keyno];
-
-		Assert(key->sk_attno == column->bv_attno);
-
-		/* handle IS NULL/IS NOT NULL tests */
-		if (key->sk_flags & SK_ISNULL)
-		{
-			if (key->sk_flags & SK_SEARCHNULL)
-			{
-				if (column->bv_allnulls || column->bv_hasnulls)
-					continue;	/* this key is fine, continue */
-
-				PG_RETURN_BOOL(false);
-			}
-
-			/*
-			 * For IS NOT NULL, we can only skip ranges that are known to have
-			 * only nulls.
-			 */
-			if (key->sk_flags & SK_SEARCHNOTNULL)
-			{
-				if (column->bv_allnulls)
-					PG_RETURN_BOOL(false);
-
-				continue;
-			}
-
-			/*
-			 * Neither IS NULL nor IS NOT NULL was used; assume all indexable
-			 * operators are strict and return false.
-			 */
-			PG_RETURN_BOOL(false);
-		}
-		else
-			/* note we have regular (non-NULL) scan keys */
-			regular_keys = true;
-	}
-
-	/*
-	 * If the page range is all nulls, it cannot possibly be consistent if
-	 * there are some regular scan keys.
-	 */
-	if (column->bv_allnulls && regular_keys)
-		PG_RETURN_BOOL(false);
-
-	/* If there are no regular keys, the page range is considered consistent. */
-	if (!regular_keys)
-		PG_RETURN_BOOL(true);
 
 	matches = true;
 
@@ -217,9 +160,8 @@ brin_minmax_consistent(PG_FUNCTION_ARGS)
 	{
 		ScanKey	key = keys[keyno];
 
-		/* ignore IS NULL/IS NOT NULL tests handled above */
-		if (key->sk_flags & SK_ISNULL)
-			continue;
+		/* NULL keys are handled and filtered-out in bringetbitmap */
+		Assert(!(key->sk_flags & SK_ISNULL));
 
 		matches = minmax_consistent_key(bdesc, column, key, colloid);
 
-- 
2.25.4

--vurodj3csurzcvv3
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment;
	filename="0003-Move-processing-of-NULLs-from-BRIN-support--20200906.patch"