On Sat, May 7, 2016 at 5:40 AM, Robert Haas <[email protected]> wrote: > On Wed, May 4, 2016 at 8:08 PM, Andres Freund <[email protected]> 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
fix_freeze_map_fd31cd2.patch
Description: Binary data
-- Sent via pgsql-hackers mailing list ([email protected]) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
