From: Tom Lane <tgl@sss.pgh.pa.us>
To: Andres Freund <andres@2ndquadrant.com>
cc: Bruce Momjian <bruce@momjian.us>,
 Alvaro Herrera <alvherre@2ndquadrant.com>,
 PostgreSQL Bugs <pgsql-bugs@postgresql.org>
Subject: Re: pg_upgrade < 9.3 -> >=9.3 misses a step around multixacts
In-reply-to: <22178.1405903964@sss.pgh.pa.us>
References: <16274.1405892584@sss.pgh.pa.us>
 <20140720220253.GE5974@alap3.anarazel.de> <17255.1405894611@sss.pgh.pa.us>
 <20140720223010.GF5974@alap3.anarazel.de> <17935.1405895946@sss.pgh.pa.us>
 <20140720225653.GG5974@alap3.anarazel.de> <18673.1405897470@sss.pgh.pa.us>
 <18851.1405897900@sss.pgh.pa.us> <20140720233125.GI5974@alap3.anarazel.de>
 <19458.1405899435@sss.pgh.pa.us> <20140720234925.GJ5974@alap3.anarazel.de>
 <22178.1405903964@sss.pgh.pa.us>
Comments: In-reply-to Tom Lane <tgl@sss.pgh.pa.us>
 message dated "Sun, 20 Jul 2014 20:52:44 -0400"
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0"
Content-ID: <26482.1405912722.0@sss.pgh.pa.us>
Date: Sun, 20 Jul 2014 23:19:30 -0400
Message-ID: <26504.1405912770@sss.pgh.pa.us>
Precedence: bulk
Sender: pgsql-bugs-owner@postgresql.org

------- =_aaaaaaaaaa0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <26482.1405912722.1@sss.pgh.pa.us>

I wrote:
> Here's a draft patch for this.  I think this will fix all cases where
> the "1" minmxid inserted by previous pg_upgrade versions is actually
> in the future at the time we run VACUUM.  We would still be at risk if
> it had been in the future when pg_upgrade ran but no longer is now,
> since that would mean there could be non-lock-only mxids on disk that
> are older than "1".  However, for the reasons discussed upthread, it
> seems fairly unlikely to me that people would actually get burnt in
> practice, so I'm satisfied with doing this much and no more.

Ah, belay that: as coded, that would allow truncation of clog/multixact
as soon as any one relation in any one database had sane
frozenxid/minmxid.  If we want to have any pretense of being safe, we have
to postpone truncation until *all* relations have been vacuumed.  So more
like the attached, instead.

			regards, tom lane


------- =_aaaaaaaaaa0
Content-Type: text/x-diff;
 name="overwrite-nonsensical-frozenxid-or-minmxid-2.patch";
 charset="us-ascii"
Content-ID: <26482.1405912722.2@sss.pgh.pa.us>
Content-Description: overwrite-nonsensical-frozenxid-or-minmxid-2.patch
Content-Transfer-Encoding: quoted-printable

diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c
index 8822a15..ec9a7b6 100644
*** a/src/backend/commands/vacuum.c
--- b/src/backend/commands/vacuum.c
*************** static BufferAccessStrategy vac_strategy
*** 66,72 ****
=20=20
  /* non-export function prototypes */
  static List *get_rel_oids(Oid relid, const RangeVar *vacrel);
! static void vac_truncate_clog(TransactionId frozenXID, MultiXactId minMul=
ti);
  static bool vacuum_rel(Oid relid, VacuumStmt *vacstmt, bool do_toast,
  		   bool for_wraparound);
=20=20
--- 66,75 ----
=20=20
  /* non-export function prototypes */
  static List *get_rel_oids(Oid relid, const RangeVar *vacrel);
! static void vac_truncate_clog(TransactionId frozenXID,
! 				  MultiXactId minMulti,
! 				  TransactionId lastSaneFrozenXid,
! 				  MultiXactId lastSaneMinMulti);
  static bool vacuum_rel(Oid relid, VacuumStmt *vacstmt, bool do_toast,
  		   bool for_wraparound);
=20=20
*************** vac_update_relstats(Relation relation,
*** 733,751 ****
  	}
=20=20
  	/*
! 	 * relfrozenxid should never go backward.  Caller can pass
! 	 * InvalidTransactionId if it has no new data.
  	 */
  	if (TransactionIdIsNormal(frozenxid) &&
! 		TransactionIdPrecedes(pgcform->relfrozenxid, frozenxid))
  	{
  		pgcform->relfrozenxid =3D frozenxid;
  		dirty =3D true;
  	}
=20=20
! 	/* relminmxid must never go backward, either */
  	if (MultiXactIdIsValid(minmulti) &&
! 		MultiXactIdPrecedes(pgcform->relminmxid, minmulti))
  	{
  		pgcform->relminmxid =3D minmulti;
  		dirty =3D true;
--- 736,768 ----
  	}
