Thread: Regarding the necessity of RelationGetNumberOfBlocks for every rescan / bitmap heap scan.

Regarding the necessity of RelationGetNumberOfBlocks for every rescan / bitmap heap scan.

From

Andy Fan

Date:

29 May 2021, 06:23:31

Hi:

I'm always confused about the following codes.

static void
initscan(HeapScanDesc scan, ScanKey key, bool keep_startblock)
{
ParallelBlockTableScanDesc bpscan = NULL;
bool allow_strat;
bool allow_sync;

/*
* Determine the number of blocks we have to scan.
*
* It is sufficient to do this once at scan start, since any tuples added
* while the scan is in progress will be invisible to my snapshot anyway.
* (That is not true when using a non-MVCC snapshot. However, we couldn't
* guarantee to return tuples added after scan start anyway, since they
* might go into pages we already scanned. To guarantee consistent
* results for a non-MVCC snapshot, the caller must hold some higher-level
* lock that ensures the interesting tuple(s) won't change.)
*/
if (scan->rs_base.rs_parallel != NULL)
{
bpscan = (ParallelBlockTableScanDesc) scan->rs_base.rs_parallel;
scan->rs_nblocks = bpscan->phs_nblocks;
}
else
scan->rs_nblocks = RelationGextNumberOfBlocks(scan->rs_base.rs_rd);

..
}

1. Why do we need scan->rs_nblocks =
RelationGextNumberOfBlocks(scan->rs_base.rs_rd) for every rescan, which looks
mismatched with the comments along the code. and the comments looks
reasonable to me.
2. For the heap scan after an IndexScan, we don't need to know the heap
size, then why do we need to get the nblocks for bitmap heap scan? I think the
similarity between the 2 is that both of them can get a "valid" CTID/pages number
from index scan. To be clearer, I think for bitmap heap scan, we even don't
need check the RelationGextNumberOfBlocks for the initscan.

3. If we need to check nblocks every time, why Parallel Scan doesn't change it

every time?

shall we remove the RelationGextNumberOfBlocks for bitmap heap scan totally

and the rescan for normal heap scan?

Best Regards

Andy Fan (https://www.aliyun.com/)

Re: Regarding the necessity of RelationGetNumberOfBlocks for every rescan / bitmap heap scan.

From

Andy Fan

Date:

30 May 2021, 07:51:40

1. Why do we need scan->rs_nblocks =
RelationGextNumberOfBlocks(scan->rs_base.rs_rd) for every rescan, which looks
mismatched with the comments along the code. and the comments looks
reasonable to me.
2. For the heap scan after an IndexScan, we don't need to know the heap
size, then why do we need to get the nblocks for bitmap heap scan? I think the
similarity between the 2 is that both of them can get a "valid" CTID/pages number
from index scan. To be clearer, I think for bitmap heap scan, we even don't
need check the RelationGextNumberOfBlocks for the initscan.
3. If we need to check nblocks every time, why Parallel Scan doesn't change it
every time?

shall we remove the RelationGextNumberOfBlocks for bitmap heap scan totally
and the rescan for normal heap scan?

yizhi.fzh@e18c07352 /u/y/g/postgres> git diff
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 6ac07f2fda..6df096fb46 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -246,7 +246,7 @@ initscan(HeapScanDesc scan, ScanKey key, bool keep_startblock)
bpscan = (ParallelBlockTableScanDesc) scan->rs_base.rs_parallel;
scan->rs_nblocks = bpscan->phs_nblocks;
}
- else
+ else if (scan->rs_nblocks == -1 && !(scan->rs_base.rs_flags & SO_TYPE_BITMAPSCAN))
scan->rs_nblocks = RelationGetNumberOfBlocks(scan->rs_base.rs_rd);

/*
@@ -1209,6 +1209,7 @@ heap_beginscan(Relation relation, Snapshot snapshot,
scan->rs_base.rs_flags = flags;
scan->rs_base.rs_parallel = parallel_scan;
scan->rs_strategy = NULL; /* set in initscan */
+ scan->rs_nblocks = -1;

I did the above hacks, and all the existing tests passed.

Best Regards

