Re: [HACKERS] [PATCH] Vacuum: Update FSM more frequently - Mailing list pgsql-hackers

I wrote:
> I have to go do something
> else right now, but I'll take a closer look at 0004 later.

OK, so after studying 0004, it seems to me that we could do it more
simply as attached; that is, move the IndexFreeSpaceMapVacuum calls
into btvacuumscan/spgvacuumscan, do them only if we found any recyclable
pages, and drop the calls in btvacuumcleanup/spgvacuumcleanup altogether.

The reason I think this is sufficient is that the scans find and record
every reusable page in the index, no matter whether they were recorded
before or not.  Therefore, if we don't find any such pages, there's
nothing useful in the FSM and no particular urgency about making its
upper pages up-to-date.  It's true that if the FSM is actually corrupt,
leaving that to be fixed retail by searches is probably less efficient
than doing an IndexFreeSpaceMapVacuum call would be --- but *only* if
you assume that the problem is just in the upper pages and the leaf
pages are all fine.  That doesn't seem to be a case we should optimize
for.

I realized that the reason BRIN doesn't go through indexfsm.c is
that it's actually interested in less-than-page-size free space.
So it's using the right API.  However, it still looks to me like
we could win something there by replacing FreeSpaceMapVacuum calls
with FreeSpaceMapVacuumRange calls.  I've not wrapped my head around
the logic completely, but it looks like there are several places
where it calls FreeSpaceMapVacuum to bubble up a free-space update
for just a single page.  If so, that's much less efficient than it
could be.

            regards, tom lane

diff --git a/src/backend/access/nbtree/nbtree.c b/src/backend/access/nbtree/nbtree.c
index 8158508..6fca8e3 100644
*** a/src/backend/access/nbtree/nbtree.c
--- b/src/backend/access/nbtree/nbtree.c
*************** btvacuumcleanup(IndexVacuumInfo *info, I
*** 832,840 ****
          btvacuumscan(info, stats, NULL, NULL, 0);
      }

-     /* Finally, vacuum the FSM */
-     IndexFreeSpaceMapVacuum(info->index);
-
      /*
       * It's quite possible for us to be fooled by concurrent page splits into
       * double-counting some index tuples, so disbelieve any total that exceeds
--- 832,837 ----
*************** btvacuumscan(IndexVacuumInfo *info, Inde
*** 976,981 ****
--- 973,993 ----

      MemoryContextDelete(vstate.pagedelcontext);

+     /*
+      * If we found any recyclable pages (and recorded them in the FSM), then
+      * forcibly update the upper-level FSM pages to ensure that searchers can
+      * find them.  It's possible that the pages were also found during
+      * previous scans and so this is a waste of time, but it's cheap enough
+      * relative to scanning the index that it shouldn't matter much, and
+      * making sure that free pages are available sooner not later seems
+      * worthwhile.
+      *
+      * Note that if no recyclable pages exist, we don't bother vacuuming the
+      * FSM at all.
+      */
+     if (vstate.totFreePages > 0)
+         IndexFreeSpaceMapVacuum(rel);
+
      /* update statistics */
      stats->num_pages = num_pages;
      stats->pages_free = vstate.totFreePages;
diff --git a/src/backend/access/spgist/spgvacuum.c b/src/backend/access/spgist/spgvacuum.c
index 72839cb..a83a4b5 100644
*** a/src/backend/access/spgist/spgvacuum.c
--- b/src/backend/access/spgist/spgvacuum.c
*************** spgvacuumscan(spgBulkDeleteState *bds)
*** 846,851 ****
--- 846,866 ----
      SpGistUpdateMetaPage(index);

      /*
+      * If we found any empty pages (and recorded them in the FSM), then
+      * forcibly update the upper-level FSM pages to ensure that searchers can
+      * find them.  It's possible that the pages were also found during
+      * previous scans and so this is a waste of time, but it's cheap enough
+      * relative to scanning the index that it shouldn't matter much, and
+      * making sure that free pages are available sooner not later seems
+      * worthwhile.
+      *
+      * Note that if no empty pages exist, we don't bother vacuuming the FSM at
+      * all.
+      */
+     if (bds->stats->pages_deleted > 0)
+         IndexFreeSpaceMapVacuum(index);
+
+     /*
       * Truncate index if possible
       *
       * XXX disabled because it's unsafe due to possible concurrent inserts.
*************** dummy_callback(ItemPointer itemptr, void
*** 916,922 ****
  IndexBulkDeleteResult *
  spgvacuumcleanup(IndexVacuumInfo *info, IndexBulkDeleteResult *stats)
  {
-     Relation    index = info->index;
      spgBulkDeleteState bds;

      /* No-op in ANALYZE ONLY mode */
--- 931,936 ----
*************** spgvacuumcleanup(IndexVacuumInfo *info,
*** 926,933 ****
      /*
       * We don't need to scan the index if there was a preceding bulkdelete
       * pass.  Otherwise, make a pass that won't delete any live tuples, but
!      * might still accomplish useful stuff with redirect/placeholder cleanup,
!      * and in any case will provide stats.
       */
      if (stats == NULL)
      {
--- 940,947 ----
      /*
       * We don't need to scan the index if there was a preceding bulkdelete
       * pass.  Otherwise, make a pass that won't delete any live tuples, but
!      * might still accomplish useful stuff with redirect/placeholder cleanup
!      * and/or FSM housekeeping, and in any case will provide stats.
       */
      if (stats == NULL)
      {
*************** spgvacuumcleanup(IndexVacuumInfo *info,
*** 940,948 ****
          spgvacuumscan(&bds);
      }

-     /* Finally, vacuum the FSM */
-     IndexFreeSpaceMapVacuum(index);
-
      /*
       * It's quite possible for us to be fooled by concurrent tuple moves into
       * double-counting some index tuples, so disbelieve any total that exceeds
--- 954,959 ----