Re: New IndexAM API controlling index vacuum strategies

On Wed, Mar 31, 2021 at 12:01 PM Peter Geoghegan <pg@bowt.ie> wrote:
>
> On Sun, Mar 28, 2021 at 9:16 PM Peter Geoghegan <pg@bowt.ie> wrote:
> > And now here's v8, which has the following additional cleanup:
>
> And here's v9, which has improved commit messages for the first 2
> patches, and many small tweaks within all 4 patches.
>
> The most interesting change is that lazy_scan_heap() now has a fairly
> elaborate assertion that verifies that its idea about whether or not
> the page is all_visible and all_frozen is shared by
> heap_page_is_all_visible() -- this is a stripped down version of the
> logic that now lives in lazy_scan_heap(). It exists so that the second
> pass over the heap can set visibility map bits.

Thank you for updating the patches.

Both 0001 and 0002 patch refactors the whole lazy vacuum code. Can we
merge them? I basically agree with the refactoring made by 0001 patch
but I'm concerned a bit that having such a large refactoring at very
close to feature freeze could be risky. We would need more eyes to
review during stabilization.

Here are some comments on 0001 patch:

-/*
- * Macro to check if we are in a parallel vacuum.  If true, we are in the
- * parallel mode and the DSM segment is initialized.
- */
-#define ParallelVacuumIsActive(lps) PointerIsValid(lps)
-

I think it's more clear to use this macro. The macro can be like this:

ParallelVacuumIsActive(vacrel) (((LVRelState) vacrel)->lps != NULL)

---
 /*
- * LVDeadTuples stores the dead tuple TIDs collected during the heap scan.
- * This is allocated in the DSM segment in parallel mode and in local memory
- * in non-parallel mode.
+ * LVDeadTuples stores TIDs that are gathered during pruning/the initial heap
+ * scan.  These get deleted from indexes during index vacuuming.  They're then
+ * removed from the heap during a second heap pass that performs heap
+ * vacuuming.
  */

The second sentence of the removed lines still seems to be useful
information for readers?

---
-                                *
-                                * Note that vacrelstats->dead_tuples
could have tuples which
-                                * became dead after HOT-pruning but
are not marked dead yet.
-                                * We do not process them because it's
a very rare condition,
-                                * and the next vacuum will process them anyway.

Maybe the above comments should not be removed by 0001 patch.

---
+       /* Free resources managed by lazy_space_alloc() */
+       lazy_space_free(vacrel);

and

+/* Free space for dead tuples */
+static void
+lazy_space_free(LVRelState *vacrel)
+{
+       if (!vacrel->lps)
+               return;
+
+       /*
+        * End parallel mode before updating index statistics as we cannot write
+        * during parallel mode.
+        */
+       end_parallel_vacuum(vacrel);

Looking at the comments, I thought that this function also frees
palloc'd dead tuple space but it doesn't. It seems to more clear that
doing pfree(vacrel->dead_tuples) here or not creating
lazy_space_free().

Also, the comment for end_paralle_vacuum() looks not relevant with
this function. Maybe we can update to:

/* Exit parallel mode and free the parallel context */

---
+       if (shared_istat)
+       {
+               /* Get the space for IndexBulkDeleteResult */
+               bulkdelete_res = &(shared_istat->istat);
+
+               /*
+                * Update the pointer to the corresponding
bulk-deletion result if
+                * someone has already updated it.
+                */
+               if (shared_istat->updated && istat == NULL)
+                       istat = bulkdelete_res;
+       }

(snip)

+       if (shared_istat && !shared_istat->updated && istat != NULL)
+       {
+               memcpy(bulkdelete_res, istat, sizeof(IndexBulkDeleteResult));
+               shared_istat->updated = true;
+
+               /*
+                * Now that top-level indstats[idx] points to the DSM
segment, we
+                * don't need the locally allocated results.
+                */
+               pfree(istat);
+               istat = bulkdelete_res;
+       }
+
+       return istat;

If we have parallel_process_one_index() return the address of
IndexBulkDeleteResult, we can simplify the first part above. Also, it
seems better to use a separate variable from istat to store the
result. How about the following structure?

    IndexBulkDeleteResult *istat_res;

    /*
     * Update the pointer of the corresponding bulk-deletion result if
     * someone has already updated it.
     */
    if (shared_istat && shared_istat->updated && istat == NULL)
        istat = shared_istat->istat;

    /* Do vacuum or cleanup of the index */
    if (lvshared->for_cleanup)
        istat_res = lazy_cleanup_one_index(indrel, istat, ...);
    else
        istat_res = lazy_vacuum_one_index(indrel, istat, ...);

    /*
     * (snip)
     */
    if (shared_istat && !shared_istat->updated && istat_res != NULL)
    {
        memcpy(shared_istat->istat, istat_res, sizeof(IndexBulkDeleteResult));
        shared_istat->updated = true;

        /* free the locally-allocated bulk-deletion result */
        pfree(istat_res);

        /* return the pointer to the result on the DSM segment */
        return shared_istat->istat;
    }

    return istat_res;

Comment on 0002 patch:

+           /* This won't have changed: */
+           Assert(savefreespace && freespace == PageGetHeapFreeSpace(page));

This assertion can be false because freespace can be 0 if the page's
PD_HAS_FREE_LINES hint can wrong. Since lazy_vacuum_heap_page() fixes
it, PageGetHeapFreeSpace(page) in the assertion returns non-zero
value.

And, here are commends on 0004 patch:

+               ereport(WARNING,
+                               (errmsg("abandoned index vacuuming of
table \"%s.%s.%s\" as a fail safe after %d index scans",
+                                               get_database_name(MyDatabaseId),
+                                               vacrel->relname,
+                                               vacrel->relname,
+                                               vacrel->num_index_scans),

The first vacrel->relname should be vacrel->relnamespace.

I think we can use errmsg_plural() for "X index scans" part.

---
+               ereport(elevel,
+                               (errmsg("\"%s\": index scan bypassed:
%u pages from table (%.2f%% of total) have %lld dead item
identifiers",
+                                               vacrel->relname,
vacrel->rel_pages,
+                                               100.0 *
vacrel->lpdead_item_pages / vacrel->rel_pages,
+                                               (long long)
vacrel->lpdead_items)));

We should use vacrel->lpdead_item_pages instead of vacrel->rel_pages

---
+               /* Stop applying cost limits from this point on */
+               VacuumCostActive = false;
+               VacuumCostBalance = 0;
+       }

I agree with the idea of disabling vacuum delay in emergency cases.
But why do we do that only in the case of the table with indexes? I
think this optimization is helpful even in the table with no indexes.
We can check the XID wraparound emergency by calling
vacuum_xid_limit_emergency() at some point to disable vacuum delay?

---
+                                       if (vacrel->do_index_cleanup)
+                                               appendStringInfo(&buf,
_("index scan bypassed:"));
+                                       else
+                                               appendStringInfo(&buf,
_("index scan bypassed due to emergency:")\
);
+                                       msgfmt = _(" %u pages from
table (%.2f%% of total) have %lld dead item identifiers\n");
+                               }

Both vacrel->do_index_vacuuming and vacrel->do_index_cleanup can be
false also when INDEX_CLEANUP is off. So autovacuum could wrongly
report emergency if the table's vacuum_index_vacuum reloption is
false.

Regards,

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