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

On Tue, Jan 9, 2024 at 5:42 PM Melanie Plageman
<melanieplageman(at)gmail(dot)com> wrote:
> Done in attached v6.

Don't kill me, but:

+ /*
+ * For now, pass no_indexes == false
regardless of whether or not
+ * the relation has indexes. In the future we
may enable immediate
+ * reaping for on access pruning.
+ */
+ heap_page_prune(relation, buffer, vistest, false,
+ &presult, NULL);

My previous comments about lying seem equally applicable here.

- if (!ItemIdIsUsed(itemid) || ItemIdIsDead(itemid))
+ if (!ItemIdIsUsed(itemid))
continue;

There is a bit of overhead here for the !no_indexes case. I assume it
doesn't matter.

static void
heap_prune_record_dead(PruneState *prstate, OffsetNumber offnum)
{
+ /*
+ * If the relation has no indexes, we can remove dead tuples during
+ * pruning instead of marking their line pointers dead. Set this tuple's
+ * line pointer LP_UNUSED. We hint that tables with indexes are more
+ * likely.
+ */
+ if (unlikely(prstate->no_indexes))
+ {
+ heap_prune_record_unused(prstate, offnum);
+ return;
+ }

I think this should be pushed to the callers. Else you make the
existing function name into another lie.

+ bool recordfreespace;

Not sure if it's necessary to move this to an outer scope like this?
The whole handling of this looks kind of confusing. If we're going to
do it this way, then I think lazy_scan_prune() definitely needs to
document how it handles this function (i.e. might set true to false,
won't set false to true, also meaning). But are we sure we want to let
a local variable with a weird name "leak out" like this?

+ Assert(vacrel->do_index_vacuuming);

Is this related?

--
Robert Haas
EDB: http://www.enterprisedb.com