Re: Eliminate redundant tuple visibility check in vacuum

Hi Melanie,

On 8/31/23 02:59, Melanie Plageman wrote:
>> I created a large table and then updated a tuple on every page in the
>> relation and vacuumed it. I saw a consistent slight improvement in
>> vacuum execution time. I profiled a bit with perf stat as well. The
>> difference is relatively small for this kind of example, but I can
>> work on a more compelling, realistic example. I think eliminating the
>> retry loop is also useful, as I have heard that users have had to
>> cancel vacuums which were in this retry loop for too long.
> Just to provide a specific test case, if you create a small table like this
>
> create table foo (a int, b int, c int) with(autovacuum_enabled=false);
> insert into foo select i, i, i from generate_series(1, 10000000);
>
> And then vacuum it. I find that with my patch applied I see a
> consistent ~9% speedup (averaged across multiple runs).
>
> master: ~533ms
> patch: ~487ms
>
> And in the profile, with my patch applied, you notice less time spent
> in HeapTupleSatisfiesVacuumHorizon()
>
> master:
>      11.83%  postgres  postgres           [.] heap_page_prune
>      11.59%  postgres  postgres           [.] heap_prepare_freeze_tuple
>       8.77%  postgres  postgres           [.] lazy_scan_prune
>       8.01%  postgres  postgres           [.] HeapTupleSatisfiesVacuumHorizon
>       7.79%  postgres  postgres           [.] heap_tuple_should_freeze
>
> patch:
>      13.41%  postgres  postgres           [.] heap_prepare_freeze_tuple
>       9.88%  postgres  postgres           [.] heap_page_prune
>       8.61%  postgres  postgres           [.] lazy_scan_prune
>       7.00%  postgres  postgres           [.] heap_tuple_should_freeze
>       6.43%  postgres  postgres           [.] HeapTupleSatisfiesVacuumHorizon

Thanks a lot for providing additional information and the test case.

I tried it on a release build and I also see a 10% speed-up. I reset the 
visibility map between VACUUM runs, see:

CREATE EXTENSION pg_visibility; CREATE TABLE foo (a INT, b INT, c INT) 
WITH(autovacuum_enabled=FALSE); INSERT INTO foo SELECT i, i, i from 
generate_series(1, 10000000) i; VACUUM foo; SELECT 
pg_truncate_visibility_map('foo'); VACUUM foo; SELECT 
pg_truncate_visibility_map('foo'); VACUUM foo; ...

The first patch, which refactors the code so we can pass the result of 
the visibility checks to the caller, looks good to me.

Regarding the 2nd patch (disclaimer: I'm not too familiar with that area 
of the code): I don't completely understand why the retry loop is not 
needed anymore and how you now detect/handle the possible race 
condition? It can still happen that an aborting transaction changes the 
state of a row after heap_page_prune() looked at that row. Would that 
case now not be ignored?

-- 
David Geier
(ServiceNow)