Re: Reviewing freeze map code

On Wed, May 4, 2016 at 8:08 PM, Andres Freund <andres@anarazel.de> wrote:
> 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.

Let's add VACUUM (FORCE) or something like that.

>         /*
>          * 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.

OK.

> -               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.

I wanted to tinker with this logic as little as possible in the
interest of ending up with something that worked.  I would not have
written it this way.

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

+1.

> +                       /*
> +                        * 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.

Yep, I had the same problem.

> 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.

Compared to what we're saving, that's obviously a trivial cost.
That's not to say that we might not want to improve it, but it's
hardly a disaster.

In short: wah, wah, wah.

> 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.

I don't really understand what the concern is here, but if it's not a
problem, let's not spend time trying to clarify.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company