pg.ddx.io pgsql-hackers@postgresql.org mailing list archive help / color / mirror / Atom feed From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: Pavan Deolasee <pavan.deolasee@gmail.com>
Cc: Wood, Dan <hexpert@amazon.com>
Cc: pgsql-hackers@postgresql.org <pgsql-hackers@postgresql.org>
Subject: Re: VM map freeze corruption
Date: Wed, 18 Apr 2018 10:36:57 -0300
Message-ID: <20180418133657.gcbg7exanyg5sglw@alvherre.pgsql> (raw )
In-Reply-To: <CABOikdOecOFE--y0i1wO0CONr54VmyyoP2Er35CVrPYOCN8hZw@mail.gmail.com >
Pavan Deolasee wrote:
> On Wed, Apr 18, 2018 at 7:37 AM, Wood, Dan <hexpert@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;
}
Attachments:
[text/plain] frozen.patch (2.2K, ../20180418133657.gcbg7exanyg5sglw@alvherre.pgsql/2-frozen.patch)
download | inline diff:
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;
}
view thread (6+ messages) latest in thread
Message-ID: <20180418133657.gcbg7exanyg5sglw@alvherre.pgsql>
Permalink: ../20180418133657.gcbg7exanyg5sglw@alvherre.pgsql/
Also on: postgresql.org/message-id/20180418133657.gcbg7exanyg5sglw@alvherre.pgsql
copy link · copy postgr.es · copy git am
reply Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: alvherre@alvh.no-ip.org, pavan.deolasee@gmail.com, hexpert@amazon.com
Subject: Re: VM map freeze corruption
In-Reply-To: <20180418133657.gcbg7exanyg5sglw@alvherre.pgsql>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox