database/postgres
1
0
Fork 0
mirror of https://github.com/postgres/postgres.git synced 2026-10-23 13:29:09 +03:00
Code Activity

Fix hash index scans that start during a bucket split.

Browse Source 
A hash index scan whose target bucket is still being populated by a
split must read both buckets of the split: forward scans read the bucket
being populated (skipping the tuples that the split copied there) and
then the bucket being split, and backward scans read them in the
opposite order.  _hash_readnext and _hash_readprev make the transition
from the first bucket to the second, but _hash_next did not: on reaching
the end of the first bucket, it ended the scan, so every matching tuple
in the second bucket was overlooked.

To fix, teach _hash_next to make the same transition, using helper
functions shared with _hash_readnext and _hash_readprev.

_hash_next only missed the transition when the last page that it read in
the first bucket had a match (otherwise _hash_readpage's loop made the
required transition).  Getting wrong answers also needed a matching
tuple to be inserted into the bucket being populated _after_ the split
began.  Forward scans then lost the older tuples in the bucket being
split.  An incomplete split would keep the buckets in this state until
an inserter finished the split at some indeterminate later time.  Note
that wrong answers didn't require an incomplete split.

Although this fix treats the bug as the omission of a required step in
_hash_next, the real problem is the way that page stepping works in
general.  The logic for stepping to the next page appears both in
high-level routines like _hash_next and in low-level routines like
_hash_readnext and _hash_readprev.  If anything, _hash_next already knew
too much about page stepping (cf. nbtree's _bt_next function).  All of
the logic around stepping really belongs in one routine, modeled on
nbtree's _bt_steppage function, but that approach is much too invasive
for a backpatched fix.

Claude Code found this problem.  The committed test case is a simplified
version of the one that it wrote to demonstrate this bug.

Oversight in commit 7c75ef57, which introduced page-at-a-time processing
of hash index scans.

Author: Peter Geoghegan <pg@bowt.ie>
Author: Amit Kapila <amit.kapila16@gmail.com>
Reviewed-by: Vignesh C <vignesh21@gmail.com>
Discussion: https://postgr.es/m/CAH2-Wz=4CJK9ysZzbzxBGGTkgAg9ib8cJyCx=9q_d1r8tf0ang@mail.gmail.com
Backpatch-through: 14
This commit is contained in:
Peter Geoghegan
2026-10-10 10:26:18 -04:00
parent 76a6c5da40
commit 22be34ac1a
1 changed files with 101 additions and 38 deletions
Download Patch File Download Diff File Expand all files Collapse all files

139
src/backend/access/hash/hashsearch.c
View File

@@ -21,14 +21,18 @@
#include "storage/predicate.h"
#include "utils/rel.h"
static void _hash_readnext(IndexScanDesc scan, Buffer *bufp,
Page *pagep, HashPageOpaque *opaquep);
static void _hash_readprev(IndexScanDesc scan, Buffer *bufp,
Page *pagep, HashPageOpaque *opaquep);
static Buffer _hash_step_to_split_bucket(IndexScanDesc scan);
static Buffer _hash_step_to_populated_bucket(IndexScanDesc scan);
static bool _hash_readpage(IndexScanDesc scan, Buffer *bufP,
ScanDirection dir);
static int _hash_load_qualified_items(IndexScanDesc scan, Page page,
OffsetNumber offnum, ScanDirection dir);
static inline void _hash_saveitem(HashScanOpaque so, int itemIndex,
OffsetNumber offnum, IndexTuple itup);
static void _hash_readnext(IndexScanDesc scan, Buffer *bufp,
Page *pagep, HashPageOpaque *opaquep);
/*
* _hash_next() -- Get the next item in a scan.
@@ -75,6 +79,14 @@ _hash_next(IndexScanDesc scan, ScanDirection dir)
if (!_hash_readpage(scan, &buf, dir))
end_of_scan = true;
}
else if (so->hashso_buc_populated && !so->hashso_buc_split)
{
buf = _hash_step_to_split_bucket(scan);
TestForOldSnapshot(scan->xs_snapshot, rel, BufferGetPage(buf));
if (!_hash_readpage(scan, &buf, dir))
end_of_scan = true;
}
else
end_of_scan = true;
}
@@ -105,6 +117,13 @@ _hash_next(IndexScanDesc scan, ScanDirection dir)
if (!_hash_readpage(scan, &buf, dir))
end_of_scan = true;
}
else if (so->hashso_buc_populated && so->hashso_buc_split)
{
buf = _hash_step_to_populated_bucket(scan);
if (!_hash_readpage(scan, &buf, dir))
end_of_scan = true;
}
else
end_of_scan = true;
}
@@ -112,6 +131,14 @@ _hash_next(IndexScanDesc scan, ScanDirection dir)
if (end_of_scan)
{
/*
* A scan that started during a bucket split ends in the bucket that
* its direction visits last: forward scans end in the bucket being
* split, backward scans in the bucket being populated.
*/
Assert(!so->hashso_buc_populated ||
so->hashso_buc_split == ScanDirectionIsForward(dir));
_hash_dropscanbuf(rel, so);
HashScanPosInvalidate(so->currPos);
return false;
@@ -163,23 +190,7 @@ _hash_readnext(IndexScanDesc scan,
* end of bucket, scan bucket being split if there was a split in
* progress at the start of scan.
*/
*bufp = so->hashso_split_bucket_buf;
/*
* buffer for bucket being split must be valid as we acquire the pin
* on it before the start of scan and retain it till end of scan.
*/
Assert(BufferIsValid(*bufp));
LockBuffer(*bufp, BUFFER_LOCK_SHARE);
PredicateLockPage(rel, BufferGetBlockNumber(*bufp), scan->xs_snapshot);
/*
* setting hashso_buc_split to true indicates that we are scanning
* bucket being split.
*/
so->hashso_buc_split = true;
*bufp = _hash_step_to_split_bucket(scan);
block_found = true;
}
@@ -248,30 +259,82 @@ _hash_readprev(IndexScanDesc scan,
* end of bucket, scan bucket being populated if there was a split in
* progress at the start of scan.
*/
*bufp = so->hashso_bucket_buf;
/*
* buffer for bucket being populated must be valid as we acquire the
* pin on it before the start of scan and retain it till end of scan.
*/
Assert(BufferIsValid(*bufp));
LockBuffer(*bufp, BUFFER_LOCK_SHARE);
*bufp = _hash_step_to_populated_bucket(scan);
*pagep = BufferGetPage(*bufp);
*opaquep = (HashPageOpaque) PageGetSpecialPointer(*pagep);
/* move to the end of bucket chain */
while (BlockNumberIsValid((*opaquep)->hasho_nextblkno))
_hash_readnext(scan, bufp, pagep, opaquep);
/*
* setting hashso_buc_split to false indicates that we are scanning
* bucket being populated.
*/
so->hashso_buc_split = false;
}
}
/*
* Cross over from the bucket being populated to the bucket being split, on
* whose primary page we have held a pin since _hash_first. Called on
* reaching the end of the populated bucket's chain while moving forward
* through it.
*
* Returns the split bucket's primary page, pinned and share-locked, and
* sets hashso_buc_split to indicate that we are now scanning that bucket.
*/
static Buffer
_hash_step_to_split_bucket(IndexScanDesc scan)
{
Relation rel = scan->indexRelation;
HashScanOpaque so = (HashScanOpaque) scan->opaque;
Buffer buf = so->hashso_split_bucket_buf;
/*
* buffer for bucket being split must be valid as we acquire the pin on it
* before the start of scan and retain it till end of scan.
*/
Assert(BufferIsValid(buf));
LockBuffer(buf, BUFFER_LOCK_SHARE);
PredicateLockPage(rel, BufferGetBlockNumber(buf), scan->xs_snapshot);
so->hashso_buc_split = true;
return buf;
}
/*
* Cross over from the bucket being split back to the bucket being
* populated, on whose primary page we have held a pin since _hash_first,
* and walk to the end of its chain (backward scans read chains tail to
* head). Called on reaching the start of the split bucket's chain while
* moving backward through it.
*
* Returns the last page in the populated bucket's chain, pinned and
* share-locked, and clears hashso_buc_split: from here on the scan skips
* the moved-by-split tuples, whose originals it reads in the split
* bucket, and stops at the populated bucket's primary page instead of
* crossing again.
*/
static Buffer
_hash_step_to_populated_bucket(IndexScanDesc scan)
{
HashScanOpaque so = (HashScanOpaque) scan->opaque;
Buffer buf = so->hashso_bucket_buf;
Page page;
HashPageOpaque opaque;
/*
* buffer for bucket being populated must be valid as we acquire the pin
* on it before the start of scan and retain it till end of scan.
*/
Assert(BufferIsValid(buf));
LockBuffer(buf, BUFFER_LOCK_SHARE);
page = BufferGetPage(buf);
opaque = (HashPageOpaque) PageGetSpecialPointer(page);
/* move to the end of bucket chain */
while (BlockNumberIsValid(opaque->hasho_nextblkno))
_hash_readnext(scan, &buf, &page, &opaque);
so->hashso_buc_split = false;
return buf;
}
/*
* _hash_first() -- Find the first item in a scan.
*