Andy Fan (https://www.aliyun.com/)

Re: Regarding the necessity of RelationGetNumberOfBlocks for every rescan / bitmap heap scan.

From

Andy Fan

Date:

31 May 2021, 08:46:22

On Sat, May 29, 2021 at 11:23 AM Andy Fan <zhihui.fan1213@gmail.com> wrote:

Hi:

I'm always confused about the following codes.

static void
initscan(HeapScanDesc scan, ScanKey key, bool keep_startblock)
{
ParallelBlockTableScanDesc bpscan = NULL;
bool allow_strat;
bool allow_sync;

/*
* Determine the number of blocks we have to scan.
*
* It is sufficient to do this once at scan start, since any tuples added
* while the scan is in progress will be invisible to my snapshot anyway.
* (That is not true when using a non-MVCC snapshot. However, we couldn't
* guarantee to return tuples added after scan start anyway, since they
* might go into pages we already scanned. To guarantee consistent
* results for a non-MVCC snapshot, the caller must hold some higher-level
* lock that ensures the interesting tuple(s) won't change.)
*/
if (scan->rs_base.rs_parallel != NULL)
{
bpscan = (ParallelBlockTableScanDesc) scan->rs_base.rs_parallel;
scan->rs_nblocks = bpscan->phs_nblocks;
}
else
scan->rs_nblocks = RelationGextNumberOfBlocks(scan->rs_base.rs_rd);

..
}

1. Why do we need scan->rs_nblocks =
RelationGextNumberOfBlocks(scan->rs_base.rs_rd) for every rescan, which looks
mismatched with the comments along the code. and the comments looks
reasonable to me.

To be more precise, this question can be expressed as if the relation size
can be changed during rescan. We are sure that the size can be increased due to
new data, but we are sure that the new data is useless for the query as well. So
looks this case is ok. and for the file size decreasing, since we have lock on
the relation, so the file size would not be reduced as well (I have verified

this logic on the online vacuum case, other cases should be similar as well).

Best Regards

Andy Fan (https://www.aliyun.com/)

回复：Re: Regarding the necessity of RelationGetNumberOfBlocks for every rescan / bitmap heap scan.

From

"陈佳昕(步真)"

Date:

31 May 2021, 08:59:27

+1, This would be an nice improvement even the lseek is fast usually, it is a system call after all

Buzhen

------------------------------------------------------------------
发件人：Andy Fan<zhihui.fan1213@gmail.com>
日　期：2021年05月31日 13:46:22
收件人：PostgreSQL Hackers<pgsql-hackers@lists.postgresql.org>
主　题：Re: Regarding the necessity of RelationGetNumberOfBlocks for every rescan / bitmap heap scan.

On Sat, May 29, 2021 at 11:23 AM Andy Fan <zhihui.fan1213@gmail.com> wrote:
Hi:

I'm always confused about the following codes.

static void
initscan(HeapScanDesc scan, ScanKey key, bool keep_startblock)
{
ParallelBlockTableScanDesc bpscan = NULL;
bool allow_strat;
bool allow_sync;

/*
* Determine the number of blocks we have to scan.
*
* It is sufficient to do this once at scan start, since any tuples added
* while the scan is in progress will be invisible to my snapshot anyway.
* (That is not true when using a non-MVCC snapshot. However, we couldn't
* guarantee to return tuples added after scan start anyway, since they
* might go into pages we already scanned. To guarantee consistent
* results for a non-MVCC snapshot, the caller must hold some higher-level
* lock that ensures the interesting tuple(s) won't change.)
*/
if (scan->rs_base.rs_parallel != NULL)
{
bpscan = (ParallelBlockTableScanDesc) scan->rs_base.rs_parallel;
scan->rs_nblocks = bpscan->phs_nblocks;
}
else
scan->rs_nblocks = RelationGextNumberOfBlocks(scan->rs_base.rs_rd);

..
}

1. Why do we need scan->rs_nblocks =
RelationGextNumberOfBlocks(scan->rs_base.rs_rd) for every rescan, which looks
mismatched with the comments along the code. and the comments looks
reasonable to me.

To be more precise, this question can be expressed as if the relation size
can be changed during rescan. We are sure that the size can be increased due to
new data, but we are sure that the new data is useless for the query as well. So
looks this case is ok. and for the file size decreasing, since we have lock on
the relation, so the file size would not be reduced as well (I have verified
this logic on the online vacuum case, other cases should be similar as well).

--
Best Regards
Andy Fan (https://www.aliyun.com/)