From 35ac618a7c4602b792160ae0d77b6dfb289f517e Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Tue, 21 Jul 2015 13:38:24 -0400 Subject: [PATCH] Fix some oversights in BRIN patch. Remove HeapScanDescData.rs_initblock, which wasn't being used for anything in the final version of the patch. Fix IndexBuildHeapScan so that it supports syncscan again; the patch broke synchronous scanning for index builds by forcing rs_startblk to zero even when the caller did not care about that and had asked for syncscan. Add some commentary and usage defenses to heap_setscanlimits(). Fix heapam so that asking for rs_numblocks == 0 does what you would reasonably expect. As coded it amounted to requesting a whole-table scan, because those "--x <= 0" tests on an unsigned variable would behave surprisingly. --- src/backend/access/heap/heapam.c | 30 ++++++++++++++++++++---------- src/backend/catalog/index.c | 12 ++++++++++-- src/include/access/relscan.h | 4 ++-- 3 files changed, 32 insertions(+), 14 deletions(-) diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c index 86a2e6bae6..6f4ff2718f 100644 --- a/src/backend/access/heap/heapam.c +++ b/src/backend/access/heap/heapam.c @@ -277,7 +277,6 @@ initscan(HeapScanDesc scan, ScanKey key, bool is_rescan) scan->rs_startblock = 0; } - scan->rs_initblock = 0; scan->rs_numblocks = InvalidBlockNumber; scan->rs_inited = false; scan->rs_ctup.t_data = NULL; @@ -302,11 +301,22 @@ initscan(HeapScanDesc scan, ScanKey key, bool is_rescan) pgstat_count_heap_scan(scan->rs_rd); } +/* + * heap_setscanlimits - restrict range of a heapscan + * + * startBlk is the page to start at + * numBlks is number of pages to scan (InvalidBlockNumber means "all") + */ void heap_setscanlimits(HeapScanDesc scan, BlockNumber startBlk, BlockNumber numBlks) { + Assert(!scan->rs_inited); /* else too late to change */ + Assert(!scan->rs_syncscan); /* else rs_startblock is significant */ + + /* Check startBlk is valid (but allow case of zero blocks...) */ + Assert(startBlk == 0 || startBlk < scan->rs_nblocks); + scan->rs_startblock = startBlk; - scan->rs_initblock = startBlk; scan->rs_numblocks = numBlks; } @@ -477,7 +487,7 @@ heapgettup(HeapScanDesc scan, /* * return null immediately if relation is empty */ - if (scan->rs_nblocks == 0) + if (scan->rs_nblocks == 0 || scan->rs_numblocks == 0) { Assert(!BufferIsValid(scan->rs_cbuf)); tuple->t_data = NULL; @@ -511,7 +521,7 @@ heapgettup(HeapScanDesc scan, /* * return null immediately if relation is empty */ - if (scan->rs_nblocks == 0) + if (scan->rs_nblocks == 0 || scan->rs_numblocks == 0) { Assert(!BufferIsValid(scan->rs_cbuf)); tuple->t_data = NULL; @@ -651,7 +661,7 @@ heapgettup(HeapScanDesc scan, if (backward) { finished = (page == scan->rs_startblock) || - (scan->rs_numblocks != InvalidBlockNumber ? --scan->rs_numblocks <= 0 : false); + (scan->rs_numblocks != InvalidBlockNumber ? --scan->rs_numblocks == 0 : false); if (page == 0) page = scan->rs_nblocks; page--; @@ -662,7 +672,7 @@ heapgettup(HeapScanDesc scan, if (page >= scan->rs_nblocks) page = 0; finished = (page == scan->rs_startblock) || - (scan->rs_numblocks != InvalidBlockNumber ? --scan->rs_numblocks <= 0 : false); + (scan->rs_numblocks != InvalidBlockNumber ? --scan->rs_numblocks == 0 : false); /* * Report our new scan position for synchronization purposes. We @@ -754,7 +764,7 @@ heapgettup_pagemode(HeapScanDesc scan, /* * return null immediately if relation is empty */ - if (scan->rs_nblocks == 0) + if (scan->rs_nblocks == 0 || scan->rs_numblocks == 0) { Assert(!BufferIsValid(scan->rs_cbuf)); tuple->t_data = NULL; @@ -785,7 +795,7 @@ heapgettup_pagemode(HeapScanDesc scan, /* * return null immediately if relation is empty */ - if (scan->rs_nblocks == 0) + if (scan->rs_nblocks == 0 || scan->rs_numblocks == 0) { Assert(!BufferIsValid(scan->rs_cbuf)); tuple->t_data = NULL; @@ -914,7 +924,7 @@ heapgettup_pagemode(HeapScanDesc scan, if (backward) { finished = (page == scan->rs_startblock) || - (scan->rs_numblocks != InvalidBlockNumber ? --scan->rs_numblocks <= 0 : false); + (scan->rs_numblocks != InvalidBlockNumber ? --scan->rs_numblocks == 0 : false); if (page == 0) page = scan->rs_nblocks; page--; @@ -925,7 +935,7 @@ heapgettup_pagemode(HeapScanDesc scan, if (page >= scan->rs_nblocks) page = 0; finished = (page == scan->rs_startblock) || - (scan->rs_numblocks != InvalidBlockNumber ? --scan->rs_numblocks <= 0 : false); + (scan->rs_numblocks != InvalidBlockNumber ? --scan->rs_numblocks == 0 : false); /* * Report our new scan position for synchronization purposes. We diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c index 4246554d19..69f35c9330 100644 --- a/src/backend/catalog/index.c +++ b/src/backend/catalog/index.c @@ -2168,7 +2168,8 @@ IndexBuildHeapScan(Relation heapRelation, /* * As above, except that instead of scanning the complete heap, only the given * number of blocks are scanned. Scan to end-of-rel can be signalled by - * passing InvalidBlockNumber as numblocks. + * passing InvalidBlockNumber as numblocks. Note that restricting the range + * to scan cannot be done when requesting syncscan. */ double IndexBuildHeapRangeScan(Relation heapRelation, @@ -2251,7 +2252,14 @@ IndexBuildHeapRangeScan(Relation heapRelation, allow_sync); /* syncscan OK? */ /* set our scan endpoints */ - heap_setscanlimits(scan, start_blockno, numblocks); + if (!allow_sync) + heap_setscanlimits(scan, start_blockno, numblocks); + else + { + /* syncscan can only be requested on whole relation */ + Assert(start_blockno == 0); + Assert(numblocks == InvalidBlockNumber); + } reltuples = 0; diff --git a/src/include/access/relscan.h b/src/include/access/relscan.h index f2482e99d6..6e6231971f 100644 --- a/src/include/access/relscan.h +++ b/src/include/access/relscan.h @@ -38,8 +38,8 @@ typedef struct HeapScanDescData /* state set up at initscan time */ BlockNumber rs_nblocks; /* total number of blocks in rel */ BlockNumber rs_startblock; /* block # to start at */ - BlockNumber rs_initblock; /* block # to consider initial of rel */ - BlockNumber rs_numblocks; /* number of blocks to scan */ + BlockNumber rs_numblocks; /* max number of blocks to scan */ + /* rs_numblocks is usually InvalidBlockNumber, meaning "scan whole rel" */ BufferAccessStrategy rs_strategy; /* access strategy for reads */ bool rs_syncscan; /* report location to syncscan logic? */ -- 2.40.0