Date: Wed, 18 Jun 2014 18:51:31 -0400
From: Alvaro Herrera <alvherre@2ndquadrant.com>
To: Bruce Momjian <bruce@momjian.us>
Cc: Andres Freund <andres@2ndquadrant.com>,
 PostgreSQL Bugs <pgsql-bugs@postgresql.org>
Subject: Re: pg_upgrade < 9.3 -> >=9.3 misses a step around multixacts
Message-ID: <20140618225131.GN18688@eldon.alvh.no-ip.org>
References: <20140530121631.GE25431@alap3.anarazel.de>
 <20140530132916.GM28490@momjian.us>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="eMnpOGXCMazMAbfp"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20140530132916.GM28490@momjian.us>
User-Agent: Mutt/1.5.21 (2010-09-15)
Precedence: bulk
Sender: pgsql-bugs-owner@postgresql.org


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

Bruce Momjian wrote:
> On Fri, May 30, 2014 at 02:16:31PM +0200, Andres Freund wrote:
> > Hi,
> > 
> > When upgrading a < 9.3 cluster pg_upgrade doesn't bother to keep the old
> > multixacts around because they won't be read after the upgrade (and
> > aren't compatible). It just resets the new cluster's nextMulti to the
> > old + 1.
> > Unfortunately that means that there'll be a offsets/0000 file created by
> > initdb around. Sounds harmless enough, but that'll actually cause
> > problems if the old cluster had a nextMulti that's bigger than that
> > page.

I've been playing with this a bit.  Here's a patch that just does
rmtree() of the problematic file during pg_upgrade, as proposed by
Andres, which solves the problem.  Note that this patch removes the 0000
file in both cases: when upgrading from 9.2, and when upgrading from
9.3+.  The former case is the bug that Andres reported.  In the second
case, we overwrite the files with the ones from the old cluster; if
there's a lingering 0000 file in the new cluster, it would cause
problems as well.  (Really, I don't see any reason to think that these
two cases are any different.)

> This is a bug in 9.3 pg_upgrade as well?  Why has no one reported it
> before?

I think one reason is that not all upgrades see an issue here; for old
clusters that haven't gone beyond the 0000 offset file, there is no
problem.  For clusters that have gone beyond 0000 but not by much, the
file will be deleted during the first truncation.  It only becomes a
problem if the cluster is close enough to 2^31.  Another thing to keep
in consideration is that initdb initializes all databases' datminmxid to
1.  If the old cluster was past the 2^31 point, it means the datminmxid
doesn't move from 1 until the actual wraparound.


I noticed another problem while trying to reproduce it.  It only happens
in assert-enabled builds: when FreezeMultiXactId() sees an old multi, it
asserts that it mustn't be running anymore, which is fine except that if
the multi value is one that survived a pg_upgrade cycle from 9.2 or
older (i.e. it was a share-locked tuple in the old cluster), an error is
raised when the assertion is checked.  Since it doesn't make sense to
examine its running status at that point, the attached patch simply
disables the assertion in that case.

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

--eMnpOGXCMazMAbfp
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="multiupgrade.patch"

diff --git a/contrib/pg_upgrade/pg_upgrade.c b/contrib/pg_upgrade/pg_upgrade.c
index cd70b6a..74e5b63 100644
--- a/contrib/pg_upgrade/pg_upgrade.c
+++ b/contrib/pg_upgrade/pg_upgrade.c
@@ -398,6 +398,8 @@ copy_subdir_files(char *subdir)
 static void
 copy_clog_xlog_xid(void)
 {
+	char	multioffdir[MAXPGPATH];
+
 	/* copy old commit logs to new data dir */
 	copy_subdir_files("pg_clog");
 
@@ -410,6 +412,16 @@ copy_clog_xlog_xid(void)
 	check_ok();
 
 	/*
+	 * Remove pg_multixact/offset files before going any further.  initdb
+	 * created file 0000 there, which is problematic for future multixact
+	 * truncation.  Note we remove any other junk in the subdir alongside the
+	 * 0000 file.
+	 */
+	snprintf(multioffdir, sizeof(multioffdir),
+			 "%s/pg_multixact/offsets", new_cluster.pgdata);
+	rmtree(multioffdir, false);
+
+	/*
 	 * If the old server is before the MULTIXACT_FORMATCHANGE_CAT_VER change
 	 * (see pg_upgrade.h) and the new server is after, then we don't copy
 	 * pg_multixact files, but we need to reset pg_control so that the new
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index ccdcb5b..baed722 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -5326,8 +5326,14 @@ FreezeMultiXactId(MultiXactId multi, uint16 t_infomask,
 		 * was a locker only, it can be removed without any further
 		 * consideration; but if it contained an update, we might need to
 		 * preserve it.
+		 *
+		 * Don't assert MultiXactIdIsRunning if the multi came from a
+		 * pg_upgrade'd share-locked tuple, though, as doing that causes an
+		 * error to be raised unnecessarily.
 		 */
-		Assert(!MultiXactIdIsRunning(multi));
+		Assert((!(t_infomask & HEAP_LOCK_MASK) &&
+				HEAP_XMAX_IS_LOCKED_ONLY(t_infomask)) ||
+			   !MultiXactIdIsRunning(multi));
 		if (HEAP_XMAX_IS_LOCKED_ONLY(t_infomask))
 		{
 			*flags |= FRM_INVALIDATE_XMAX;

--eMnpOGXCMazMAbfp
Content-Type: text/plain
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0


-- 
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

--eMnpOGXCMazMAbfp--