Re: New IndexAM API controlling index vacuum strategies

On Wed, Mar 31, 2021 at 12:01 PM Peter Geoghegan <pg(at)bowt(dot)ie> wrote:
>
> On Sun, Mar 28, 2021 at 9:16 PM Peter Geoghegan <pg(at)bowt(dot)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/