On Fri, 3 Feb 2023 at 15:26, David Rowley <dgrowle...@gmail.com> wrote:
> I've pushed all but the final 2 patches now.
I just pushed the final patch in the series. I held back on moving
the setting of rs_inited back into the heapgettup_initial_block()
helper function as I wondered if we should even keep that field.
It seems that rs_cblock == InvalidBlockNumber in all cases where
rs_inited == false, so maybe it's better just to use that as a
condition to check if the scan has started or not. I've attached a
patch which does that.
I ended up adjusting HeapScanDescData more than what is minimally
required to remove rs_inited. I wondered if rs_cindex should be closer
to rs_cblock in the struct so that we're more likely to be adjusting
the same cache line than if that field were closer to the end of the
struct. We don't need rs_coffset and rs_cindex at the same time, so I
made it a union. I see that the struct is still 712 bytes before and
after this change. I've not yet tested to see if there are any
performance gains to this change.
David
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 7eb79cee58..e171d6e38b 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -321,13 +321,15 @@ initscan(HeapScanDesc scan, ScanKey key, bool
keep_startblock)
}
scan->rs_numblocks = InvalidBlockNumber;
- scan->rs_inited = false;
scan->rs_ctup.t_data = NULL;
ItemPointerSetInvalid(&scan->rs_ctup.t_self);
scan->rs_cbuf = InvalidBuffer;
scan->rs_cblock = InvalidBlockNumber;
- /* page-at-a-time fields are always invalid when not rs_inited */
+ /*
+ * page-at-a-time fields are always invalid when
+ * rs_cblock == InvalidBlockNumber
+ */
/*
* copy the scan key, if appropriate
@@ -355,7 +357,8 @@ heap_setscanlimits(TableScanDesc sscan, BlockNumber
startBlk, BlockNumber numBlk
{
HeapScanDesc scan = (HeapScanDesc) sscan;
- Assert(!scan->rs_inited); /* else too late to change */
+ /* we can't set any limits when there's a scan already running */
+ Assert(scan->rs_cblock == InvalidBlockNumber);
/* else rs_startblock is significant */
Assert(!(scan->rs_base.rs_flags & SO_ALLOW_SYNC));
@@ -493,7 +496,7 @@ heapgetpage(TableScanDesc sscan, BlockNumber block)
static BlockNumber
heapgettup_initial_block(HeapScanDesc scan, ScanDirection dir)
{
- Assert(!scan->rs_inited);
+ Assert(scan->rs_cblock == InvalidBlockNumber);
/* When there are no pages to scan, return InvalidBlockNumber */
if (scan->rs_nblocks == 0 || scan->rs_numblocks == 0)
@@ -559,7 +562,7 @@ heapgettup_start_page(HeapScanDesc scan, ScanDirection dir,
int *linesleft,
{
Page page;
- Assert(scan->rs_inited);
+ Assert(scan->rs_cblock != InvalidBlockNumber);
Assert(BufferIsValid(scan->rs_cbuf));
/* Caller is responsible for ensuring buffer is locked if needed */
@@ -592,7 +595,7 @@ heapgettup_continue_page(HeapScanDesc scan, ScanDirection
dir, int *linesleft,
{
Page page;
- Assert(scan->rs_inited);
+ Assert(scan->rs_cblock != InvalidBlockNumber);
Assert(BufferIsValid(scan->rs_cbuf));
/* Caller is responsible for ensuring buffer is locked if needed */
@@ -717,9 +720,9 @@ heapgettup_advance_block(HeapScanDesc scan, BlockNumber
block, ScanDirection dir
* the scankeys.
*
* Note: when we fall off the end of the scan in either direction, we
- * reset rs_inited. This means that a further request with the same
- * scan direction will restart the scan, which is a bit odd, but a
- * request with the opposite scan direction will start a fresh scan
+ * reset rs_cblock to InvalidBlockNumber. This means that a further request
+ * with the same scan direction will restart the scan, which is a bit odd, but
+ * a request with the opposite scan direction will start a fresh scan
* in the proper direction. The latter is required behavior for cursors,
* while the former case is generally undefined behavior in Postgres
* so we don't care too much.
@@ -737,12 +740,11 @@ heapgettup(HeapScanDesc scan,
OffsetNumber lineoff;
int linesleft;
- if (unlikely(!scan->rs_inited))
+ if (unlikely(scan->rs_cblock == InvalidBlockNumber))
{
block = heapgettup_initial_block(scan, dir);
/* ensure rs_cbuf is invalid when we get InvalidBlockNumber */
Assert(block != InvalidBlockNumber ||
!BufferIsValid(scan->rs_cbuf));
- scan->rs_inited = true;
}
else
{
@@ -824,7 +826,6 @@ continue_page:
scan->rs_cbuf = InvalidBuffer;
scan->rs_cblock = InvalidBlockNumber;
tuple->t_data = NULL;
- scan->rs_inited = false;
}
/* ----------------
@@ -852,12 +853,11 @@ heapgettup_pagemode(HeapScanDesc scan,
int lineindex;
int linesleft;
- if (unlikely(!scan->rs_inited))
+ if (unlikely(scan->rs_cblock == InvalidBlockNumber))
{
block = heapgettup_initial_block(scan, dir);
/* ensure rs_cbuf is invalid when we get InvalidBlockNumber */
Assert(block != InvalidBlockNumber ||
!BufferIsValid(scan->rs_cbuf));
- scan->rs_inited = true;
}
else
{
@@ -924,7 +924,6 @@ continue_page:
scan->rs_cbuf = InvalidBuffer;
scan->rs_cblock = InvalidBlockNumber;
tuple->t_data = NULL;
- scan->rs_inited = false;
}
diff --git a/src/backend/access/heap/heapam_handler.c
b/src/backend/access/heap/heapam_handler.c
index c4b1916d36..e763ac61a6 100644
--- a/src/backend/access/heap/heapam_handler.c
+++ b/src/backend/access/heap/heapam_handler.c
@@ -2279,14 +2279,9 @@ heapam_scan_sample_next_block(TableScanDesc scan,
SampleScanState *scanstate)
/* scanning table sequentially */
if (hscan->rs_cblock == InvalidBlockNumber)
- {
- Assert(!hscan->rs_inited);
blockno = hscan->rs_startblock;
- }
else
{
- Assert(hscan->rs_inited);
-
blockno = hscan->rs_cblock + 1;
if (blockno >= hscan->rs_nblocks)
@@ -2321,13 +2316,12 @@ heapam_scan_sample_next_block(TableScanDesc scan,
SampleScanState *scanstate)
ReleaseBuffer(hscan->rs_cbuf);
hscan->rs_cbuf = InvalidBuffer;
hscan->rs_cblock = InvalidBlockNumber;
- hscan->rs_inited = false;
return false;
}
heapgetpage(scan, blockno);
- hscan->rs_inited = true;
+ Assert(hscan->rs_cblock != InvalidBlockNumber);
return true;
}
diff --git a/src/include/access/heapam.h b/src/include/access/heapam.h
index 8d28bc93ef..c6efd59eb5 100644
--- a/src/include/access/heapam.h
+++ b/src/include/access/heapam.h
@@ -56,9 +56,18 @@ typedef struct HeapScanDescData
/* rs_numblocks is usually InvalidBlockNumber, meaning "scan whole rel"
*/
/* scan current state */
- bool rs_inited; /* false = scan not init'd yet
*/
- OffsetNumber rs_coffset; /* current offset # in
non-page-at-a-time mode */
- BlockNumber rs_cblock; /* current block # in scan, if any */
+ union
+ {
+ /* current offset in non-page-at-a-time mode */
+ OffsetNumber rs_coffset;
+
+ /* current tuple's index in vistuples for page-at-a-time mode */
+ int rs_cindex;
+ };
+
+ BlockNumber rs_cblock; /* current block # in scan, or
+ *
InvalidBlockNumber when the scan is not yet
+ * initialized
*/
Buffer rs_cbuf; /* current buffer in scan, if
any */
/* NB: if rs_cbuf is not InvalidBuffer, we hold a pin on that buffer */
@@ -74,7 +83,6 @@ typedef struct HeapScanDescData
ParallelBlockTableScanWorkerData *rs_parallelworkerdata;
/* these fields only used in page-at-a-time mode and for bitmap scans */
- int rs_cindex; /* current tuple's
index in vistuples */
int rs_ntuples; /* number of visible
tuples on page */
OffsetNumber rs_vistuples[MaxHeapTuplesPerPage]; /* their
offsets */
} HeapScanDescData;
