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.
> - 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.94...@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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
