Re: Reviewing freeze map code

On Sat, May 7, 2016 at 5:40 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> 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.

Fixed.

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

Attached patch optimises skipping pages logic so that blkno can jump to
next_unskippable_block directly while counting the number of all_visible
and all_frozen pages. So we can avoid double checking visibility map.

Regards,

--
Masahiko Sawada