Re: decoupling table and index vacuum

On Thu, Sep 16, 2021 at 7:09 AM Peter Geoghegan <pg@bowt.ie> wrote:
>
> On Wed, Apr 21, 2021 at 8:21 AM Robert Haas <robertmhaas@gmail.com> wrote:
> > Now, the reason for this is that when we discover dead TIDs, we only
> > record them in memory, not on disk. So, as soon as VACUUM ends, we
> > lose all knowledge of whether those TIDs were and must rediscover
> > them. Suppose we didn't do this, and instead had a "dead TID" fork for
> > each table. Suppose further that this worked like a conveyor belt,
> > similar to WAL, where every dead TID we store into the fork is
> > assigned an identifying 64-bit number that is never reused.
>
> Enabling index-only scans is a good enough reason to pursue this
> project, even on its own.

+1

> I attached a POC autovacuum logging instrumentation patch that shows
> how VACUUM uses *and* sets VM bits.

Logging how vacuum uses and sets VM bits seems a good idea.

I've read the proposed PoC patch. Probably it's better to start a new
thread for this patch and write the comment for it there but let me
leave one comment on the patch:

With the patch, we increment allfrozen_pages counter, which is used to
determine whether or not we advance relfrozenxid and relminmxid, at
two places:

@@ -1141,7 +1201,9 @@ lazy_scan_heap(LVRelState *vacrel, VacuumParams
*params, bool aggressive)
                                  * in this case an approximate answer is OK.
                                  */
                                 if (aggressive ||
VM_ALL_FROZEN(vacrel->rel, blkno, &vmbuffer))
-                                        vacrel->frozenskipped_pages++;
+                                        vacrel->allfrozen_pages++;
+                                else
+                                        vacrel->allvisible_pages++;
                                 continue;

@@ -1338,6 +1400,8 @@ lazy_scan_heap(LVRelState *vacrel, VacuumParams
*params, bool aggressive)
                          */
                         if (!PageIsAllVisible(page))
                         {
+                                vacrel->allfrozen_pages++;
+

I think that we will end up doubly counting the page as scanned_pages
and allfrozen_pages due to the newly added latter change. This seems
wrong to me because we calculate as follows:

@@ -644,7 +656,7 @@ heap_vacuum_rel(Relation rel, VacuumParams *params,
          * NB: We need to check this before truncating the relation,
because that
          * will change ->rel_pages.
          */
-        if ((vacrel->scanned_pages + vacrel->frozenskipped_pages)
+        if ((vacrel->scanned_pages + vacrel->allfrozen_pages)
                 < vacrel->rel_pages)
         {
                 Assert(!aggressive);

Regards,

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