On Thu, Jul 9, 2026 at 10:02 PM Peter Geoghegan <[email protected]> wrote:
>
> I ran this against CI, and saw failures on 32-bit meson tied to tuple
> alignment. When MAXALIGN is 4, the assumptions about page layout
> underlying the test case break.
>
> Attached is V3, which directly addresses the alignment issue, and
> fully passes CI.
>
Thanks, the fixup-patch looks good to me. However, I was thinking
whether it is a good idea to refactor the common code into separate
functions like attached? This is atop your patches. BTW, I took help
from Claude to do this refactoring. I am fine with committing
/sql/hash_split.sql test along with the code-fix.
--
With Regards,
Amit Kapila.
diff --git a/src/backend/access/hash/hashsearch.c
b/src/backend/access/hash/hashsearch.c
index 7882a531f4f..96a2641372f 100644
--- a/src/backend/access/hash/hashsearch.c
+++ b/src/backend/access/hash/hashsearch.c
@@ -30,6 +30,9 @@ 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);
+static Buffer _hash_step_to_split_bucket(IndexScanDesc scan);
+static void _hash_step_to_populated_bucket(IndexScanDesc scan, Buffer *bufp,
+
Page *pagep, HashPageOpaque *opaquep);
/*
* _hash_next() -- Get the next item in a scan.
@@ -77,22 +80,7 @@ _hash_next(IndexScanDesc scan, ScanDirection dir)
}
else if (so->hashso_buc_populated &&
!so->hashso_buc_split)
{
- /*
- * end of bucket being populated: continue the
scan in the
- * bucket being split, on whose primary page we
have held a
- * pin since _hash_first
- */
- buf = so->hashso_split_bucket_buf;
- Assert(BufferIsValid(buf));
- LockBuffer(buf, BUFFER_LOCK_SHARE);
- PredicateLockPage(rel,
BufferGetBlockNumber(buf),
-
scan->xs_snapshot);
-
- /*
- * setting hashso_buc_split to true indicates
that we are
- * scanning bucket being split.
- */
- so->hashso_buc_split = true;
+ buf = _hash_step_to_split_bucket(scan);
if (!_hash_readpage(scan, &buf, dir))
end_of_scan = true;
@@ -131,29 +119,7 @@ _hash_next(IndexScanDesc scan, ScanDirection dir)
Page page;
HashPageOpaque opaque;
- /*
- * start of bucket being split: continue the
scan from the
- * last page in the chain of the bucket being
populated, on
- * whose primary page we have held a pin since
_hash_first
- */
- buf = so->hashso_bucket_buf;
- Assert(BufferIsValid(buf));
- LockBuffer(buf, BUFFER_LOCK_SHARE);
- page = BufferGetPage(buf);
- opaque = HashPageGetOpaque(page);
-
- /* move to the end of bucket chain */
- while
(BlockNumberIsValid(opaque->hasho_nextblkno))
- _hash_readnext(scan, &buf, &page,
&opaque);
-
- /*
- * setting hashso_buc_split to false indicates
that we are
- * scanning the bucket being populated. Only
set it after
- * the chain walk above; otherwise
_hash_readnext would
- * advance to the bucket being split on
reaching the end of
- * the chain, instead of stopping there.
- */
- so->hashso_buc_split = false;
+ _hash_step_to_populated_bucket(scan, &buf,
&page, &opaque);
if (!_hash_readpage(scan, &buf, dir))
end_of_scan = true;
@@ -177,6 +143,76 @@ _hash_next(IndexScanDesc scan, ScanDirection dir)
return true;
}
+/*
+ * Cross over from the bucket being populated to the bucket being split, on
+ * whose primary page we have held a pin since _hash_first. Used both when
+ * a forward scan of the populated bucket's own chain runs out of pages, and
+ * (via _hash_readnext) when the chain-walk inside _hash_readpage does.
+ *
+ * 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). Used both when a backward scan of the split bucket's own chain
+ * runs out of pages, and (via _hash_readprev) when the chain-walk inside
+ * _hash_readpage does.
+ *
+ * On return, the output parameters describe the last page in the
+ * populated bucket's chain, pinned and share-locked, and hashso_buc_split
+ * has been cleared to indicate that we are now scanning that bucket. The
+ * flag must be cleared only after the chain walk; otherwise _hash_readnext
+ * would cross back over to the split bucket on reaching the end of the
+ * chain, instead of stopping there.
+ */
+static void
+_hash_step_to_populated_bucket(IndexScanDesc scan, Buffer *bufp,
+ Page *pagep,
HashPageOpaque *opaquep)
+{
+ HashScanOpaque so = (HashScanOpaque) scan->opaque;
+
+ *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);
+ *pagep = BufferGetPage(*bufp);
+ *opaquep = HashPageGetOpaque(*pagep);
+
+ /* move to the end of bucket chain */
+ while (BlockNumberIsValid((*opaquep)->hasho_nextblkno))
+ _hash_readnext(scan, bufp, pagep, opaquep);
+
+ so->hashso_buc_split = false;
+}
+
/*
* Advance to next page in a bucket, if any. If we are scanning the bucket
* being populated during split operation then this function advances to the
@@ -216,23 +252,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;
}
@@ -299,27 +319,7 @@ _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);
- *pagep = BufferGetPage(*bufp);
- *opaquep = HashPageGetOpaque(*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;
+ _hash_step_to_populated_bucket(scan, bufp, pagep, opaquep);
}
}
