v22b-0005-review.patch

From 3382c9290f3c01389134156d95a9c92531748e48 Mon Sep 17 00:00:00 2001
From: Tomas Vondra <tv@fuzzy.cz>
Date: Tue, 18 Jun 2024 21:03:49 +0200
Subject: [PATCH v22b 05/29] review

---
 src/backend/executor/nodeBitmapHeapscan.c | 14 ++++++++++++++
 src/include/access/tableam.h              |  2 ++
 2 files changed, 16 insertions(+)

diff --git a/src/backend/executor/nodeBitmapHeapscan.c b/src/backend/executor/nodeBitmapHeapscan.c
index e8b4a754434..6d7ef9ced19 100644
--- a/src/backend/executor/nodeBitmapHeapscan.c
+++ b/src/backend/executor/nodeBitmapHeapscan.c
@@ -270,6 +270,20 @@ new_page:
 
 		BitmapAdjustPrefetchIterator(node);
 
+		/*
+		 * XXX maybe add comment explaining what "false" means here?
+		 *
+		 * XXX I'm a bit unsure if this needs to be handled using goto. Wouldn't
+		 * it be simpler / easier to understand to have two nested loops?
+		 *
+		 * while (true)
+		 *		if (!table_scan_bitmap_next_block(...)) { break; }
+		 *		while (table_scan_bitmap_next_tuple(...)) {
+		 *			... process tuples ...
+		 *		}
+		 *
+		 * But I haven't tried implementing this.
+		 */
 		if (!table_scan_bitmap_next_block(scan, &node->blockno, &node->recheck,
 										  &node->lossy_pages, &node->exact_pages))
 			break;
diff --git a/src/include/access/tableam.h b/src/include/access/tableam.h
index 2f4e221870e..c6a9fb34bd6 100644
--- a/src/include/access/tableam.h
+++ b/src/include/access/tableam.h
@@ -1023,6 +1023,7 @@ table_beginscan_analyze(Relation rel)
 static inline void
 table_endscan(TableScanDesc scan)
 {
+	/* XXX worth a comment? */
 	if (scan->rs_flags & SO_TYPE_BITMAPSCAN)
 	{
 		if (scan->shared_tbmiterator)
@@ -1048,6 +1049,7 @@ static inline void
 table_rescan(TableScanDesc scan,
 			 struct ScanKeyData *key)
 {
+	/* XXX worth a comment? */
 	if (scan->rs_flags & SO_TYPE_BITMAPSCAN)
 	{
 		if (scan->shared_tbmiterator)
-- 
2.45.2