Re: [HACKERS] VACUUM FULL/CLUSTER doesn't update pg_class's pg_class.relfrozenxid

2014-03-04 Thread Robert Haas
On Mon, Mar 3, 2014 at 7:52 AM, Robert Haas robertmh...@gmail.com wrote:
 But all that having been said, a deadline is a deadline, so if anyone
 wishes to declare this untimely please speak up.

Hearing only crickets, committed.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


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


Re: [HACKERS] VACUUM FULL/CLUSTER doesn't update pg_class's pg_class.relfrozenxid

2014-03-03 Thread Robert Haas
On Thu, Feb 27, 2014 at 1:06 PM, Andres Freund and...@2ndquadrant.com wrote:
 As Robert previously complained a database wide VACUUM FULL now (as of
 3cff1879f8d03) reliably increases the relfrozenxid for all tables but
 pg_class itself. That's a bit sad because it means doing a VACUUM FULL
 won't help in a anti-wraparound scenario.

 The reason for that is explained by the following comment:
 /*
  * Update the tuples in pg_class --- unless the target relation of the
  * swap is pg_class itself.  In that case, there is zero point in 
 making
  * changes because we'd be updating the old data that we're about to 
 throw
  * away.  Because the real work being done here for a mapped relation 
 is
  * just to change the relation map settings, it's all right to not 
 update
  * the pg_class rows in this case.
  */

 I think the easiest fix for that is to update pg_class' relfrozenxid in
 finish_heap_swap() after the indexes have been rebuilt, that's just a
 couple of lines. There's more complex solutions that'd avoid the need
 for that special case, but I it's sufficient. A patch doing that is
 attached.

So, this patch is obviously after the final CommitFest deadline, but
I'd like to commit it to 9.4 anyway on admittedly-arguable theory that
it's tying up a loose end introduced by
3cff1879f8d03cb729368722ca823a4bf74c0cac.  Prior to that commit,
VACUUM FULL and CLUSTER *never* updated relfrozenxid; beginning with
that commit, they do so for all relations except pg_class.  This
tidies up that omission.

And I think that's a pretty worthwhile thing to do, because we get
periodic reports from people who have run VACUUM FULL on a database in
danger of wraparound and then wondered why it did not fix the problem.
 The previously-mentioned commit did most of the heavy lifting as far
as tidying that up, but without this adjustment it won't quite get us
over the hump.

But all that having been said, a deadline is a deadline, so if anyone
wishes to declare this untimely please speak up.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


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


[HACKERS] VACUUM FULL/CLUSTER doesn't update pg_class's pg_class.relfrozenxid

2014-02-27 Thread Andres Freund
Hi,

As Robert previously complained a database wide VACUUM FULL now (as of
3cff1879f8d03) reliably increases the relfrozenxid for all tables but
pg_class itself. That's a bit sad because it means doing a VACUUM FULL
won't help in a anti-wraparound scenario.

The reason for that is explained by the following comment:
/*
 * Update the tuples in pg_class --- unless the target relation of the
 * swap is pg_class itself.  In that case, there is zero point in making
 * changes because we'd be updating the old data that we're about to 
throw
 * away.  Because the real work being done here for a mapped relation is
 * just to change the relation map settings, it's all right to not 
update
 * the pg_class rows in this case.
 */

I think the easiest fix for that is to update pg_class' relfrozenxid in
finish_heap_swap() after the indexes have been rebuilt, that's just a
couple of lines. There's more complex solutions that'd avoid the need
for that special case, but I it's sufficient. A patch doing that is
attached.

Note that VACUUM FULL will still require more xids than a plain VACUUM,
but it scales linearly with the number of relations, so I have a hard
time seing that as problematic.

Greetings,

Andres Freund

-- 
 Andres Freund http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training  Services
From cc8943822e18f283af01c1f14489f7bd9a2abede Mon Sep 17 00:00:00 2001
From: Andres Freund and...@anarazel.de
Date: Thu, 27 Feb 2014 19:00:11 +0100
Subject: [PATCH] Increase relfrozenxid even when doing a VACUUM FULL on
 pg_class.

Previously VACUUM FULL (and CLUSTER) didn't update pg_class's own
relfrozenxid because the place doing so only has convenient (as in
indexed) access to the old heap, not to the new rebuilt heap. Fix that
by adding a special case updating pg_class's relfrozenxid after the
indexes have been rebuilt.

That's useful because now a database VACUUM FULL reliably increases
the database's datfrozenxid (and datminmxid, although that's often
less critical).
---
 src/backend/commands/cluster.c | 37 -
 1 file changed, 36 insertions(+), 1 deletion(-)

diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c
index 8b18e4a..c478ba5 100644
--- a/src/backend/commands/cluster.c
+++ b/src/backend/commands/cluster.c
@@ -1269,7 +1269,8 @@ swap_relation_files(Oid r1, Oid r2, bool target_is_pg_class,
 	 * changes because we'd be updating the old data that we're about to throw
 	 * away.  Because the real work being done here for a mapped relation is
 	 * just to change the relation map settings, it's all right to not update
-	 * the pg_class rows in this case.
+	 * the pg_class rows in this case. The most important changes will instead
+	 * performed later, in finish_heap_swap() itself.
 	 */
 	if (!target_is_pg_class)
 	{
@@ -1504,6 +1505,40 @@ finish_heap_swap(Oid OIDOldHeap, Oid OIDNewHeap,
 		reindex_flags |= REINDEX_REL_CHECK_CONSTRAINTS;
 	reindex_relation(OIDOldHeap, reindex_flags);
 
+	/*
+	 * If the relation being rebuild is pg_class, swap_relation_files()
+	 * couldn't update pg_class's own pg_class entry (check comments in
+	 * swap_relation_files()), thus relfrozenxid was not updated. That's
+	 * annoying because a potential reason for doing a VACUUM FULL is a
+	 * imminent or actual anti-wraparound shutdown.  So, now that we can
+	 * access the new relation using it's indices, update
+	 * relfrozenxid. pg_class doesn't have a toast relation, so we don't need
+	 * to update the corresponding toast relation. Not that there's little
+	 * point moving all relfrozenxid updates here since swap_relation_files()
+	 * needs to write to pg_class for non-mapped relations anyway.
+	 */
+	if (OIDOldHeap == RelationRelationId)
+	{
+		Relation	relRelation;
+		HeapTuple	reltup;
+		Form_pg_class relform;
+
+		relRelation = heap_open(RelationRelationId, RowExclusiveLock);
+
+		reltup = SearchSysCacheCopy1(RELOID, ObjectIdGetDatum(OIDOldHeap));
+		if (!HeapTupleIsValid(reltup))
+			elog(ERROR, cache lookup failed for relation %u, OIDOldHeap);
+		relform = (Form_pg_class) GETSTRUCT(reltup);
+
+		relform-relfrozenxid = frozenXid;
+		relform-relminmxid = cutoffMulti;
+
+		simple_heap_update(relRelation, reltup-t_self, reltup);
+		CatalogUpdateIndexes(relRelation, reltup);
+
+		heap_close(relRelation, RowExclusiveLock);
+	}
+
 	/* Destroy new heap with old filenode */
 	object.classId = RelationRelationId;
 	object.objectId = OIDNewHeap;
-- 
1.8.3.251.g1462b67


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