Re: Reviewing freeze map code

On 2016-05-02 14:48:18 -0700, Andres Freund wrote:
> 77a1d1e Department of second thoughts: remove PD_ALL_FROZEN.

Nothing to say here.


> fd31cd2 Don't vacuum all-frozen pages.

Hm. I do wonder if it's going to bite us that we don't have a way to
actually force vacuuming of the whole table (besides manually rm'ing the
VM). I've more than once seen VACUUM used to try to do some integrity
checking of the database.  How are we actually going to test that the
feature works correctly? They'd have to write checks ontop of
pg_visibility to see whether things are borked.

/* * Compute whether we actually scanned the whole relation. If we did, we * can adjust relfrozenxid and relminmxid. *
*NB: We need to check this before truncating the relation, because that * will change ->rel_pages. */
 

Comment is out-of-date now.


-        if (blkno == next_not_all_visible_block)
+        if (blkno == next_unskippable_block)        {
-            /* Time to advance next_not_all_visible_block */
-            for (next_not_all_visible_block++;
-                 next_not_all_visible_block < nblocks;
-                 next_not_all_visible_block++)
+            /* Time to advance next_unskippable_block */
+            for (next_unskippable_block++;
+                 next_unskippable_block < nblocks;
+                 next_unskippable_block++)

Hm. So we continue with the course of re-processing pages, even if
they're marked all-frozen. For all-visible there at least can be a
benefit by freezing earlier, but for all-frozen pages there's really no
point.  I don't really buy the arguments for the skipping logic. But
even disregarding that, maybe we should skip processing a block if it's
all-frozen (without preventing the page from being read?); as there's no
possible benefit?  Acquring the exclusive/content lock and stuff is far
from free.


Not really related to this patch, but the FORCE_CHECK_PAGE is rather
ugly.


+            /*
+             * The current block is potentially skippable; if we've seen a
+             * long enough run of skippable blocks to justify skipping it, and
+             * we're not forced to check it, then go ahead and skip.
+             * Otherwise, the page must be at least all-visible if not
+             * all-frozen, so we can set all_visible_according_to_vm = true.
+             */
+            if (skipping_blocks && !FORCE_CHECK_PAGE())
+            {
+                /*
+                 * Tricky, tricky.  If this is in aggressive vacuum, the page
+                 * must have been all-frozen at the time we checked whether it
+                 * was skippable, but it might not be any more.  We must be
+                 * careful to count it as a skipped all-frozen page in that
+                 * case, or else we'll think we can't update relfrozenxid and
+                 * relminmxid.  If it's not an aggressive vacuum, we don't
+                 * know whether it was all-frozen, so we have to recheck; but
+                 * in this case an approximate answer is OK.
+                 */
+                if (aggressive || VM_ALL_FROZEN(onerel, blkno, &vmbuffer))
+                    vacrelstats->frozenskipped_pages++;                continue;
+            }

Hm. This indeed seems a bit tricky.  Not sure how to make it easier
though without just ripping out the SKIP_PAGES_THRESHOLD stuff.

Hm. This also doubles the number of VM accesses. While I guess that's
not noticeable most of the time, it's still not nice; especially when a
large relation is entirely frozen, because it'll mean we'll sequentially
go through the visibilityma twice.


I wondered for a minute whether #14057 could cause really bad issues
here
http://www.postgresql.org/message-id/20160331103739.8956.94469@wrigleys.postgresql.org
but I don't see it being more relevant here.


Andres