Thread: [COMMITTERS] pgsql: Fix traversal of half-frozen update chains
Fix traversal of half-frozen update chains When some tuple versions in an update chain are frozen due to them being older than freeze_min_age, the xmax/xmin trail can become broken. This breaks HOT (and probably other things). A subsequent VACUUM can break things in more serious ways, such as leaving orphan heap-only tuples whose root HOT redirect items were removed. This can be seen because index creation (or REINDEX) complain like ERROR: XX000: failed to find parent tuple for heap-only tuple at (0,7) in table"t" Because of relfrozenxid contraints, we cannot avoid the freezing of the early tuples, so we must cope with the results: whenever we see an Xmin of FrozenTransactionId, consider it a match for whatever the previous Xmax value was. This problem seems to have appeared in 9.3 with multixact changes, though strictly speaking it seems unrelated. Since 9.4 we have commit 37484ad2a "Change the way we mark tuples as frozen", so the fix is simple: just compare the raw Xmin (still stored in the tuple header, since freezing merely set an infomask bit) to the Xmax. But in 9.3 we rewrite the Xmin value to FrozenTransactionId, so the original value is lost and we have nothing to compare the Xmax with. To cope with that case we need to compare the Xmin with FrozenXid, assume it's a match, and hope for the best. Sadly, since you can pg_upgrade a 9.3 instance containing half-frozen pages to newer releases, we need to keep the old check in newer versions too, which seems a bit brittle; I hope we can somehow get rid of that. I didn't optimize the new function for performance. The new coding is probably a bit slower than before, since there is a function call rather than a straight comparison, but I'd rather have it work correctly than be fast but wrong. This is a followup after 20b655224249 fixed a few related problems. Apparently, in 9.6 and up there are more ways to get into trouble, but in 9.3 - 9.5 I cannot reproduce a problem anymore with this patch, so there must be a separate bug. Reported-by: Peter Geoghegan Diagnosed-by: Peter Geoghegan, Michael Paquier, Daniel Wood,Yi Wen Wong, Álvaro Discussion: https://postgr.es/m/CAH2-Wznm4rCrhFAiwKPWTpEw2bXDtgROZK7jWWGucXeH3D1fmA@mail.gmail.com Branch ------ master Details ------- https://git.postgresql.org/pg/commitdiff/a5736bf754c82d8b86674e199e232096c679201d Modified Files -------------- src/backend/access/heap/heapam.c | 52 +++++++++++++++++++++++++++++++++---- src/backend/access/heap/pruneheap.c | 4 +-- src/backend/executor/execMain.c | 6 ++--- src/include/access/heapam.h | 3 +++ 4 files changed, 54 insertions(+), 11 deletions(-) -- Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-committers
On Fri, Oct 6, 2017 at 8:29 AM, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> Fix traversal of half-frozen update chains
I have a question about this (this is taken from the master branch):
> bool
> HeapTupleUpdateXmaxMatchesXmin(TransactionId xmax, HeapTupleHeader htup)
> {
> TransactionId xmin = HeapTupleHeaderGetXmin(htup);
>
> /*
> * If the xmax of the old tuple is identical to the xmin of the new one,
> * it's a match.
> */
> if (TransactionIdEquals(xmax, xmin))
> return true;
>
> /*
> * If the Xmin that was in effect prior to a freeze matches the Xmax,
> * it's good too.
> */
> if (HeapTupleHeaderXminFrozen(htup) &&
> TransactionIdEquals(HeapTupleHeaderGetRawXmin(htup), xmax))
> return true;
>
> /*
> * When a tuple is frozen, the original Xmin is lost, but we know it's a
> * committed transaction. So unless the Xmax is InvalidXid, we don't know
> * for certain that there is a match, but there may be one; and we must
> * return true so that a HOT chain that is half-frozen can be walked
> * correctly.
> *
> * We no longer freeze tuples this way, but we must keep this in order to
> * interpret pre-pg_upgrade pages correctly.
> */
> if (TransactionIdEquals(xmin, FrozenTransactionId) &&
> TransactionIdIsValid(xmax))
> return true;
>
> return false;
> }
Wouldn't this last "if" test, to cover the pg_upgrade case, be better
targeted by comparing *raw* xmin to FrozenTransactionId? You're using
the potentially distinct xmin value returned by
HeapTupleHeaderGetXmin() for the test here. I think we should be
directly targeting tuples frozen on or before 9.4 (prior to
pg_upgrade) instead.
Have I missed something?
--
Peter Geoghegan
--
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers
Peter Geoghegan wrote:
> Wouldn't this last "if" test, to cover the pg_upgrade case, be better
> targeted by comparing *raw* xmin to FrozenTransactionId? You're using
> the potentially distinct xmin value returned by
> HeapTupleHeaderGetXmin() for the test here. I think we should be
> directly targeting tuples frozen on or before 9.4 (prior to
> pg_upgrade) instead.
I also realized we can stop checking (i.e. don't compare xmin to
frozenxid) if the XMIN_FROZEN bits are set -- because in that case the
tuple cannot possibly come from 9.3 frozen. So I think this should do
it.
/** HeapTupleUpdateXmaxMatchesXmin - verify update chain xmax/xmin lineage** Given the new version of a tuple after
someupdate, verify whether the* given Xmax (corresponding to the previous version) matches the tuple's* Xmin, taking
intoaccount that the Xmin might have been frozen after the* update.*/
bool
HeapTupleUpdateXmaxMatchesXmin(TransactionId xmax, HeapTupleHeader htup)
{TransactionId rawxmin = HeapTupleHeaderGetRawXmin(htup);TransactionId xmin = HeapTupleHeaderGetXmin(htup);
/* * If the xmax of the old tuple is identical to the xmin of the new one, * it's a match. */if
(TransactionIdEquals(xmax,xmin)) return true;
/* * If the HEAP_XMIN_FROZEN flag is set, then we know it can only be a match * if the Xmin before the freezing is
identicalto the Xmax -- we can skip * the fuzzy pg_upgraded check below. */if (HeapTupleHeaderXminFrozen(htup)){ if
(TransactionIdEquals(rawxmin,xmax)) return true; else return false;}
/* * When a tuple is frozen, the original Xmin is lost, but we know it's a * committed transaction. So unless the Xmax
isInvalidXid, we don't know * for certain that there is a match, but there may be one; and we must * return true so
thata HOT chain that is half-frozen can be walked * correctly. * * We no longer freeze tuples this way, but we must
keepthis in order to * interpret pre-pg_upgrade pages correctly. */if (TransactionIdEquals(rawxmin,
FrozenTransactionId)&& TransactionIdIsValid(xmax)) return true;
return false;
}
--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
--
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers
On Tue, Oct 17, 2017 at 3:40 AM, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > Peter Geoghegan wrote: > >> Wouldn't this last "if" test, to cover the pg_upgrade case, be better >> targeted by comparing *raw* xmin to FrozenTransactionId? You're using >> the potentially distinct xmin value returned by >> HeapTupleHeaderGetXmin() for the test here. I think we should be >> directly targeting tuples frozen on or before 9.4 (prior to >> pg_upgrade) instead. > > I also realized we can stop checking (i.e. don't compare xmin to > frozenxid) if the XMIN_FROZEN bits are set -- because in that case the > tuple cannot possibly come from 9.3 frozen. So I think this should do > it. > > (New HeapTupleUpdateXmaxMatchesXmin() implementation) Yeah, this is what I had in mind, too. -- Peter Geoghegan -- Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-committers
On Tue, Oct 17, 2017 at 12:23 PM, Peter Geoghegan <pg@bowt.ie> wrote: > On Tue, Oct 17, 2017 at 3:40 AM, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: >> Peter Geoghegan wrote: >> >>> Wouldn't this last "if" test, to cover the pg_upgrade case, be better >>> targeted by comparing *raw* xmin to FrozenTransactionId? You're using >>> the potentially distinct xmin value returned by >>> HeapTupleHeaderGetXmin() for the test here. I think we should be >>> directly targeting tuples frozen on or before 9.4 (prior to >>> pg_upgrade) instead. >> >> I also realized we can stop checking (i.e. don't compare xmin to >> frozenxid) if the XMIN_FROZEN bits are set -- because in that case the >> tuple cannot possibly come from 9.3 frozen. So I think this should do >> it. >> >> (New HeapTupleUpdateXmaxMatchesXmin() implementation) > > Yeah, this is what I had in mind, too. BTW, seems worth fixing this old comment above heap_prepare_freeze_tuple() while you're at it: * NB: It is not enough to set hint bits to indicate something is* committed/invalid -- they might not be set on a standby,or after crash* recovery. We really need to remove old xids. -- Peter Geoghegan -- Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-committers
On Tue, Oct 17, 2017 at 3:40 AM, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > I also realized we can stop checking (i.e. don't compare xmin to > frozenxid) if the XMIN_FROZEN bits are set -- because in that case the > tuple cannot possibly come from 9.3 frozen. So I think this should do > it. When are you planning on committing this? -- Peter Geoghegan -- Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-committers