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