Re: Confine vacuum skip logic to lazy_scan_skip

On Sun, Dec 15, 2024 at 10:10 AM Tomas Vondra <tomas(at)vondra(dot)me> wrote:
>
> I've been looking at some other vacuum-related patches, so I took a look
> at these remaining bits too. I don't have much to say about the code
> (seems perfectly fine to me), so I decided to do a bit of testing.

Thanks for doing this!

> I did a simple test (see the attached .sh script) that runs vacuum on a
> table with different fractions of rows updated. The table has ~100 rows
> per page, with 50% fill factor, and the updates touch ~1%, 0.1%, 0.05%
> rows, and then even smaller fractions up to 0.0001%. This determines how
> many pages get touched. With 1% fraction almost every page gets
> modified, then the fraction quickly drops. With 0.0001% only about 0.01%
> of pages gets modified.
>
> Attached is a CSV with raw results from two machines, and also a PDF
> with a comparison of the two build (master vs. patched). In vast
> majority of cases, the patched build is much faster, usually 2-3x.
>
> There are a couple cases where it regresses by ~30%, but only on one of
> the machines with older storage (RAID on SATA SSDs), with 1% rows
> updated (which means almost all pages get modified). So this is about
> sequential access. It's a bit weird, but probably not a fatal issue.

Actually, while rebasing these with the intent to start investigating
the regressions you report, I noticed something quite wrong with my
code. In lazy_scan_heap(), I had put read_stream_next_buffer() before
a few expensive operations (like a round of index vacuuming and dead
item reaping if the TID store is full). It returns the pinned buffer,
so this could mean a buffer remaining pinned for a whole round of
vacuuming of items from the TID store. Not good. Anyway, this version
has fixed that. I do wonder if there is any chance that this affected
your benchmarks.

I've attached a new v13. Perhaps you could give it another go and see
if the regressions are still there?

> There is also a couple smaller regressions on the "xeon" machine with
> M.2 SSD, for lower update fractions - 0.05% rows updated means 5% pages
> need vacuum. But I think this is more a sign of us being too aggressive
> in detecting (and forcing) sequential patterns - on master, we end up
> scanning 50% pages, thanks to SKIP_PAGES_THRESHOLD. I'll start a new
> thread about that ...

Hmm. If only 5% of pages need vacuum, then you are right, readahead
seems like it would often be wasted (depending on _which_ 5% needs
vacuuming). But, the read stream API will only prefetch and build up
larger I/Os of blocks we actually need. So, it seems like this would
behave the same on master and with the patch. That is, both would do
extra unneeded I/O because of SKIP_PAGES_THRESHOLD. Is the 5% of the
table that needs vacuuming dispersed randomly throughout or
concentrated?

If it is concentrated and readahead would be useful, then maybe we
need to increase read_ahead_kb. You mentioned off-list that the
read_ahead_kb on this machine for this SSD was 128kB -- the same as
io_combine_limit. If we want to read ahead, issuing 128 kB I/Os might
be thwarting us and increasing latency.

- Melanie