Re: Reviewing freeze map code

On Wed, May 4, 2016 at 8:08 PM, Andres Freund <andres(at)anarazel(dot)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