Fix btree stop-at-nulls logic properly.

As pointed out by Naoya Anzai, my previous try at this was a few bricks
shy of a load, because I had forgotten that the initial-positioning logic
might not try to skip over nulls at the end of the index the scan will
start from.  We ought to fix that, because it represents an unnecessary
inefficiency, but first let's get the scan-stop logic back to a safe
state.  With this patch, we preserve the performance benefit requested
in bug #6278 for the case of scanning forward into NULLs (in a NULLS
LAST index), but the reverse case of scanning backward across NULLs
when there's no suitable initial-positioning qual is still inefficient.

diff --git a/src/backend/access/nbtree/nbtutils.c b/src/backend/access/nbtree/nbtutils.c
index c00255567116e3b6f7bd8426eb32b2398c8d95d5..bb4d07368ff0fe97141abb10d1689327c073202e 100644 (file)
--- a/src/backend/access/nbtree/nbtutils.c
+++ b/src/backend/access/nbtree/nbtutils.c
@@ -609,7 +609,7 @@ _bt_advance_array_keys(IndexScanDesc scan, ScanDirection dir)
  * so that the index sorts in the desired direction.
  *
  * One key purpose of this routine is to discover which scan keys must be
- * satisfied to continue the scan. It also attempts to eliminate redundant
+ * satisfied to continue the scan.  It also attempts to eliminate redundant
  * keys and detect contradictory keys.  (If the index opfamily provides
  * incomplete sets of cross-type operators, we may fail to detect redundant
  * or contradictory keys, but we can survive that.)
@@ -647,7 +647,7 @@ _bt_advance_array_keys(IndexScanDesc scan, ScanDirection dir)
  * </<= keys if we can't compare them.  The logic about required keys still
  * works if we don't eliminate redundant keys.
  *
- * Note that the reason we need direction-sensitive required-key flags is
+ * Note that one reason we need direction-sensitive required-key flags is
  * precisely that we may not be able to eliminate redundant keys.  Suppose
  * we have "x > 4::int AND x > 10::bigint", and we are unable to determine
  * which key is more restrictive for lack of a suitable cross-type operator.
@@ -655,7 +655,10 @@ _bt_advance_array_keys(IndexScanDesc scan, ScanDirection dir)
  * positioning with.  If it picks x > 4, then the x > 10 condition will fail
  * until we reach index entries > 10; but we can't stop the scan just because
  * x > 10 is failing.  On the other hand, if we are scanning backwards, then
- * failure of either key is indeed enough to stop the scan.
+ * failure of either key is indeed enough to stop the scan.  (In general, when
+ * inequality keys are present, the initial-positioning code only promises to
+ * position before the first possible match, not exactly at the first match,
+ * for a forward scan; or after the last match for a backward scan.)
  *
  * As a byproduct of this work, we can detect contradictory quals such
  * as "x = 1 AND x > 2".  If we see that, we return so->qual_ok = FALSE,
@@ -1394,16 +1397,15 @@ _bt_checkkeys(IndexScanDesc scan,
                        }
 
                        /*
-                        * Tuple fails this qual.  If it's a required qual, then we can
-                        * conclude no further tuples will pass, either.  We can stop
-                        * regardless of the scan direction, because we know that NULLs
-                        * sort to one end or the other of the range of values.  If this
-                        * tuple doesn't pass, then no future ones will either, until we
-                        * reach the next set of values of the higher-order index attrs
-                        * (if any) ... and those attrs must have equality quals, else
-                        * this one wouldn't be marked required.
+                        * Tuple fails this qual.  If it's a required qual for the current
+                        * scan direction, then we can conclude no further tuples will
+                        * pass, either.
                         */
