On 11/27/13 11:15, Andres Freund wrote:
On 2013-11-27 11:01:55 +0200, Heikki Linnakangas wrote:
On 11/27/13 01:21, Andres Freund wrote:
On 2013-11-26 13:32:44 +0100, Andres Freund wrote:
This seems to be the case since
b4b6923e03f4d29636a94f6f4cc2f5cf6298b8c8. I suggest we go back to using
scan_all to determine whether we can set new_frozen_xid. That's a slight
pessimization when we scan a relation fully without explicitly scanning
it in its entirety, but given this isn't the first bug around
scanned_pages/rel_pages I'd rather go that way. The aforementioned
commit wasn't primarily concerned with that.
Alternatively we could just compute new_frozen_xid et al before the
lazy_truncate_heap.
I've gone for the latter in this preliminary patch. Not increasing
relfrozenxid after an initial data load seems like a bit of a shame.
I wonder if we should just do scan_all || vacrelstats->scanned_pages <
vacrelstats->rel_pages?
Hmm, you did (scan_all || vacrelstats->scanned_pages <
vacrelstats->rel_pages) for relminmxid, and just (vacrelstats->scanned_pages
< vacrelstats->rel_pages) for relfrozenxid. That was probably not what you
meant to do, the thing you did for relfrozenxid looks good to me.
I said it's a preliminary patch ;), really, I wasn't sure what of both
to go for.
Does the attached look correct to you?
Looks good.
Ok, committed and backpatched that.
I wonder if we need to integrate any mitigating logic? Currently the
corruption may only become apparent long after it occurred, that's
pretty bad. And instructing people run a vacuum after the ugprade will
cause the corrupted data being lost if they are already 2^31 xids.
Ugh :-(. Running vacuum after the upgrade is the right thing to do to
prevent further damage, but you're right that it will cause any
already-wrapped around data to be lost forever. Nasty.
But integrating logic to fix things into heap_page_prune() looks
somewhat ugly as well.
I think any mitigating logic we might add should go into vacuum. It
should be possible for a DBA to run a command, and after it's finished,
be confident that you're safe. That means vacuum.
Afaics the likelihood of the issue occuring on non-all-visible pages is
pretty low, since they'd need to be skipped due to lock contention
repeatedly.
Hmm. If a page has its visibility-map flag set, but contains a tuple
that appears to be dead because you've wrapped around, vacuum will give
a warning: "page containing dead tuples is marked as all-visible in
relation \"%s\" page %u". So I think if a manual VACUUM FREEZE passes
without giving that warning, that vacuum hasn't destroyed any data. So
we could advise to take a physical backup of the data directory, and run
a manual VACUUM FREEZE on all databases. If it doesn't give a warning,
you're safe from that point onwards. If it does, you'll want to recover
from an older backup, or try to manually salvage just the lost rows from
the backup, and re-index. Ugly, but hopefully rare in practice.
Unfortunately that doesn't mean that you haven't already lost some data.
Wrap-around could've already happened, and vacuum might already have run
and removed some rows. You'll want to examine your logs and grep for
that warning.
- Heikki
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
