Re: [HACKERS] [PATCH] Vacuum: Update FSM more frequently

Claudio Freire <klaussfreire(at)gmail(dot)com> writes:
> Attached patches, rebased and modified as discussed:
> 1 no longer does tree pruning, it just vacuums a range of the FSM
> 2 reintroduces tree pruning for the initial FSM vacuum
> 3 and 4 remain as they were, but rebased

I reviewed and cleaned up 0001. The API for FreeSpaceMapVacuumRange was
underspecified, and the use of it seemed to have off-by-one errors. Also,
you still had vacuum doing a full FreeSpaceMapVacuum call at the end;
I thought the point was to get rid of that. We then need to make sure
we clean up after a truncation, but we can do that by introducing a
FreeSpaceMapVacuumRange call into FreeSpaceMapTruncateRel. I think the
attached 0001 is committable, but feel free to take another look.

I still don't like 0002. It's adding a lot of complexity, and
not-negligible overhead, to solve yesterday's problem. After 0001,
there's no reason to assume that vacuum is particularly likely to get
cancelled between having made cleanups and having updated the upper FSM
levels. (Maybe the odds are a bit more for the no-indexes case, but
that doesn't seem like it's common enough to justify a special mechanism
either.)

Not sure what to think about 0003. At this point I'd be inclined
to flush UpdateFreeSpaceMap altogether and use FreeSpaceMapVacuumRange
in its place. I don't think the design of that function is any better
chosen than its name, and possible bugs in its subroutines don't make
it more attractive.

Not sure about 0004 either. The fact that we can't localize what part of
the index needs to be updated means that repeated IndexFreeSpaceMapVacuum
calls are a roughly quadratic cost. Maybe in proportion to the other work
we have to do, they're not much, but on the other hand how much benefit is
there? Should we make the call conditional on how many index pages got
updated? Also, I wonder why you only touched nbtree and spgist. (For
that matter, I wonder why BRIN doesn't go through IndexFreeSpaceMapVacuum
like the rest of the index AMs. Or perhaps it has the right idea and we
should get rid of IndexFreeSpaceMapVacuum as a useless layer.)

regards, tom lane