Re: Confine vacuum skip logic to lazy_scan_skip - Mailing list pgsql-hackers

On Fri, Mar 8, 2024 at 12:34 PM Melanie Plageman
<melanieplageman@gmail.com> wrote:
>
> On Fri, Mar 08, 2024 at 06:07:33PM +0200, Heikki Linnakangas wrote:
> > On 08/03/2024 02:46, Melanie Plageman wrote:
> > > On Wed, Mar 06, 2024 at 10:00:23PM -0500, Melanie Plageman wrote:
> > > > On Wed, Mar 06, 2024 at 09:55:21PM +0200, Heikki Linnakangas wrote:
> > > I will say that now all of the variable names are *very* long. I didn't
> > > want to remove the "state" from LVRelState->next_block_state. (In fact, I
> > > kind of miss the "get". But I had to draw the line somewhere.) I think
> > > without "state" in the name, next_block sounds too much like a function.
> > >
> > > Any ideas for shortening the names of next_block_state and its members
> > > or are you fine with them?
> >
> > Hmm, we can remove the inner struct and add the fields directly into
> > LVRelState. LVRelState already contains many groups of variables, like
> > "Error reporting state", with no inner structs. I did it that way in the
> > attached patch. I also used local variables more.
>
> +1; I like the result of this.

I did some perf testing of 0002 and 0003 using that fully-in-SB vacuum
test I mentioned in an earlier email. 0002 is a vacuum time reduction
from an average of 11.5 ms on master to 9.6 ms with 0002 applied. And
0003 reduces the time vacuum takes from 11.5 ms on master to 7.4 ms
with 0003 applied.

I profiled them and 0002 seems to simply spend less time in
heap_vac_scan_next_block() than master did in lazy_scan_skip().

And 0003 reduces the time vacuum takes because vacuum_delay_point()
shows up pretty high in the profile.

Here are the profiles for my test.

profile of master:

+   29.79%  postgres  postgres           [.] visibilitymap_get_status
+   27.35%  postgres  postgres           [.] vacuum_delay_point
+   17.00%  postgres  postgres           [.] lazy_scan_skip
+    6.59%  postgres  postgres           [.] heap_vacuum_rel
+    6.43%  postgres  postgres           [.] BufferGetBlockNumber

profile with 0001-0002:

+   40.30%  postgres  postgres           [.] visibilitymap_get_status
+   20.32%  postgres  postgres           [.] vacuum_delay_point
+   20.26%  postgres  postgres           [.] heap_vacuum_rel
+    5.17%  postgres  postgres           [.] BufferGetBlockNumber

profile with 0001-0003

+   59.77%  postgres  postgres           [.] visibilitymap_get_status
+   23.86%  postgres  postgres           [.] heap_vacuum_rel
+    6.59%  postgres  postgres           [.] StrategyGetBuffer

Test DDL and setup:

psql -c "ALTER SYSTEM SET shared_buffers = '16 GB';"
psql -c "CREATE TABLE foo(id INT, a INT, b INT, c INT, d INT, e INT, f
INT, g INT) with (autovacuum_enabled=false, fillfactor=25);"
psql -c "INSERT INTO foo SELECT i, i, i, i, i, i, i, i FROM
generate_series(1, 46000000)i;"
psql -c "VACUUM (FREEZE) foo;"
pg_ctl restart
psql -c "SELECT pg_prewarm('foo');"
# make sure there isn't an ill-timed checkpoint
psql -c "\timing on" -c "vacuum (verbose) foo;"

- Melanie