On Thu, 4 Apr 2024 at 16:45, David Rowley <dgrowleyml@gmail.com> wrote:
> I've pushed the v9-0001 with that rename done.
I've now just pushed the 0002 patch with some revisions:
1. The function declarations you added for heapgettup_advance_block()
and heapgettup_initial_block() didn't match the properties of their
definitions. You'd declared both of these static inline but neither
of these were.
2. I felt inclined to rename heapfetchbuf() to heapfetchnextbuf() as
that's effectively what it does with v8-0002, however, that's just too
many words all shoved together, so I renamed it to
heap_fetch_next_buffer().
3. I changed heapgettup_initial_block() to pg_noinline as it both
makes more sense to have this out of line and it also fixed a small
performance regression.
Looks like I also failed to grep for all the remaining instances of
"heapgetpage" in 44086b097. Those are now fixed by 3a4a3537a.
I also rebased the 0003 patch which I've attached as a raw patch.
FWIW, using Heikki's test in [1] with a pg_prewarm each time after
restarting the instance. No parallel aggregate.
drowley@amd3990x:~$ cat bench.sql
select count(*) from giga;
drowley@amd3990x:~$ pgbench -n -f bench.sql -T 120 postgres | grep latency
44086b097~1
latency average = 34.323 ms
latency average = 34.332 ms
44086b097
latency average = 34.811 ms
latency average = 34.862 ms
3a4a3537a
latency average = 34.497 ms
latency average = 34.538 ms
3a4a3537a + read_stream_for_seqscans.patch
latency average = 40.923 ms
latency average = 41.415 ms
i.e. no meaningful change from the refactor, but a regression from a
cached workload that changes the page often without doing much work in
between with the read stread patch.
I'm happy to run further benchmarks, but for the remainder of the
committer responsibility for the next patches, I'm going to leave that
to Thomas.
David
[1] https://www.postgresql.org/message-id/3b0f3701-addd-4629-9257-cf28e1a6e6a1@iki.fi