Re: New IndexAM API controlling index vacuum strategies - Mailing list pgsql-hackers

On Sat, Mar 20, 2021 at 11:05 AM Peter Geoghegan <pg@bowt.ie> wrote:
>
> On Wed, Mar 17, 2021 at 7:55 PM Peter Geoghegan <pg@bowt.ie> wrote:
> > Attached patch series splits everything up. There is now a large patch
> > that removes the tupgone special case, and a second patch that
> > actually adds code that dynamically decides to not do index vacuuming
> > in cases where (for whatever reason) it doesn't seem useful.
>
> Attached is v4. This revision of the patch series is split up into
> smaller pieces for easier review. There are now 3 patches in the
> series:

Thank you for the patches!

>
> 1. A refactoring patch that takes code from lazy_scan_heap() and
> breaks it into several new functions.
>
> Not too many changes compared to the last revision here (mostly took
> things out and put them in the second patch).

I've looked at this 0001 patch and here are some review comments:

+/*
+ *     scan_prune_page() -- lazy_scan_heap() pruning and freezing.
+ *
+ * Caller must hold pin and buffer cleanup lock on the buffer.
+ *
+ * Prior to PostgreSQL 14 there were very rare cases where lazy_scan_heap()
+ * treated tuples that still had storage after pruning as DEAD.  That happened
+ * when heap_page_prune() could not prune tuples that were nevertheless deemed
+ * DEAD by its own HeapTupleSatisfiesVacuum() call.  This created rare hard to
+ * test cases.  It meant that there was no very sharp distinction between DEAD
+ * tuples and tuples that are to be kept and be considered for freezing inside
+ * heap_prepare_freeze_tuple().  It also meant that lazy_vacuum_page() had to
+ * be prepared to remove items with storage (tuples with tuple headers) that
+ * didn't get pruned, which created a special case to handle recovery
+ * conflicts.
+ *
+ * The approach we take here now (to eliminate all of this complexity) is to
+ * simply restart pruning in these very rare cases -- cases where a concurrent
+ * abort of an xact makes our HeapTupleSatisfiesVacuum() call disagrees with
+ * what heap_page_prune() thought about the tuple only microseconds earlier.
+ *
+ * Since we might have to prune a second time here, the code is structured to
+ * use a local per-page copy of the counters that caller accumulates.  We add
+ * our per-page counters to the per-VACUUM totals from caller last of all, to
+ * avoid double counting.

Those comments should be a part of 0002 patch?

---
+                       pc.num_tuples += 1;
+                       ps->hastup = true;
+
+                       /*
+                        * Each non-removable tuple must be checked to
see if it needs
+                        * freezing
+                        */
+                       if (heap_prepare_freeze_tuple(tuple.t_data,
+
           RelFrozenXid, RelMinMxid,
+
           FreezeLimit, MultiXactCutoff,
+
           &frozen[nfrozen],
+
           &tuple_totally_frozen))
+                               frozen[nfrozen++].offset = offnum;
+
+                       pc.num_tuples += 1;
+                       ps->hastup = true;

pc.num_tuples is incremented twice. ps->hastup = true is also duplicated.

---
In step 7, with the patch, we save the freespace of the page and do
lazy_vacuum_page(). But should it be done in reverse?

---
+static void
+two_pass_strategy(Relation onerel, LVRelStats *vacrelstats,
+                                 Relation *Irel,
IndexBulkDeleteResult **indstats, int nindexes,
+                                 LVParallelState *lps,
VacOptTernaryValue index_cleanup)

How about renaming to vacuum_two_pass_strategy() or something to clear
this function is used to vacuum?

---
+               /*
+                * skipped index vacuuming.  Make log report that
lazy_vacuum_heap
+                * would've made.
+                *
+                * Don't report tups_vacuumed here because it will be
zero here in
+                * common case where there are no newly pruned LP_DEAD
items for this
+                * VACUUM.  This is roughly consistent with
lazy_vacuum_heap(), and
+                * the similar !useindex ereport() at the end of
lazy_scan_heap().
+                * Note, however, that has_dead_items_pages is # of
heap pages with
+                * one or more LP_DEAD items (could be from us or from another
+                * VACUUM), not # blocks scanned.
+                */
+               ereport(elevel,
+                               (errmsg("\"%s\": INDEX_CLEANUP off
forced VACUUM to not totally remove %d pruned items",
+                                               vacrelstats->relname,
+
vacrelstats->dead_tuples->num_tuples)));

It seems that the comment needs to be updated.

>
> 2. A patch to remove the tupgone case.
>
> Severa new and interesting changes here -- see below.
>
> 3. The patch to optimize VACUUM by teaching it to skip index and heap
> vacuuming in certain cases where we only expect a very small benefit.

I’ll review the other two patches tomorrow.

>
> We now go further with removing unnecessary stuff in WAL records in
> the second patch. We also go further with simplifying heap page
> vacuuming more generally.
>
> I have invented a new record that is only used by heap page vacuuming.
> This means that heap page pruning and heap page vacuuming no longer
> share the same xl_heap_clean/XLOG_HEAP2_CLEAN WAL record (which is
> what they do today, on master). Rather, there are two records:
>
> * XLOG_HEAP2_PRUNE/xl_heap_prune -- actually just the new name for
> xl_heap_clean, renamed to reflect the fact that only pruning uses it.
>
> * XLOG_HEAP2_VACUUM/xl_heap_vacuum -- this one is truly new, though
> it's actually just a very primitive version of xl_heap_prune -- since
> of course heap page vacuuming is now so much simpler.

I didn't look at the 0002 patch in-depth but the main difference
between those two WAL records is that XLOG_HEAP2_PRUNE has the offset
numbers of unused, redirected, and dead whereas XLOG_HEAP2_VACUUM has
only the offset numbers of unused?

Regards,

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