Fix btree mark/restore bug.

Commit 2ed5b87f96d473962ec5230fd820abfeaccb2069 introduced a bug in
mark/restore, in an attempt to optimize repeated restores to the
same page.  This caused an assertion failure during a merge join
which fed directly from an index scan, although the impact would
not be limited to that case.  Revert the bad chunk of code from
that commit.

While investigating this bug it was discovered that a particular
"paranoia" set of the mark position field would not prevent bad
behavior; it would just make it harder to diagnose.  Change that
into an assertion, which will draw attention to any future problem
in that area more directly.

Backpatch to 9.5, where the bug was introduced.

Bug #14169 reported by Shinta Koyanagi.
Preliminary analysis by Tom Lane identified which commit caused
the bug.

diff --git a/src/backend/access/nbtree/nbtree.c b/src/backend/access/nbtree/nbtree.c
index af24f6713099a144d70a0ee6a56335663dcb9285..3a7942997c2dcffb5476df4c57c3d4d59c500965 100644 (file)
--- a/src/backend/access/nbtree/nbtree.c
+++ b/src/backend/access/nbtree/nbtree.c
@@ -608,25 +608,6 @@ btrestrpos(IndexScanDesc scan)
                 */
                so->currPos.itemIndex = so->markItemIndex;
        }
-       else if (so->currPos.currPage == so->markPos.currPage)
-       {
-               /*
-                * so->markItemIndex < 0 but mark and current positions are on the
-                * same page.  This would be an unusual case, where the scan moved to
-                * a new index page after the mark, restored, and later restored again
-                * without moving off the marked page.  It is not clear that this code
-                * can currently be reached, but it seems better to make this function
-                * robust for this case than to Assert() or elog() that it can't
-                * happen.
-                *
-                * We neither want to set so->markItemIndex >= 0 (because that could
-                * cause a later move to a new page to redo the memcpy() executions)
-                * nor re-execute the memcpy() functions for a restore within the same
-                * page.  The previous restore to this page already set everything
-                * except markPos as it should be.
-                */
-               so->currPos.itemIndex = so->markPos.itemIndex;
-       }
        else
        {
                /*

diff --git a/src/backend/access/nbtree/nbtsearch.c b/src/backend/access/nbtree/nbtsearch.c
index c392fd6cb319723202871fa396d0c59a55d426e4..ee46023c5a66a65735f1df38e0d20fddd0019bae 100644 (file)
--- a/src/backend/access/nbtree/nbtsearch.c
+++ b/src/backend/access/nbtree/nbtsearch.c
@@ -1011,7 +1011,7 @@ _bt_first(IndexScanDesc scan, ScanDirection dir)
                so->currPos.moreRight = false;
        }
        so->numKilled = 0;                      /* just paranoia */
-       so->markItemIndex = -1;         /* ditto */
+       Assert(so->markItemIndex == -1);
 
        /* position to the precise item on the page */
        offnum = _bt_binsrch(rel, buf, keysCount, scankeys, nextkey);