Re: Emit fewer vacuum records by reaping removable tuples during pruning

On Wed, Jan 17, 2024 at 3:58 PM Robert Haas <robertmhaas@gmail.com> wrote:
>
> On Wed, Jan 17, 2024 at 3:12 PM Melanie Plageman
> <melanieplageman@gmail.com> wrote:
>
> > Yes, I also spent some time thinking about this. In master, we do
> > always call lazy_scan_new_or_empty() before calling
> > lazy_scan_noprune(). The code is aiming to ensure we call
> > lazy_scan_new_or_empty() once before calling either of
> > lazy_scan_noprune() or lazy_scan_prune(). I think it didn't call
> > lazy_scan_new_or_empty() unconditionally first because of the
> > different lock types expected. But, your structure has solved that.
> > I've used a version of your example code above in attached v9. It is
> > much nicer.
>
> Oh, OK, I see it now. I missed that lazy_scan_new_or_empty() was
> called either way. Glad that my proposed restructuring managed to be
> helpful despite that confusion, though. :-)
>
> At a quick glance, I also like the way this looks. I'll review it more
> thoroughly later. Does this patch require 0002 and 0003 or could it
> equally well go first? I confess that I don't entirely understand why
> we want 0002 and 0003.

Well, 0002 and 0003 move the updates to the visibility map into
lazy_scan_prune(). We only want to update the VM if we called
lazy_scan_prune() (i.e. not if lazy_scan_noprune() returned true). We
also need the lock on the heap page when updating the visibility map
but we want to have released the lock before updating the FSM, so we
need to first update the VM then the FSM.

The VM update code, in my opinion, belongs in lazy_scan_prune() --
since it is only done when lazy_scan_prune() is called. To keep the VM
update code in lazy_scan_heap() and still consolidate the FSM update
code, we would have to surround all of the VM update code in a test
(if got_cleanup_lock, I suppose). I don't see any advantage in doing
that.

> > Ah, I realize I was not clear. I am now talking about inconsistencies
> > in vacuuming the FSM itself. FreeSpaceMapVacuumRange(). Not updating
> > the freespace map during the course of vacuuming the heap relation.
>
> Fair enough, but I'm still not quite sure exactly what the question
> is. It looks to me like the current code, when there are indexes,
> vacuums the FSM after each round of index vacuuming. When there are no
> indexes, doing it after each round of index vacuuming would mean never
> doing it, so instead we vacuum the FSM every ~8GB. I assume what
> happened here is that somebody decided doing it after each round of
> index vacuuming was the "right thing," and then realized that was not
> going to work if no index vacuuming was happening, and so inserted the
> 8GB threshold to cover that case.

Ah, I see. I understood that we want to update the FSM every 8GB, but
I didn't understand that we wanted to check if we were at that 8GB
only after a round of index vacuuming. That would explain why we also
had to do it in the no indexes case -- because, as you say, there
wouldn't be a round of index vacuuming.

This does mean that something is not quite right with 0001 as well as
0004. We'd end up checking if we are at 8GB much more often. I should
probably find a way to replicate the cadence on master.

> I don't really know what to make of
> all of this. On a happy PostgreSQL system, doing anything after each
> round of index vacuuming means doing it once, because multiple rounds
> of indexing vacuum are extremely expensive and we hope that it won't
> ever occur. From that point of view, the 8GB threshold is better,
> because it means that when we vacuum a large relation, space should
> become visible to the rest of the system incrementally without needing
> to wait for the entire vacuum to finish. On the other hand, we also
> have this idea that we want to record free space in the FSM once,
> after the last time we touch the page. Given that behavior, vacuuming
> the FSM every 8GB when we haven't yet done index vacuuming wouldn't
> accomplish much of anything, because we haven't updated it for the
> pages we just touched. On the third hand, the current behavior seems
> slightly ridiculous, because pruning the page is where we're mostly
> going to free up space, so we might be better off just updating the
> FSM then instead of waiting. That free space could be mighty useful
> during the long wait between pruning and marking line pointers unused.
> On the fourth hand, that seems like a significant behavior change that
> we might not want to undertake without a bunch of research that we
> might not want to do right now -- and if we did do it, should we then
> update the FSM a second time after marking line pointers unused?

I suspect we'd need to do some testing of various scenarios to justify
such a change.

- Melanie