Re: New IndexAM API controlling index vacuum strategies

On Wed, Apr 7, 2021 at 12:16 PM Peter Geoghegan <pg@bowt.ie> wrote:
>
> On Tue, Apr 6, 2021 at 7:05 AM Matthias van de Meent
> <boekewurm+postgres@gmail.com> wrote:
> > If you have updated patches, I'll try to check them this evening (CEST).
>
> Here is v11, which is not too different from v10 as far as the
> truncation stuff goes.
>
> Masahiko should take a look at the last patch again. I renamed the
> GUCs to reflect the fact that we do everything possible to advance
> relfrozenxid in the case where the fail safe mechanism kicks in -- not
> just skipping index vacuuming. It also incorporates your most recent
> round of feedback.

Thank you for updating the patches!

I've done the final round of review:

+       /*
+        * Before beginning heap scan, check if it's already necessary to apply
+        * fail safe speedup
+        */
+       should_speedup_failsafe(vacrel);

Maybe we can call it at an earlier point, for example before
lazy_space_alloc()? That way, we will not need to enable parallelism
if we know it's already an emergency situation.

---
+               msgfmt = _(" %u pages from table (%.2f%% of total) had
%lld dead item identifiers removed\n");
+
+               if (vacrel->nindexes == 0 || (vacrel->do_index_vacuuming &&
+                                             vacrel->num_index_scans == 0))
+                   appendStringInfo(&buf, _("index scan not needed:"));
+               else if (vacrel->do_index_vacuuming &&
vacrel->num_index_scans > 0)
+                   appendStringInfo(&buf, _("index scan needed:"));
+               else
+               {
+                   msgfmt = _(" %u pages from table (%.2f%% of total)
have %lld dead item identifiers\n");
+
+                   if (!vacrel->do_failsafe_speedup)
+                       appendStringInfo(&buf, _("index scan bypassed:"));
+                   else
+                       appendStringInfo(&buf, _("index scan bypassed
due to emergency:"));
+               }
+               appendStringInfo(&buf, msgfmt,
+                                vacrel->lpdead_item_pages,
+                                100.0 * vacrel->lpdead_item_pages /
vacrel->rel_pages,
+                                (long long) vacrel->lpdead_items);

I think we can make it clean if we check vacrel->do_index_vacuuming first.

I've attached the patch that proposes the change for the above points
and can be applied on top of 0002 patch. Please feel free to adopt or
reject it.

For 0001 patch, we call PageTruncateLinePointerArray() only in the
second pass over heap. I think we should note that the second pass is
called only when we found/made LP_DEAD on the page. That is, if all
dead tuples have been marked as LP_UNUSED by HOT pruning, the page
would not be processed by the second pass, resulting in not removing
LP_UNUSED at the end of line pointer array. So think we can call it in
this case, i.g., when lpdead_items is 0 and tuples_deleted > 0 in
lazy_scan_prune().

Regards,

--
Masahiko Sawada
EDB:  https://www.enterprisedb.com/