-                       if (key->sk_flags & (SK_BT_REQFWD | SK_BT_REQBKWD))
+                       if ((key->sk_flags & SK_BT_REQFWD) &&
+                               ScanDirectionIsForward(dir))
+                               *continuescan = false;
+                       else if ((key->sk_flags & SK_BT_REQBKWD) &&
+                                        ScanDirectionIsBackward(dir))
                                *continuescan = false;
 
                        /*
@@ -1414,15 +1416,38 @@ _bt_checkkeys(IndexScanDesc scan,
 
                if (isNull)
                {
-                       /*
-                        * The index entry is NULL, so it must fail this qual (we assume
-                        * all btree operators are strict).  Furthermore, we know that
-                        * all remaining entries with the same higher-order index attr
-                        * values must be NULLs too.  So, just as above, we can stop the
-                        * scan regardless of direction, if the qual is required.
-                        */
-                       if (key->sk_flags & (SK_BT_REQFWD | SK_BT_REQBKWD))
-                               *continuescan = false;
+                       if (key->sk_flags & SK_BT_NULLS_FIRST)
+                       {
+                               /*
+                                * Since NULLs are sorted before non-NULLs, we know we have
+                                * reached the lower limit of the range of values for this
+                                * index attr.  On a backward scan, we can stop if this qual
+                                * is one of the "must match" subset.  We can stop regardless
+                                * of whether the qual is > or <, so long as it's required,
+                                * because it's not possible for any future tuples to pass.
+                                * On a forward scan, however, we must keep going, because we
+                                * may have initially positioned to the start of the index.
+                                */
+                               if ((key->sk_flags & (SK_BT_REQFWD | SK_BT_REQBKWD)) &&
+                                       ScanDirectionIsBackward(dir))
+                                       *continuescan = false;
+                       }
+                       else
+                       {
+                               /*
+                                * Since NULLs are sorted after non-NULLs, we know we have
+                                * reached the upper limit of the range of values for this
+                                * index attr.  On a forward scan, we can stop if this qual is
+                                * one of the "must match" subset.  We can stop regardless of
+                                * whether the qual is > or <, so long as it's required,
+                                * because it's not possible for any future tuples to pass.
+                                * On a backward scan, however, we must keep going, because we
+                                * may have initially positioned to the end of the index.
+                                */
+                               if ((key->sk_flags & (SK_BT_REQFWD | SK_BT_REQBKWD)) &&
+                                       ScanDirectionIsForward(dir))
+                                       *continuescan = false;
+                       }
 
                        /*
                         * In any case, this indextuple doesn't match the qual.
@@ -1502,15 +1527,38 @@ _bt_check_rowcompare(ScanKey skey, IndexTuple tuple, TupleDesc tupdesc,
 
                if (isNull)
                {
-                       /*
-                        * The index entry is NULL, so it must fail this qual (we assume
-                        * all btree operators are strict).  Furthermore, we know that
-                        * all remaining entries with the same higher-order index attr
-                        * values must be NULLs too.  So, just as above, we can stop the
-                        * scan regardless of direction, if the qual is required.
-                        */
-                       if (subkey->sk_flags & (SK_BT_REQFWD | SK_BT_REQBKWD))
-                               *continuescan = false;
+                       if (subkey->sk_flags & SK_BT_NULLS_FIRST)
+                       {
+                               /*
+                                * Since NULLs are sorted before non-NULLs, we know we have
+                                * reached the lower limit of the range of values for this
+                                * index attr.  On a backward scan, we can stop if this qual
+                                * is one of the "must match" subset.  We can stop regardless
+                                * of whether the qual is > or <, so long as it's required,
+                                * because it's not possible for any future tuples to pass.
+                                * On a forward scan, however, we must keep going, because we
+                                * may have initially positioned to the start of the index.
+                                */
+                               if ((subkey->sk_flags & (SK_BT_REQFWD | SK_BT_REQBKWD)) &&
+                                       ScanDirectionIsBackward(dir))
+                                       *continuescan = false;
+                       }
+                       else
+                       {
+                               /*
+                                * Since NULLs are sorted after non-NULLs, we know we have
+                                * reached the upper limit of the range of values for this
+                                * index attr.  On a forward scan, we can stop if this qual is
+                                * one of the "must match" subset.  We can stop regardless of
+                                * whether the qual is > or <, so long as it's required,
+                                * because it's not possible for any future tuples to pass.
+                                * On a backward scan, however, we must keep going, because we
+                                * may have initially positioned to the end of the index.
+                                */
+                               if ((subkey->sk_flags & (SK_BT_REQFWD | SK_BT_REQBKWD)) &&
+                                       ScanDirectionIsForward(dir))
+                                       *continuescan = false;
+                       }
 
                        /*
                         * In any case, this indextuple doesn't match the qual.

diff --git a/src/test/regress/expected/create_index.out b/src/test/regress/expected/create_index.out
index b23c712204e2052005db2a8f8da88d81e0baf340..bdd1f4ec78e2a98f1c09ee4bc9e1f509bf8b5bc2 100644 (file)
--- a/src/test/regress/expected/create_index.out
+++ b/src/test/regress/expected/create_index.out
@@ -1405,6 +1405,60 @@ SELECT count(*) FROM onek_with_null WHERE unique1 IS NULL AND unique1 > 500;
      0
 (1 row)
 
+DROP INDEX onek_nulltest;
+-- Check initial-positioning logic too
+CREATE UNIQUE INDEX onek_nulltest ON onek_with_null (unique2);
+SET enable_seqscan = OFF;
+SET enable_indexscan = ON;
+SET enable_bitmapscan = OFF;
+SELECT unique1, unique2 FROM onek_with_null
+  ORDER BY unique2 LIMIT 2;
+ unique1 | unique2 
+---------+---------
+         |      -1
+     147 |       0
+(2 rows)
+
+SELECT unique1, unique2 FROM onek_with_null WHERE unique2 >= -1
+  ORDER BY unique2 LIMIT 2;
+ unique1 | unique2 
+---------+---------
+         |      -1
+     147 |       0
+(2 rows)
+
+SELECT unique1, unique2 FROM onek_with_null WHERE unique2 >= 0
+  ORDER BY unique2 LIMIT 2;
+ unique1 | unique2 
+---------+---------
+     147 |       0
+     931 |       1
+(2 rows)
+
+SELECT unique1, unique2 FROM onek_with_null
+  ORDER BY unique2 DESC LIMIT 2;
+ unique1 | unique2 
+---------+---------
+         |        
+     278 |     999
+(2 rows)
+
+SELECT unique1, unique2 FROM onek_with_null WHERE unique2 >= -1
+  ORDER BY unique2 DESC LIMIT 2;
+ unique1 | unique2 
+---------+---------
+     278 |     999
+       0 |     998
+(2 rows)
+
+SELECT unique1, unique2 FROM onek_with_null WHERE unique2 < 999
+  ORDER BY unique2 DESC LIMIT 2;
+ unique1 | unique2 
+---------+---------
+       0 |     998
+     744 |     997
+(2 rows)
+
 RESET enable_seqscan;
 RESET enable_indexscan;
 RESET enable_bitmapscan;

diff --git a/src/test/regress/sql/create_index.sql b/src/test/regress/sql/create_index.sql
index bf27379f5918e112da8546b2b9472b131ef5cd31..85cf23ccb8f1868486a12634af141a35db9b7db0 100644 (file)
--- a/src/test/regress/sql/create_index.sql
+++ b/src/test/regress/sql/create_index.sql
@@ -487,6 +487,30 @@ SELECT count(*) FROM onek_with_null WHERE unique1 IS NULL AND unique2 IS NOT NUL
 SELECT count(*) FROM onek_with_null WHERE unique1 IS NOT NULL AND unique1 > 500;
 SELECT count(*) FROM onek_with_null WHERE unique1 IS NULL AND unique1 > 500;
 
+DROP INDEX onek_nulltest;
+
+-- Check initial-positioning logic too
+
+CREATE UNIQUE INDEX onek_nulltest ON onek_with_null (unique2);
+
+SET enable_seqscan = OFF;
+SET enable_indexscan = ON;
+SET enable_bitmapscan = OFF;
+
+SELECT unique1, unique2 FROM onek_with_null
+  ORDER BY unique2 LIMIT 2;
+SELECT unique1, unique2 FROM onek_with_null WHERE unique2 >= -1
+  ORDER BY unique2 LIMIT 2;
+SELECT unique1, unique2 FROM onek_with_null WHERE unique2 >= 0
+  ORDER BY unique2 LIMIT 2;
+
+SELECT unique1, unique2 FROM onek_with_null
+  ORDER BY unique2 DESC LIMIT 2;
+SELECT unique1, unique2 FROM onek_with_null WHERE unique2 >= -1
+  ORDER BY unique2 DESC LIMIT 2;
+SELECT unique1, unique2 FROM onek_with_null WHERE unique2 < 999
+  ORDER BY unique2 DESC LIMIT 2;
+
 RESET enable_seqscan;
 RESET enable_indexscan;
 RESET enable_bitmapscan;