Date: Wed, 18 Apr 2018 10:36:57 -0300
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: Pavan Deolasee <pavan.deolasee@gmail.com>
Cc: "Wood, Dan" <hexpert@amazon.com>,
	"pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>
Subject: Re: VM map freeze corruption
Message-ID: <20180418133657.gcbg7exanyg5sglw@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="if2usbmasy6ej2yg"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: 
 <CABOikdOecOFE--y0i1wO0CONr54VmyyoP2Er35CVrPYOCN8hZw@mail.gmail.com>
User-Agent: NeoMutt/20170306-137-4415bd-dirty (1.8.0)
Precedence: bulk


--if2usbmasy6ej2yg
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit

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

--if2usbmasy6ej2yg
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment; filename="frozen.patch"

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;
 }
 

--if2usbmasy6ej2yg--