Re: initscan for MVCC snapshot

On Mon, Dec 7, 2020 at 8:26 PM Andy Fan <zhihui(dot)fan1213(at)gmail(dot)com> wrote:

> Hi:
> I see initscan calls RelationGetwNumberOfBlocks every time and rescan
> calls
> initscan as well. In my system, RelationGetNumberOfBlocks is expensive
> (the reason
> doesn't deserve a talk.. ), so in a nest loop + Bitmap heap scan case,
> the
> impact will be huge. The comments of initscan are below.
>
> /*
> * 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.)
> */
>
> I still do not fully understand the comments. Looks we only need to call
> multi times for non-MVCC snapshot, IIUC, does the following change
> reasonable?
>
> ===
>
> diff --git a/src/backend/access/heap/heapam.c
> b/src/backend/access/heap/heapam.c
> index 1b2f70499e..8238eabd8b 100644
> --- a/src/backend/access/heap/heapam.c
> +++ b/src/backend/access/heap/heapam.c
> @@ -211,6 +211,7 @@ initscan(HeapScanDesc scan, ScanKey key, bool
> keep_startblock)
> ParallelBlockTableScanDesc bpscan = NULL;
> bool allow_strat;
> bool allow_sync;
> + bool is_mvcc = scan->rs_base.rs_snapshot &&
> IsMVCCSnapshot(scan->rs_base.rs_snapshot);
>
> /*
> * Determine the number of blocks we have to scan.
> @@ -229,7 +230,8 @@ initscan(HeapScanDesc scan, ScanKey key, bool
> keep_startblock)
> scan->rs_nblocks = bpscan->phs_nblocks;
> }
> else
> - scan->rs_nblocks =
> RelationGetNumberOfBlocks(scan->rs_base.rs_rd);
> + if (scan->rs_nblocks == -1 || !is_mvcc)
> + scan->rs_nblocks =
> RelationGetNumberOfBlocks(scan->rs_base.rs_rd);
>
> /*
> * If the table is large relative to NBuffers, use a bulk-read
> access
> @@ -1210,6 +1212,7 @@ heap_beginscan(Relation relation, Snapshot snapshot,
> else
> scan->rs_base.rs_key = NULL;
>
> + scan->rs_nblocks = -1;
> initscan(scan, key, false);
>
> --
> Best Regards
> Andy Fan
>

I have tested this with an ext4 file system, and I can get a 7%+
performance improvement
for the given test case. Here are the steps:

create table t(a int, b char(8000));
insert into t select i, i from generate_series(1, 1000000)i;
create index on t(a);
delete from t where a <= 10000;
vacuum t;
alter system set enable_indexscan to off;
select pg_reload_conf();

cat 1.sql
select * from generate_series(1, 10000)i, t where i = t.a;

bin/pgbench -f 1.sql postgres -T 300 -c 10

Without this patch: latency average = 61.806 ms
with this patch: latency average = 57.484 ms

I think the result is good and I think we can probably make this change.
However, I'm not
sure about it.

--
Best Regards
Andy Fan