=20=20
  	/*
! 	 * Update relfrozenxid, unless caller passed InvalidTransactionId
! 	 * indicating it has no new data.
! 	 *
! 	 * Ordinarily, we don't let relfrozenxid go backwards: if things are
! 	 * working correctly, the only way the new frozenxid could be older would
! 	 * be if a previous VACUUM was done with a tighter freeze_min_age, in
! 	 * which case we don't want to forget the work it already did.  However,
! 	 * if the stored relfrozenxid is "in the future", then it must be corrupt
! 	 * and it seems best to overwrite it with the cutoff we used this time.
! 	 * See vac_update_datfrozenxid() concerning what we consider to be "in t=
he
! 	 * future".
  	 */
  	if (TransactionIdIsNormal(frozenxid) &&
! 		pgcform->relfrozenxid !=3D frozenxid &&
! 		(TransactionIdPrecedes(pgcform->relfrozenxid, frozenxid) ||
! 		 TransactionIdPrecedes(GetOldestXmin(NULL, true),
! 							   pgcform->relfrozenxid)))
  	{
  		pgcform->relfrozenxid =3D frozenxid;
  		dirty =3D true;
  	}
=20=20
! 	/* Similarly for relminmxid */
  	if (MultiXactIdIsValid(minmulti) &&
! 		pgcform->relminmxid !=3D minmulti &&
! 		(MultiXactIdPrecedes(pgcform->relminmxid, minmulti) ||
! 		 MultiXactIdPrecedes(GetOldestMultiXactId(), pgcform->relminmxid)))
  	{
  		pgcform->relminmxid =3D minmulti;
  		dirty =3D true;
*************** vac_update_relstats(Relation relation,
*** 772,779 ****
   *		truncate pg_clog and pg_multixact.
   *
   *		We violate transaction semantics here by overwriting the database's
!  *		existing pg_database tuple with the new value.  This is reasonably
!  *		safe since the new value is correct whether or not this transaction
   *		commits.  As with vac_update_relstats, this avoids leaving dead tuples
   *		behind after a VACUUM.
   */
--- 789,796 ----
   *		truncate pg_clog and pg_multixact.
   *
   *		We violate transaction semantics here by overwriting the database's
!  *		existing pg_database tuple with the new values.  This is reasonably
!  *		safe since the new values are correct whether or not this transaction
   *		commits.  As with vac_update_relstats, this avoids leaving dead tuples
   *		behind after a VACUUM.
   */
*************** vac_update_datfrozenxid(void)
*** 786,792 ****
--- 803,812 ----
  	SysScanDesc scan;
  	HeapTuple	classTup;
  	TransactionId newFrozenXid;
+ 	TransactionId lastSaneFrozenXid;
  	MultiXactId newMinMulti;
+ 	MultiXactId lastSaneMinMulti;
+ 	bool		bogus =3D false;
  	bool		dirty =3D false;
=20=20
  	/*
*************** vac_update_datfrozenxid(void)
*** 795,807 ****
  	 * committed pg_class entries for new tables; see AddNewRelationTuple().
  	 * So we cannot produce a wrong minimum by starting with this.
  	 */
! 	newFrozenXid =3D GetOldestXmin(NULL, true);
=20=20
  	/*
  	 * Similarly, initialize the MultiXact "min" with the value that would be
  	 * used on pg_class for new tables.  See AddNewRelationTuple().
  	 */
! 	newMinMulti =3D GetOldestMultiXactId();
=20=20
  	/*
  	 * We must seqscan pg_class to find the minimum Xid, because there is no
--- 815,827 ----
  	 * committed pg_class entries for new tables; see AddNewRelationTuple().
  	 * So we cannot produce a wrong minimum by starting with this.
  	 */
! 	newFrozenXid =3D lastSaneFrozenXid =3D GetOldestXmin(NULL, true);
=20=20
  	/*
  	 * Similarly, initialize the MultiXact "min" with the value that would be
  	 * used on pg_class for new tables.  See AddNewRelationTuple().
  	 */
! 	newMinMulti =3D lastSaneMinMulti =3D GetOldestMultiXactId();
=20=20
  	/*
  	 * We must seqscan pg_class to find the minimum Xid, because there is no
*************** vac_update_datfrozenxid(void)
*** 828,833 ****
--- 848,868 ----
  		Assert(TransactionIdIsNormal(classForm->relfrozenxid));
  		Assert(MultiXactIdIsValid(classForm->relminmxid));
=20=20
+ 		/*
+ 		 * If things are working properly, no relation should have a
+ 		 * relfrozenxid or relminmxid that is "in the future".  However, such
+ 		 * cases have been known to arise due to bugs in pg_upgrade.  If we
+ 		 * see any entries that are "in the future", chicken out and don't do
+ 		 * anything.  This ensures we won't truncate clog before those
+ 		 * relations have been scanned and cleaned up.
+ 		 */
+ 		if (TransactionIdPrecedes(lastSaneFrozenXid, classForm->relfrozenxid) ||
+ 			MultiXactIdPrecedes(lastSaneMinMulti, classForm->relminmxid))
+ 		{
+ 			bogus =3D true;
+ 			break;
+ 		}
+=20
  		if (TransactionIdPrecedes(classForm->relfrozenxid, newFrozenXid))
  			newFrozenXid =3D classForm->relfrozenxid;
=20=20
*************** vac_update_datfrozenxid(void)
*** 839,844 ****
--- 874,883 ----
  	systable_endscan(scan);
  	heap_close(relation, AccessShareLock);
=20=20
+ 	/* chicken out if bogus data found */
+ 	if (bogus)
+ 		return;
+=20
  	Assert(TransactionIdIsNormal(newFrozenXid));
  	Assert(MultiXactIdIsValid(newMinMulti));
=20=20
*************** vac_update_datfrozenxid(void)
*** 852,872 ****
  	dbform =3D (Form_pg_database) GETSTRUCT(tuple);
=20=20
  	/*
! 	 * Don't allow datfrozenxid to go backward (probably can't happen anyway=
);
! 	 * and detect the common case where it doesn't go forward either.
  	 */
! 	if (TransactionIdPrecedes(dbform->datfrozenxid, newFrozenXid))
  	{
  		dbform->datfrozenxid =3D newFrozenXid;
  		dirty =3D true;
  	}
=20=20
! 	/* ditto */
! 	if (MultiXactIdPrecedes(dbform->datminmxid, newMinMulti))
  	{
  		dbform->datminmxid =3D newMinMulti;
  		dirty =3D true;
  	}
=20=20
  	if (dirty)
  		heap_inplace_update(relation, tuple);
--- 891,920 ----
  	dbform =3D (Form_pg_database) GETSTRUCT(tuple);
=20=20
  	/*
! 	 * As in vac_update_relstats(), we ordinarily don't want to let
! 	 * datfrozenxid go backward; but if it's "in the future" then it must be
! 	 * corrupt and it seems best to overwrite it.
  	 */
! 	if (dbform->datfrozenxid !=3D newFrozenXid &&
! 		(TransactionIdPrecedes(dbform->datfrozenxid, newFrozenXid) ||
! 		 TransactionIdPrecedes(lastSaneFrozenXid, dbform->datfrozenxid)))
  	{
  		dbform->datfrozenxid =3D newFrozenXid;
  		dirty =3D true;
  	}
+ 	else
+ 		newFrozenXid =3D dbform->datfrozenxid;
=20=20
! 	/* Ditto for datminmxid */
! 	if (dbform->datminmxid !=3D newMinMulti &&
! 		(MultiXactIdPrecedes(dbform->datminmxid, newMinMulti) ||
! 		 MultiXactIdPrecedes(lastSaneMinMulti, dbform->datminmxid)))
  	{
  		dbform->datminmxid =3D newMinMulti;
  		dirty =3D true;
  	}
+ 	else
+ 		newMinMulti =3D dbform->datminmxid;
=20=20
  	if (dirty)
  		heap_inplace_update(relation, tuple);
*************** vac_update_datfrozenxid(void)
*** 875,886 ****
  	heap_close(relation, RowExclusiveLock);
=20=20
  	/*
! 	 * If we were able to advance datfrozenxid, see if we can truncate
! 	 * pg_clog. Also do it if the shared XID-wrap-limit info is stale, since
! 	 * this action will update that too.
  	 */
  	if (dirty || ForceTransactionIdLimitUpdate())
! 		vac_truncate_clog(newFrozenXid, newMinMulti);
  }
=20=20
=20=20
--- 923,935 ----
  	heap_close(relation, RowExclusiveLock);
=20=20
  	/*
! 	 * If we were able to advance datfrozenxid or datminmxid, see if we can
! 	 * truncate pg_clog and/or pg_multixact.  Also do it if the shared
! 	 * XID-wrap-limit info is stale, since this action will update that too.
  	 */
  	if (dirty || ForceTransactionIdLimitUpdate())
! 		vac_truncate_clog(newFrozenXid, newMinMulti,
! 						  lastSaneFrozenXid, lastSaneMinMulti);
  }
