Re: Reviewing freeze map code

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

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