Pavan Deolasee wrote:
> On Wed, Apr 18, 2018 at 7:37 AM, Wood, Dan <hexp...@amazon.com> wrote:
> > My analysis is that heap_prepare_freeze_tuple->FreezeMultiXactId()
> > returns FRM_NOOP if the MultiXACT locked rows haven't committed. This
> > results in changed=false and totally_frozen=true(as initialized). When
> > this returns to lazy_scan_heap(), no rows are added to the frozen[] array.
> > Yet, tuple_totally_frozen is true. This means the page is marked frozen in
> > the VM, even though the MultiXACT row wasn't left untouch.
> >
> > A fix to heap_prepare_freeze_tuple() that seems to do the trick is:
> > else
> > {
> > Assert(flags & FRM_NOOP);
> > + totally_frozen = false;
> > }
> >
>
> That's a great find!
Indeed.
This family of bugs (introduced by freeze map changes in 9.6) was
initially fixed in 38e9f90a227d but this spot was missed in that fix.
IMO the cause is the totally_frozen variable, which starts life in a
bogus state (true) and the different code paths can set it to the right
state, or by inaction end up deciding that the initial bogus state was
correct in the first place. Errors of omission are far too easy in that
kind of model, ISTM, so I propose this slightly different patch, which
albeit yet untested seems easier to verify and easier to get right.
--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 4fdb549099..791958a543 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -6800,9 +6800,10 @@ heap_prepare_freeze_tuple(HeapTupleHeader tuple,
xl_heap_freeze_tuple *frz,
bool *totally_frozen_p)
{
bool changed = false;
- bool freeze_xmax = false;
+ bool xmin_frozen = false; /* is xmin frozen after this? */
+ bool freeze_xmax = false; /* whether to freeze xmax now */
+ bool xmax_frozen = false; /* ix xmax frozen after this? */
TransactionId xid;
- bool totally_frozen = true;
frz->frzflags = 0;
frz->t_infomask2 = tuple->t_infomask2;
@@ -6829,10 +6830,11 @@ heap_prepare_freeze_tuple(HeapTupleHeader tuple,
frz->t_infomask |= HEAP_XMIN_FROZEN;
changed = true;
+ xmin_frozen = true;
}
- else
- totally_frozen = false;
}
+ else if (HeapTupleHeaderXminFrozen(tuple))
+ xmin_frozen = true;
/*
* Process xmax. To thoroughly examine the current Xmax value we need
to
@@ -6870,7 +6872,6 @@ heap_prepare_freeze_tuple(HeapTupleHeader tuple,
if (flags & FRM_MARK_COMMITTED)
frz->t_infomask |= HEAP_XMAX_COMMITTED;
changed = true;
- totally_frozen = false;
}
else if (flags & FRM_RETURN_IS_MULTI)
{
@@ -6892,7 +6893,6 @@ heap_prepare_freeze_tuple(HeapTupleHeader tuple,
frz->xmax = newxmax;
changed = true;
- totally_frozen = false;
}
else
{
@@ -6923,9 +6923,10 @@ heap_prepare_freeze_tuple(HeapTupleHeader tuple,
xid)));
freeze_xmax = true;
}
- else
- totally_frozen = false;
}
+ else if (((tuple->t_infomask & HEAP_XMAX_INVALID) == HEAP_XMAX_INVALID)
||
+
!TransactionIdIsValid(HeapTupleHeaderGetRawXmax(tuple)))
+ xmax_frozen = true;
if (freeze_xmax)
{
@@ -6941,6 +6942,7 @@ heap_prepare_freeze_tuple(HeapTupleHeader tuple,
frz->t_infomask2 &= ~HEAP_HOT_UPDATED;
frz->t_infomask2 &= ~HEAP_KEYS_UPDATED;
changed = true;
+ xmax_frozen = true;
}
/*
@@ -6981,7 +6983,7 @@ heap_prepare_freeze_tuple(HeapTupleHeader tuple,
}
}
- *totally_frozen_p = totally_frozen;
+ *totally_frozen_p = xmin_frozen && xmax_frozen;
return changed;
}