=20=20
=20=20
*************** vac_update_datfrozenxid(void)
*** 890,905 ****
   *		Scan pg_database to determine the system-wide oldest datfrozenxid,
   *		and use it to truncate the transaction commit log (pg_clog).
   *		Also update the XID wrap limit info maintained by varsup.c.
   *
!  *		The passed XID is simply the one I just wrote into my pg_database
!  *		entry.  It's used to initialize the "min" calculation.
   *
   *		This routine is only invoked when we've managed to change our
!  *		DB's datfrozenxid entry, or we found that the shared XID-wrap-limit
!  *		info is stale.
   */
  static void
! vac_truncate_clog(TransactionId frozenXID, MultiXactId minMulti)
  {
  	TransactionId myXID =3D GetCurrentTransactionId();
  	Relation	relation;
--- 939,960 ----
   *		Scan pg_database to determine the system-wide oldest datfrozenxid,
   *		and use it to truncate the transaction commit log (pg_clog).
   *		Also update the XID wrap limit info maintained by varsup.c.
+  *		Likewise for datminmxid.
   *
!  *		The passed frozenXID and minMulti are the updated values for my own
!  *		pg_database entry. They're used to initialize the "min" calculations.
!  *		The caller also passes the "last sane" XID and MXID, since it has
!  *		those at hand already.
   *
   *		This routine is only invoked when we've managed to change our
!  *		DB's datfrozenxid/datminmxid values, or we found that the shared
!  *		XID-wrap-limit info is stale.
   */
  static void
! vac_truncate_clog(TransactionId frozenXID,
! 				  MultiXactId minMulti,
! 				  TransactionId lastSaneFrozenXid,
! 				  MultiXactId lastSaneMinMulti)
  {
  	TransactionId myXID =3D GetCurrentTransactionId();
  	Relation	relation;
*************** vac_truncate_clog(TransactionId frozenXI
*** 907,920 ****
  	HeapTuple	tuple;
  	Oid			oldestxid_datoid;
  	Oid			minmulti_datoid;
  	bool		frozenAlreadyWrapped =3D false;
=20=20
! 	/* init oldest datoids to sync with my frozen values */
  	oldestxid_datoid =3D MyDatabaseId;
  	minmulti_datoid =3D MyDatabaseId;
=20=20
  	/*
! 	 * Scan pg_database to compute the minimum datfrozenxid
  	 *
  	 * Note: we need not worry about a race condition with new entries being
  	 * inserted by CREATE DATABASE.  Any such entry will have a copy of some
--- 962,976 ----
  	HeapTuple	tuple;
  	Oid			oldestxid_datoid;
  	Oid			minmulti_datoid;
+ 	bool		bogus =3D false;
  	bool		frozenAlreadyWrapped =3D false;
=20=20
! 	/* init oldest datoids to sync with my frozenXID/minMulti values */
  	oldestxid_datoid =3D MyDatabaseId;
  	minmulti_datoid =3D MyDatabaseId;
=20=20
  	/*
! 	 * Scan pg_database to compute the minimum datfrozenxid/datminmxid
  	 *
  	 * Note: we need not worry about a race condition with new entries being
  	 * inserted by CREATE DATABASE.  Any such entry will have a copy of some
*************** vac_truncate_clog(TransactionId frozenXI
*** 936,941 ****
--- 992,1010 ----
  		Assert(TransactionIdIsNormal(dbform->datfrozenxid));
  		Assert(MultiXactIdIsValid(dbform->datminmxid));
=20=20
+ 		/*
+ 		 * If things are working properly, no database should have a
+ 		 * datfrozenxid or datminmxid that is "in the future".  However, such
+ 		 * cases have been known to arise due to bugs in pg_upgrade.  If we
+ 		 * see any entries that are "in the future", chicken out and don't do
+ 		 * anything.  This ensures we won't truncate clog before those
+ 		 * databases have been scanned and cleaned up.  (We will issue the
+ 		 * "already wrapped" warning if appropriate, though.)
+ 		 */
+ 		if (TransactionIdPrecedes(lastSaneFrozenXid, dbform->datfrozenxid) ||
+ 			MultiXactIdPrecedes(lastSaneMinMulti, dbform->datminmxid))
+ 			bogus =3D true;
+=20
  		if (TransactionIdPrecedes(myXID, dbform->datfrozenxid))
  			frozenAlreadyWrapped =3D true;
  		else if (TransactionIdPrecedes(dbform->datfrozenxid, frozenXID))
*************** vac_truncate_clog(TransactionId frozenXI
*** 969,974 ****
--- 1038,1047 ----
  		return;
  	}
=20=20
+ 	/* chicken out if data is bogus in any other way */
+ 	if (bogus)
+ 		return;
+=20
  	/*
  	 * Truncate CLOG to the oldest computed value.  Note we don't truncate
  	 * multixacts; that will be done by the next checkpoint.

------- =_aaaaaaaaaa0
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

------- =_aaaaaaaaaa0--