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)