Subject: Autovacuum and XID wraparound

From: David Fetter <[email-masked]>
Date: Sun, 13 May 2007 18:17:40 -0700

Folks,

Per Neil Conway, here's some doc patches re: the autovacuum daemon's
behavior.  Should this be back-patched to 8.2x?

Cheers,
D
-- 
David Fetter <[email-masked]> http://fetter.org/
phone: +1 415 235 3778        AIM: dfetter666
                              Skype: davidfetter

Remember to vote!
Consider donating to PostgreSQL: http://www.postgresql.org/about/donate
Index: doc/src/sgml/config.sgml
===================================================================
RCS file: /projects/cvsroot/pgsql/doc/src/sgml/config.sgml,v
retrieving revision 1.122
diff -c -r1.122 config.sgml
*** doc/src/sgml/config.sgml	20 Apr 2007 02:37:37 -0000	1.122
--- doc/src/sgml/config.sgml	14 May 2007 01:16:02 -0000
***************
*** 3172,3177 ****
--- 3172,3185 ----
          This parameter can only be set in the <filename>postgresql.conf</>
          file or on the server command line.
         </para>
+        <note>
+         <para>
+           Even when this variable is set to off, the autovacuum daemon
+           will run periodically in order to prevent transaction_id
+           wraparound.  See <xref linkend="vacuum-for-wraparound"> for
+           more information.
+         </para>
+        </note>
        </listitem>
       </varlistentry>
  
Index: src/backend/utils/misc/postgresql.conf.sample
===================================================================
RCS file: /projects/cvsroot/pgsql/src/backend/utils/misc/postgresql.conf.sample,v
retrieving revision 1.215
diff -c -r1.215 postgresql.conf.sample
*** src/backend/utils/misc/postgresql.conf.sample	18 Apr 2007 16:44:18 -0000	1.215
--- src/backend/utils/misc/postgresql.conf.sample	14 May 2007 01:16:02 -0000
***************
*** 372,377 ****
--- 372,379 ----
  #---------------------------------------------------------------------------
  # AUTOVACUUM PARAMETERS
  #---------------------------------------------------------------------------
+ # Note: even when autovacuum is turned off, the autovacuum daemon will
+ # run in order to prevent transaction id wraparound.
  
  #autovacuum = on			# enable autovacuum subprocess?
  					# 'on' requires stats_start_collector

---

From: Tom Lane <[email-masked]>
Date: Sun, 13 May 2007 22:06:40 -0400

> behavior.  Should this be back-patched to 8.2x?

This fact is already documented in at least three places; do we really
need two more?  The proposed addition to postgresql.conf seems
particularly over-the-top, since there is no entry in that file that
even pretends to offer a complete description of the associated
behavior.

			regards, tom lane

---

From: David Fetter <[email-masked]>
Date: Sun, 13 May 2007 19:15:03 -0700

On Sun, May 13, 2007 at 10:06:40PM -0400, Tom Lane wrote:
> > Per Neil Conway, here's some doc patches re: the autovacuum
> > daemon's behavior.  Should this be back-patched to 8.2x?
> 
> This fact is already documented in at least three places; do we
> really need two more?

Yes.

> The proposed addition to postgresql.conf seems particularly
> over-the-top, since there is no entry in that file that even
> pretends to offer a complete description of the associated behavior.

I think that a boolean that doesn't do what you expect booleans to do,
i.e. turn the thing all the way off, is worth a mention.

Cheers,
David.

Remember to vote!

---

From: Neil Conway <[email-masked]>
Date: Mon, 14 May 2007 00:34:29 -0400

On Sun, 2007-13-05 at 22:06 -0400, Tom Lane wrote:
> need two more?

I think we need to at least modify the documentation for the autovacuum
GUC parameter, which currently states only that it "controls whether the
server should run the autovacuum launcher daemon" -- this is not
strictly true, and in any case, it isn't the whole story.

> over-the-top

I agree that this information doesn't really belong in postgresql.conf.

-Neil

---

From: Bruce Momjian <[email-masked]>
Date: Mon, 14 May 2007 16:22:23 -0400 (EDT)

> i.e. turn the thing all the way off, is worth a mention.

I agree with Tom.  I don't think the current behavior is a major issue
for users for it to be mentioned more than it already is, though if you
want to move one of those, we can do that.

-- 
  Bruce Momjian  <[email-masked]>          http://momjian.us
  EnterpriseDB                               http://www.enterprisedb.com

  + If your life is a hard drive, Christ can be your backup. +

---

From: Chris Browne <[email-masked]>
Date: Mon, 14 May 2007 16:25:08 -0400

> I agree that this information doesn't really belong in postgresql.conf.

Question... (note: this does not strictly fit into the purview of the
.patches list)

Would the following 'maintenance' regimen be truly safe against XID
wraparound:

 - Most tables are being vacuumed regularly, so that
   pg_class.relfrozenxid is kept "safe."

 - There are some tables that periodically get TRUNCATEd so that, in
   principle, they never need to be vacuumed.

Is it actually true that we'd never need to vacuum those tables
(assuming 8.2+)?  I suppose it would be rather cheap to VACUUM
immediately after the TRUNCATE...

The application is one where we might use partitioning, rolling from
table to table every so often, with the expectation that we'll
TRUNCATE the eldest data often enough that we shouldn't need to VACUUM
any of the partitions.
-- 
let name="cbbrowne" and tld="linuxdatabases.info" in name ^ "@" ^ tld;;
http://linuxdatabases.info/info/linuxxian.html
Why are men like blenders?
You need one, but you're not quite sure why.

---

From: Alvaro Herrera <[email-masked]>
Date: Mon, 14 May 2007 18:16:19 -0400

> immediately after the TRUNCATE...

You'd need to vacuum after the truncate.  It would be pretty cheap, the
tables being empty.

I suppose it would be pretty trivial to set the relfrozenxid to
RecentXmin or something during TRUNCATE.

-- 
Alvaro Herrera                                http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

---

From: Tom Lane <[email-masked]>
Date: Mon, 14 May 2007 19:10:22 -0400

> RecentXmin or something during TRUNCATE.

I had the idea we were doing that already --- at least I'm pretty sure I
remember it being discussed.  But I see it's not being done in HEAD.

			regards, tom lane

---

From: Neil Conway <[email-masked]>
Date: Mon, 14 May 2007 22:49:03 -0400

On Mon, 2007-14-05 at 16:22 -0400, Bruce Momjian wrote:
> for users for it to be mentioned more than it already is

Are you really suggesting that we shouldn't modify config.sgml to note
that "autovacuum = off" does not actually imply that "the autovacuum
daemon is disabled"? ISTM that plainly violates the principle of least
surprise -- it is almost the definition of what an entry in config.sgml
*should* include.

> though if you want to move one of those, we can do that.

So the change would be okay if we also removed one of the other mentions
in an unrelated section of the manual? I don't see the logic.

-Neil

---

From: Alvaro Herrera <[email-masked]>
Date: Tue, 15 May 2007 09:07:18 -0400

> surprise -- it is almost the definition of what an entry in config.sgml

I agree, the note should be added there (but it should be a short one
and refer the reader someplace else for more complete details).

Was there a doc patch proposed already?  I seem to have missed it.

---

From: Neil Conway <[email-masked]>
Date: Tue, 15 May 2007 11:54:09 -0400

> and refer the reader someplace else for more complete details).

I've applied the attached patch to HEAD and REL8_2_STABLE.

-Neil

Index: doc/src/sgml/config.sgml
===================================================================
RCS file: /home/neilc/postgres/cvs_root/pgsql/doc/src/sgml/config.sgml,v
retrieving revision 1.122
diff -c -p -r1.122 config.sgml
*** doc/src/sgml/config.sgml	20 Apr 2007 02:37:37 -0000	1.122
--- doc/src/sgml/config.sgml	15 May 2007 15:04:35 -0000
*************** SELECT * FROM parent WHERE key = 2400;
*** 3172,3177 ****
--- 3172,3183 ----
          This parameter can only be set in the <filename>postgresql.conf</>
          file or on the server command line.
         </para>
+        <para>
+         Note that even when this parameter is disabled, the system
+         will periodically launch autovacuum processes in order to
+         prevent transaction ID wraparound.  See <xref
+         linkend="vacuum-for-wraparound"> for more information.
+        </para>
        </listitem>
       </varlistentry>

---

From: Alvaro Herrera <[email-masked]>
Date: Tue, 15 May 2007 18:13:47 -0400

> remember it being discussed.  But I see it's not being done in HEAD.

Patch to do it attached.  I am thinking we can do something similar in
CLUSTER as well.

-- 
Alvaro Herrera                                http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.
Index: src/backend/catalog/index.c
===================================================================
RCS file: /home/alvherre/Code/cvs/pgsql/src/backend/catalog/index.c,v
retrieving revision 1.282
diff -c -p -r1.282 index.c
*** src/backend/catalog/index.c	29 Mar 2007 00:15:37 -0000	1.282
--- src/backend/catalog/index.c	15 May 2007 22:06:12 -0000
*************** index_update_stats(Relation rel, bool ha
*** 1188,1196 ****
   * setNewRelfilenode		- assign a new relfilenode value to the relation
   *
   * Caller must already hold exclusive lock on the relation.
   */
  void
! setNewRelfilenode(Relation relation)
  {
  	Oid			newrelfilenode;
  	RelFileNode newrnode;
--- 1188,1199 ----
   * setNewRelfilenode		- assign a new relfilenode value to the relation
   *
   * Caller must already hold exclusive lock on the relation.
+  *
+  * The relation is marked with relfrozenxid=freezeXid (InvalidTransactionId
+  * must be passed for indexes)
   */
  void
! setNewRelfilenode(Relation relation, TransactionId freezeXid)
  {
  	Oid			newrelfilenode;
  	RelFileNode newrnode;
*************** setNewRelfilenode(Relation relation)
*** 1204,1209 ****
--- 1207,1216 ----
  		   relation->rd_rel->relkind == RELKIND_INDEX);
  	/* Can't change for shared tables or indexes */
  	Assert(!relation->rd_rel->relisshared);
+ 	/* Indexes must have Invalid frozenxid; other relations must not */
+ 	Assert((relation->rd_rel->relkind == RELKIND_INDEX &&
+ 			freezeXid == InvalidTransactionId) ||
+ 		   TransactionIdIsNormal(freezeXid));
  
  	/* Allocate a new relfilenode */
  	newrelfilenode = GetNewRelFileNode(relation->rd_rel->reltablespace,
*************** setNewRelfilenode(Relation relation)
*** 1241,1246 ****
--- 1248,1254 ----
  	rd_rel->relfilenode = newrelfilenode;
  	rd_rel->relpages = 0;		/* it's empty until further notice */
  	rd_rel->reltuples = 0;
+ 	rd_rel->relfrozenxid = freezeXid;
  	simple_heap_update(pg_class, &tuple->t_self, tuple);
  	CatalogUpdateIndexes(pg_class, tuple);
  
*************** reindex_index(Oid indexId)
*** 1957,1963 ****
  			/*
  			 * We'll build a new physical relation for the index.
  			 */
! 			setNewRelfilenode(iRel);
  		}
  
  		/* Initialize the index and rebuild */
--- 1965,1971 ----
  			/*
  			 * We'll build a new physical relation for the index.
  			 */
! 			setNewRelfilenode(iRel, InvalidTransactionId);
  		}
  
  		/* Initialize the index and rebuild */
Index: src/backend/commands/tablecmds.c
===================================================================
RCS file: /home/alvherre/Code/cvs/pgsql/src/backend/commands/tablecmds.c,v
retrieving revision 1.223
diff -c -p -r1.223 tablecmds.c
*** src/backend/commands/tablecmds.c	14 May 2007 20:24:41 -0000	1.223
--- src/backend/commands/tablecmds.c	15 May 2007 22:08:33 -0000
*************** ExecuteTruncate(TruncateStmt *stmt)
*** 616,622 ****
  		 * the relfilenode value.	The old storage file is scheduled for
  		 * deletion at commit.
  		 */
! 		setNewRelfilenode(rel);
  
  		heap_relid = RelationGetRelid(rel);
  		toast_relid = rel->rd_rel->reltoastrelid;
--- 616,622 ----
  		 * the relfilenode value.	The old storage file is scheduled for
  		 * deletion at commit.
  		 */
! 		setNewRelfilenode(rel, RecentXmin);
  
  		heap_relid = RelationGetRelid(rel);
  		toast_relid = rel->rd_rel->reltoastrelid;
*************** ExecuteTruncate(TruncateStmt *stmt)
*** 629,635 ****
  		if (OidIsValid(toast_relid))
  		{
  			rel = relation_open(toast_relid, AccessExclusiveLock);
! 			setNewRelfilenode(rel);
  			heap_close(rel, NoLock);
  		}
  
--- 629,635 ----
  		if (OidIsValid(toast_relid))
  		{
  			rel = relation_open(toast_relid, AccessExclusiveLock);
! 			setNewRelfilenode(rel, RecentXmin);
  			heap_close(rel, NoLock);
  		}
  
Index: src/include/catalog/index.h
===================================================================
RCS file: /home/alvherre/Code/cvs/pgsql/src/include/catalog/index.h,v
retrieving revision 1.73
diff -c -p -r1.73 index.h
*** src/include/catalog/index.h	9 Jan 2007 02:14:15 -0000	1.73
--- src/include/catalog/index.h	15 May 2007 21:59:31 -0000
*************** extern void FormIndexDatum(IndexInfo *in
*** 53,59 ****
  			   Datum *values,
  			   bool *isnull);
  
! extern void setNewRelfilenode(Relation relation);
  
  extern void index_build(Relation heapRelation,
  			Relation indexRelation,
--- 53,59 ----
  			   Datum *values,
  			   bool *isnull);
  
! extern void setNewRelfilenode(Relation relation, TransactionId freezeXid);
  
  extern void index_build(Relation heapRelation,
  			Relation indexRelation,

---

From: "Jim C. Nasby" <[email-masked]>
Date: Tue, 15 May 2007 19:25:00 -0500

> Patch to do it attached.  I am thinking we can do something similar in

Actually, it already happens for CLUSTER because cluster calls
heap_create_with_catalog, which calls AddNewRelationTuple. See
backend/catalog/heap.c line 716.
-- 
Jim Nasby                                      [email-masked]
EnterpriseDB      http://enterprisedb.com      512.569.9461 (cell)

---

From: Alvaro Herrera <[email-masked]>
Date: Tue, 15 May 2007 22:02:18 -0400

> backend/catalog/heap.c line 716.

Right, but that heap is dropped later, and only the relfilenode remains,
because they are swapped.

In any case the change is a very small patch, which I attach but I
haven't tested.  This only works if the new rewriteheap stuff actually
changes Xids to follow OldestXmin, i.e. all tuples that have older
Xmin/Xmax are frozen (or marked with the current Xid).  Heikki, can you
confirm that this is the case?

-- 
Alvaro Herrera                                http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.
Index: src/backend/commands/cluster.c
===================================================================
RCS file: /home/alvherre/Code/cvs/pgsql/src/backend/commands/cluster.c,v
retrieving revision 1.159
diff -c -p -r1.159 cluster.c
*** src/backend/commands/cluster.c	8 Apr 2007 01:26:28 -0000	1.159
--- src/backend/commands/cluster.c	15 May 2007 22:55:22 -0000
*************** typedef struct
*** 54,60 ****
  
  static void cluster_rel(RelToCluster *rv, bool recheck);
  static void rebuild_relation(Relation OldHeap, Oid indexOid);
! static void copy_heap_data(Oid OIDNewHeap, Oid OIDOldHeap, Oid OIDOldIndex);
  static List *get_tables_to_cluster(MemoryContext cluster_context);

--- 54,60 ----
  
  static void cluster_rel(RelToCluster *rv, bool recheck);
  static void rebuild_relation(Relation OldHeap, Oid indexOid);
! static TransactionId copy_heap_data(Oid OIDNewHeap, Oid OIDOldHeap, Oid OIDOldIndex);
  static List *get_tables_to_cluster(MemoryContext cluster_context);

*************** rebuild_relation(Relation OldHeap, Oid i
*** 512,517 ****
--- 512,518 ----
  	Oid			tableSpace = OldHeap->rd_rel->reltablespace;
  	Oid			OIDNewHeap;
  	char		NewHeapName[NAMEDATALEN];
+ 	TransactionId frozenXid;
  	ObjectAddress object;
  
  	/* Mark the correct index as clustered */
*************** rebuild_relation(Relation OldHeap, Oid i
*** 538,548 ****
  	/*
  	 * Copy the heap data into the new table in the desired order.
  	 */
! 	copy_heap_data(OIDNewHeap, tableOid, indexOid);
  
  	/* To make the new heap's data visible (probably not needed?). */
  	CommandCounterIncrement();
  
  	/* Swap the physical files of the old and new heaps. */
  	swap_relation_files(tableOid, OIDNewHeap);
  
--- 539,556 ----
  	/*
  	 * Copy the heap data into the new table in the desired order.
  	 */
! 	frozenXid = copy_heap_data(OIDNewHeap, tableOid, indexOid);
  
  	/* To make the new heap's data visible (probably not needed?). */
  	CommandCounterIncrement();
  
+ 	/*
+ 	 * update the relation's freeze Xid.  We don't need to change the 
+ 	 * actual tuple on disk, because swap_relation_files will do it for
+ 	 * us.
+ 	 */
+ 	OldHeap->rd_rel->relfrozenxid = frozenXid;
+ 
  	/* Swap the physical files of the old and new heaps. */
  	swap_relation_files(tableOid, OIDNewHeap);
  
*************** make_new_heap(Oid OIDOldHeap, const char
*** 640,648 ****
  }
  
  /*
!  * Do the physical copying of heap data.
   */
! static void
  copy_heap_data(Oid OIDNewHeap, Oid OIDOldHeap, Oid OIDOldIndex)
  {
  	Relation	NewHeap,
--- 648,657 ----
  }
  
  /*
!  * Do the physical copying of heap data.  Returns the transaction ID used as
!  * cutoff point.
   */
! static TransactionId
  copy_heap_data(Oid OIDNewHeap, Oid OIDOldHeap, Oid OIDOldIndex)
  {
  	Relation	NewHeap,
*************** copy_heap_data(Oid OIDNewHeap, Oid OIDOl
*** 809,814 ****
--- 818,825 ----
  	index_close(OldIndex, NoLock);
  	heap_close(OldHeap, NoLock);
  	heap_close(NewHeap, NoLock);
+ 
+ 	return OldestXmin;
  }
  
  /*

---

From: Tom Lane <[email-masked]>
Date: Wed, 16 May 2007 01:20:56 -0400

> Patch to do it attached.  I am thinking we can do something similar in

Umm ... you'd have to be a lot more conservative in CLUSTER now that
it's MVCC-safe.  I don't say that CLUSTER can't push up relfrozenxid,
but there's something wrong if CLUSTER and TRUNCATE are trying to
push it up the same amount.

			regards, tom lane

---

From: Heikki Linnakangas <[email-masked]>
Date: Wed, 16 May 2007 09:22:02 +0100

> confirm that this is the case?

No, CLUSTER doesn't freeze tuples. It could do that easily, it didn't 
occur to me when I wrote it. It would make it harder to debug, though, 
should we have any problems with it in the future.

-- 
   Heikki Linnakangas

---

From: Alvaro Herrera <[email-masked]>
Date: Wed, 16 May 2007 08:44:20 -0400

> push it up the same amount.

No, TRUNCATE will use RecentXmin while the CLUSTER patch I posted uses
OldestXmin, which is what the HeapTupleSatisfiesUpdate test was using.
However, given that Heikki just confirmed that CLUSTER does not freeze
tuples, it's not really possible to do this, so I'll drop the CLUSTER
patch for now.

This means that people using CLUSTER to compact tables won't have the
benefit of advancing relfrozenxid, so they will have to run VACUUM on
those tables at some point anyway, even though there will be no dead
tuples :-(

---

From: Heikki Linnakangas <[email-masked]>
Date: Wed, 16 May 2007 14:59:54 +0100

> those tables at some point anyway, even though there will be no dead

Now that I think this a bit more, I think CLUSTER should freeze the 
tuples. I was worried about losing valuable debug information by doing 
that, but thinking a bit more that's not a big concern: we wouldn't 
freeze tuples newer than recent xmin. The update chain logic is the most 
risky part of the code, and we wouldn't lose the xmin and xmax of tuples 
that are part of update chains.

Patch attached. You'll need the changes to cluster.c to set the 
relfrozenid as well.

-- 
   Heikki Linnakangas
   EnterpriseDB   http://www.enterprisedb.com
Index: src/backend/access/heap/rewriteheap.c
===================================================================
RCS file: /home/hlinnaka/pgcvsrepository/pgsql/src/backend/access/heap/rewriteheap.c,v
retrieving revision 1.3
diff -c -r1.3 rewriteheap.c
*** src/backend/access/heap/rewriteheap.c	17 Apr 2007 21:29:31 -0000	1.3
--- src/backend/access/heap/rewriteheap.c	16 May 2007 13:42:23 -0000
***************
*** 538,543 ****
--- 538,545 ----
  	OffsetNumber	newoff;
  	HeapTuple		heaptup;
  
+ 	heap_freeze_tuple(tup->t_data, state->rs_oldest_xmin, NULL);
+ 
  	/*
  	 * If the new tuple is too big for storage or contains already toasted
  	 * out-of-line attributes from some other relation, invoke the toaster.

---

From: Alvaro Herrera <[email-masked]>
Date: Wed, 16 May 2007 12:38:04 -0400

> Patch attached. You'll need the changes to cluster.c to set the 

Thanks, committed.  (I changed NULL for InvalidBuffer).

-- 
Alvaro Herrera                                http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

---

From: Alvaro Herrera <[email-masked]>
Date: Wed, 16 May 2007 13:28:50 -0400

> Patch to do it attached.  I am thinking we can do something similar in

Applied.

---

From: Tom Lane <[email-masked]>
Date: Wed, 16 May 2007 15:44:06 -0400

Heikki Linnakangas <[email-masked]> writes:
> tuples.

I disagree with that...

			regards, tom lane

---

From: Heikki Linnakangas <[email-masked]>
Date: Wed, 16 May 2007 20:51:03 +0100

>> Now that I think this a bit more, I think CLUSTER should freeze the 

Ok. Why?

---

From: Tom Lane <[email-masked]>
Date: Wed, 16 May 2007 16:30:18 -0400

> Ok. Why?

It's removing potentially-important data without any notice or recourse
to the user.  There seems to be a contingent around here that thinks
that as soon as xmin is older than GlobalXmin it is no longer of
interest to anyone, but I have lost count of how often I have found it
invaluable for forensic purposes.  I have resisted having VACUUM freeze
tuples before they've reached a quite-respectable age, and I object to
having CLUSTER do it either.

I could maybe accept a CLUSTER FREEZE option to do this, but that's not
what's in the patch.

			regards, tom lane

---

From: Heikki Linnakangas <[email-masked]>
Date: Wed, 16 May 2007 21:35:20 +0100

> having CLUSTER do it either.

How about freezing anything older than vacuum_freeze_min_age, just like 
VACUUM does?

> I could maybe accept a CLUSTER FREEZE option to do this, but that's not

I wouldn't like to add more options to CLUSTER, people are already 
confused about the similar VACUUM options.

---

From: Tom Lane <[email-masked]>
Date: Wed, 16 May 2007 17:01:12 -0400

Heikki Linnakangas <[email-masked]> writes:
> Tom Lane wrote:
>> ... I have resisted having VACUUM freeze

> How about freezing anything older than vacuum_freeze_min_age, just like 

I suppose that'd be OK, but is it likely to be worth the trouble?

> confused about the similar VACUUM options.

Agreed, I wasn't thrilled with that idea.

			regards, tom lane

---

From: Alvaro Herrera <[email-masked]>
Date: Wed, 16 May 2007 17:05:32 -0400

> I suppose that'd be OK, but is it likely to be worth the trouble?

I think so, because it means that people using CLUSTER to keep the size
of tables in line instead of VACUUM, would not need the otherwise
mandatory VACUUM.

---

From: Tom Lane <[email-masked]>
Date: Wed, 16 May 2007 17:20:56 -0400

> of tables in line instead of VACUUM, would not need the otherwise

Fair enough.  Who will fix the already-applied patch?

			regards, tom lane

---

From: Alvaro Herrera <[email-masked]>
Date: Wed, 16 May 2007 17:41:30 -0400

> Fair enough.  Who will fix the already-applied patch?

I'm on it.

---

From: Alvaro Herrera <[email-masked]>
Date: Wed, 16 May 2007 18:46:27 -0400

> Fair enough.  Who will fix the already-applied patch?

Here is my proposed patch.

-- 
Alvaro Herrera                                http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Index: src/backend/access/heap/rewriteheap.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/access/heap/rewriteheap.c,v
retrieving revision 1.4
diff -c -p -r1.4 rewriteheap.c
*** src/backend/access/heap/rewriteheap.c	16 May 2007 16:36:56 -0000	1.4
--- src/backend/access/heap/rewriteheap.c	16 May 2007 22:34:18 -0000
*************** typedef struct RewriteStateData
*** 123,128 ****
--- 123,130 ----
  	bool			rs_use_wal;			/* must we WAL-log inserts? */
  	TransactionId	rs_oldest_xmin;		/* oldest xmin used by caller to
  										 * determine tuple visibility */
+ 	TransactionId	rs_freeze_xid;		/* Xid that will be used as freeze
+ 										 * cutoff point */
  	MemoryContext	rs_cxt;				/* for hash tables and entries and
  										 * tuples in them */
  	HTAB		   *rs_unresolved_tups;	/* unmatched A tuples */
*************** static void raw_heap_insert(RewriteState
*** 171,176 ****
--- 173,179 ----
   *
   * new_heap		new, locked heap relation to insert tuples to
   * oldest_xmin	xid used by the caller to determine which tuples are dead
+  * freeze_xid	xid before which tuples will be frozen
   * use_wal		should the inserts to the new heap be WAL-logged?
   *
   * Returns an opaque RewriteState, allocated in current memory context,
*************** static void raw_heap_insert(RewriteState
*** 178,184 ****
   */
  RewriteState
  begin_heap_rewrite(Relation new_heap, TransactionId oldest_xmin,
! 				   bool use_wal)
  {
  	RewriteState state;
  	MemoryContext rw_cxt;
--- 181,187 ----
   */
  RewriteState
  begin_heap_rewrite(Relation new_heap, TransactionId oldest_xmin,
! 				   TransactionId freeze_xid, bool use_wal)
  {
  	RewriteState state;
  	MemoryContext rw_cxt;
*************** begin_heap_rewrite(Relation new_heap, Tr
*** 206,211 ****
--- 209,215 ----
  	state->rs_buffer_valid = false;
  	state->rs_use_wal = use_wal;
  	state->rs_oldest_xmin = oldest_xmin;
+ 	state->rs_freeze_xid = freeze_xid;
  	state->rs_cxt = rw_cxt;
  
  	/* Initialize hash tables used to track update chains */
*************** raw_heap_insert(RewriteState state, Heap
*** 538,544 ****
  	OffsetNumber	newoff;
  	HeapTuple		heaptup;
  
! 	heap_freeze_tuple(tup->t_data, state->rs_oldest_xmin, InvalidBuffer);
  
  	/*
--- 542,548 ----
  	OffsetNumber	newoff;
  	HeapTuple		heaptup;
  
! 	heap_freeze_tuple(tup->t_data, state->rs_freeze_xid, InvalidBuffer);
  
  	/*
  	 * If the new tuple is too big for storage or contains already toasted
Index: src/backend/commands/cluster.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/commands/cluster.c,v
retrieving revision 1.159
diff -c -p -r1.159 cluster.c
*** src/backend/commands/cluster.c	8 Apr 2007 01:26:28 -0000	1.159
--- src/backend/commands/cluster.c	16 May 2007 22:34:24 -0000
***************
*** 29,34 ****
--- 29,35 ----
  #include "catalog/namespace.h"
  #include "catalog/toasting.h"
  #include "commands/cluster.h"
+ #include "commands/vacuum.h"
  #include "miscadmin.h"
  #include "storage/procarray.h"
  #include "utils/acl.h"
*************** copy_heap_data(Oid OIDNewHeap, Oid OIDOl
*** 657,662 ****
--- 658,664 ----
  	HeapTuple	tuple;
  	bool		use_wal;
  	TransactionId OldestXmin;
+ 	TransactionId FreezeXid;
  	RewriteState rwstate;
  
  	/*
*************** copy_heap_data(Oid OIDNewHeap, Oid OIDOl
*** 688,698 ****
  	/* use_wal off requires rd_targblock be initially invalid */
  	Assert(NewHeap->rd_targblock == InvalidBlockNumber);
  
! 	/* Get the cutoff xmin we'll use to weed out dead tuples */
! 	OldestXmin = GetOldestXmin(OldHeap->rd_rel->relisshared, true);
  
  	/* Initialize the rewrite operation */
! 	rwstate = begin_heap_rewrite(NewHeap, OldestXmin, use_wal);
  
  	/*
  	 * Scan through the OldHeap in OldIndex order and copy each tuple into the
--- 690,705 ----
  	/* use_wal off requires rd_targblock be initially invalid */
  	Assert(NewHeap->rd_targblock == InvalidBlockNumber);
  
! 	/*
! 	 * compute xids used to freeze and weed out dead tuples.  We use -1
! 	 * freeze_min_age to avoid having CLUSTER freeze tuples earlier than
! 	 * a plain VACUUM would.
! 	 */
! 	vacuum_set_xid_limits(-1, OldHeap->rd_rel->relisshared,
! 						  &OldestXmin, &FreezeXid);
  
  	/* Initialize the rewrite operation */
! 	rwstate = begin_heap_rewrite(NewHeap, OldestXmin, FreezeXid, use_wal);
  
  	/*
  	 * Scan through the OldHeap in OldIndex order and copy each tuple into the
Index: src/backend/commands/vacuum.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/commands/vacuum.c,v
retrieving revision 1.350
diff -c -p -r1.350 vacuum.c
*** src/backend/commands/vacuum.c	16 Apr 2007 18:29:50 -0000	1.350
--- src/backend/commands/vacuum.c	16 May 2007 22:34:47 -0000
*************** get_rel_oids(List *relids, const RangeVa
*** 566,572 ****
   * vacuum_set_xid_limits() -- compute oldest-Xmin and freeze cutoff points
   */
  void
! vacuum_set_xid_limits(VacuumStmt *vacstmt, bool sharedRel,
  					  TransactionId *oldestXmin,
  					  TransactionId *freezeLimit)
  {
--- 566,572 ----
   * vacuum_set_xid_limits() -- compute oldest-Xmin and freeze cutoff points
   */
  void
! vacuum_set_xid_limits(int freeze_min_age, bool sharedRel,
  					  TransactionId *oldestXmin,
  					  TransactionId *freezeLimit)
  {
*************** vacuum_set_xid_limits(VacuumStmt *vacstm
*** 588,599 ****
  	Assert(TransactionIdIsNormal(*oldestXmin));
  
  	/*
! 	 * Determine the minimum freeze age to use: as specified in the vacstmt,
  	 * or vacuum_freeze_min_age, but in any case not more than half
  	 * autovacuum_freeze_max_age, so that autovacuums to prevent XID
  	 * wraparound won't occur too frequently.
  	 */
! 	freezemin = vacstmt->freeze_min_age;
  	if (freezemin < 0)
  		freezemin = vacuum_freeze_min_age;
  	freezemin = Min(freezemin, autovacuum_freeze_max_age / 2);
--- 588,599 ----
  	Assert(TransactionIdIsNormal(*oldestXmin));
  
  	/*
! 	 * Determine the minimum freeze age to use: as specified by the caller,
  	 * or vacuum_freeze_min_age, but in any case not more than half
  	 * autovacuum_freeze_max_age, so that autovacuums to prevent XID
  	 * wraparound won't occur too frequently.
  	 */
! 	freezemin = freeze_min_age;
  	if (freezemin < 0)
  		freezemin = vacuum_freeze_min_age;
  	freezemin = Min(freezemin, autovacuum_freeze_max_age / 2);
*************** full_vacuum_rel(Relation onerel, VacuumS
*** 1154,1160 ****
  				i;
  	VRelStats  *vacrelstats;
  
! 	vacuum_set_xid_limits(vacstmt, onerel->rd_rel->relisshared,
  						  &OldestXmin, &FreezeLimit);
  
  	/*
--- 1154,1160 ----
  				i;
  	VRelStats  *vacrelstats;
  
! 	vacuum_set_xid_limits(vacstmt->freeze_min_age, onerel->rd_rel->relisshared,
  						  &OldestXmin, &FreezeLimit);
  
  	/*
Index: src/backend/commands/vacuumlazy.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/commands/vacuumlazy.c,v
retrieving revision 1.88
diff -c -p -r1.88 vacuumlazy.c
*** src/backend/commands/vacuumlazy.c	30 Apr 2007 03:23:48 -0000	1.88
--- src/backend/commands/vacuumlazy.c	16 May 2007 22:34:55 -0000
*************** lazy_vacuum_rel(Relation onerel, VacuumS
*** 158,164 ****
  	else
  		elevel = DEBUG2;
  
! 	vacuum_set_xid_limits(vacstmt, onerel->rd_rel->relisshared,
  						  &OldestXmin, &FreezeLimit);
  
  	vacrelstats = (LVRelStats *) palloc0(sizeof(LVRelStats));
--- 158,164 ----
  	else
  		elevel = DEBUG2;
  
! 	vacuum_set_xid_limits(vacstmt->freeze_min_age, onerel->rd_rel->relisshared,
  						  &OldestXmin, &FreezeLimit);
  
  	vacrelstats = (LVRelStats *) palloc0(sizeof(LVRelStats));
Index: src/include/access/rewriteheap.h
===================================================================
RCS file: /cvsroot/pgsql/src/include/access/rewriteheap.h,v
retrieving revision 1.1
diff -c -p -r1.1 rewriteheap.h
*** src/include/access/rewriteheap.h	8 Apr 2007 01:26:33 -0000	1.1
--- src/include/access/rewriteheap.h	16 May 2007 22:35:05 -0000
***************
*** 20,29 ****
  typedef struct RewriteStateData *RewriteState;
  
  extern RewriteState begin_heap_rewrite(Relation NewHeap,
! 									   TransactionId OldestXmin, bool use_wal);
  extern void end_heap_rewrite(RewriteState state);
  extern void rewrite_heap_tuple(RewriteState state, HeapTuple oldTuple,
! 							   HeapTuple newTuple);
  extern void rewrite_heap_dead_tuple(RewriteState state, HeapTuple oldTuple);
  
  #endif /* REWRITE_HEAP_H */
--- 20,30 ----
  typedef struct RewriteStateData *RewriteState;
  
  extern RewriteState begin_heap_rewrite(Relation NewHeap,
! 				   TransactionId OldestXmin, TransactionId FreezeXid,
! 				   bool use_wal);
  extern void end_heap_rewrite(RewriteState state);
  extern void rewrite_heap_tuple(RewriteState state, HeapTuple oldTuple,
! 				   HeapTuple newTuple);
  extern void rewrite_heap_dead_tuple(RewriteState state, HeapTuple oldTuple);
  
  #endif /* REWRITE_HEAP_H */
Index: src/include/commands/vacuum.h
===================================================================
RCS file: /cvsroot/pgsql/src/include/commands/vacuum.h,v
retrieving revision 1.70
diff -c -p -r1.70 vacuum.h
*** src/include/commands/vacuum.h	13 Mar 2007 00:33:43 -0000	1.70
--- src/include/commands/vacuum.h	16 May 2007 22:35:08 -0000
*************** extern void vac_update_relstats(Oid reli
*** 119,125 ****
  					double num_tuples,
  					bool hasindex,
  					TransactionId frozenxid);
! extern void vacuum_set_xid_limits(VacuumStmt *vacstmt, bool sharedRel,
  					  TransactionId *oldestXmin,
  					  TransactionId *freezeLimit);
  extern void vac_update_datfrozenxid(void);
--- 119,125 ----
  					double num_tuples,
  					bool hasindex,
  					TransactionId frozenxid);
! extern void vacuum_set_xid_limits(int freeze_min_age, bool sharedRel,
  					  TransactionId *oldestXmin,
  					  TransactionId *freezeLimit);
  extern void vac_update_datfrozenxid(void);

---

From: Tom Lane <[email-masked]>
Date: Wed, 16 May 2007 20:00:41 -0400

> Here is my proposed patch.

Actually, the original patch in this series was fairly horrid, and
things haven't been made better by the subsequent changes.  It lacked
any comment explaining what it was doing; failed to comment on the way
it was abusing heap_freeze_tuple (the latter thinks it's getting a tuple
that's in a disk buffer); and IMHO puts the heap_freeze_tuple call in
the wrong place anyway.  raw_heap_insert has no business editorializing
on the tuple it's given.  It'd be better to have the call in
rewrite_heap_tuple, which is already busy messing with the tuple's
visibility info.  Perhaps like this, in addition to your changes:

*** src/backend/access/heap/rewriteheap.c~	Wed May 16 19:22:55 2007
--- src/backend/access/heap/rewriteheap.c	Wed May 16 19:54:46 2007
***************
*** 292,298 ****
  /*
   * Add a tuple to the new heap.
   *
!  * Visibility information is copied from the original tuple.
   *
   * state		opaque state as returned by begin_heap_rewrite
   * old_tuple	original tuple in the old heap
--- 292,300 ----
  /*
   * Add a tuple to the new heap.
   *
!  * Visibility information is copied from the original tuple, except that
!  * we "freeze" very-old tuples.  Note that since we scribble on new_tuple,
!  * it had better be temp storage not a pointer to the original tuple.
   *
   * state		opaque state as returned by begin_heap_rewrite
   * old_tuple	original tuple in the old heap
***************
*** 324,329 ****
--- 326,342 ----
  		old_tuple->t_data->t_infomask & HEAP_XACT_MASK;
  
  	/*
+ 	 * While we have our hands on the tuple, we may as well freeze any
+ 	 * very-old xmin or xmax, so that future VACUUM effort can be saved.
+ 	 *
+ 	 * Note we abuse heap_freeze_tuple() a bit here, since it's expecting
+ 	 * to be given a pointer to a tuple in a disk buffer.  It happens
+ 	 * though that we can get the right things to happen by passing
+ 	 * InvalidBuffer for the buffer.
+ 	 */
+ 	heap_freeze_tuple(new_tuple->t_data, state->rs_oldest_xmin, InvalidBuffer);
+ 
+ 	/*
  	 * Invalid ctid means that ctid should point to the tuple itself.
  	 * We'll override it later if the tuple is part of an update chain.
  	 */
***************
*** 537,544 ****
  	Size			len;
  	OffsetNumber	newoff;
  	HeapTuple		heaptup;
- 
- 	heap_freeze_tuple(tup->t_data, state->rs_oldest_xmin, InvalidBuffer);
  
  	/*
--- 550,555 ----

			regards, tom lane

---

From: Alvaro Herrera <[email-masked]>
Date: Thu, 17 May 2007 11:53:19 -0400

> visibility info.  Perhaps like this, in addition to your changes:

Applied, thanks.  Let me know if there is still something you don't like
about the current state of cluster or truncate.

The part about actually advancing relfrozenxid is still not done though ...

---

From: Tom Lane <[email-masked]>
Date: Thu, 17 May 2007 12:02:48 -0400

> The part about actually advancing relfrozenxid is still not done though ...

Right.  Are you intending to make that happen?

			regards, tom lane

---

From: Alvaro Herrera <[email-masked]>
Date: Thu, 17 May 2007 12:17:56 -0400

Yes.

---

From: Alvaro Herrera <[email-masked]>
Date: Thu, 17 May 2007 21:05:55 -0400

> Right.  Are you intending to make that happen?

See attached patch.  I'm intending to apply this sometime tomorrow.

Note that affecting CLUSTER changes code used by the ALTER TABLE
rewriting stuff as well; so with this patch, the latter also advances
relfrozenxid.  I could have chosen to keep it as it was, but instead I
chose RecentXmin as the new freeze point.  This is correct, because when
the table is rewritten, all the tuples are marked the rewriting
transaction's Xid, so for all purposes it behaves like a new table.

Curiously enough, TOAST tables are handled as fallout of other code, so
we don't need to do anything additional for this to work.

-- 
Alvaro Herrera                                http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Index: src/backend/commands/cluster.c
===================================================================
RCS file: /home/alvherre/Code/cvs/pgsql/src/backend/commands/cluster.c,v
retrieving revision 1.160
diff -c -p -r1.160 cluster.c
*** src/backend/commands/cluster.c	17 May 2007 15:28:29 -0000	1.160
--- src/backend/commands/cluster.c	18 May 2007 01:01:45 -0000
*************** typedef struct
*** 55,61 ****

--- 55,61 ----

*** 513,518 ****
--- 513,519 ----
  
*** 539,551 ****

  	/* Swap the physical files of the old and new heaps. */
! 	swap_relation_files(tableOid, OIDNewHeap);

--- 540,552 ----
  	/*

  	/* Swap the physical files of the old and new heaps. */
! 	swap_relation_files(tableOid, OIDNewHeap, frozenXid);

*** 641,649 ****
  }
  
  /*
--- 642,651 ----
  }
  
  /*
!  * Do the physical copying of heap data.  Returns the TransactionId used as
!  * freeze cutoff point for the tuples.
   */
! static TransactionId
  copy_heap_data(Oid OIDNewHeap, Oid OIDOldHeap, Oid OIDOldIndex)
  {
  	Relation	NewHeap,
*************** copy_heap_data(Oid OIDNewHeap, Oid OIDOl
*** 816,821 ****
--- 818,825 ----
  	index_close(OldIndex, NoLock);
  	heap_close(OldHeap, NoLock);
  	heap_close(NewHeap, NoLock);
+ 
+ 	return FreezeXid;
  }
  
  /*
*************** copy_heap_data(Oid OIDNewHeap, Oid OIDOl
*** 826,834 ****
   *
   * Also swap any TOAST links, so that the toast data moves along with
   * the main-table data.
   */
  void
! swap_relation_files(Oid r1, Oid r2)
  {
  	Relation	relRelation;
  	HeapTuple	reltup1,
--- 830,845 ----
   *
   * Also swap any TOAST links, so that the toast data moves along with
   * the main-table data.
+  *
+  * Additionally, the first relation is marked with relfrozenxid set to
+  * frozenXid.  It seems a bit ugly to have this here, but all callers would
+  * have to do it anyway, so having it here saves a heap_update.  Note: the
+  * TOAST table needs no special handling, because since we swapped the links,
+  * the entry for the TOAST table will now contain RecentXmin in relfrozenxid,
+  * which is the correct value.
   */
  void
! swap_relation_files(Oid r1, Oid r2, TransactionId frozenXid)
  {
  	Relation	relRelation;
  	HeapTuple	reltup1,
*************** swap_relation_files(Oid r1, Oid r2)
*** 872,877 ****
--- 883,891 ----
  
  	/* we should not swap reltoastidxid */
  
+ 	/* set rel1's frozen Xid */
+ 	relform1->relfrozenxid = frozenXid;
+ 
  	/* swap size statistics too, since new rel has freshly-updated stats */
  	{
  		int4		swap_pages;
Index: src/backend/commands/tablecmds.c
===================================================================
RCS file: /home/alvherre/Code/cvs/pgsql/src/backend/commands/tablecmds.c,v
retrieving revision 1.224
diff -c -p -r1.224 tablecmds.c
*** src/backend/commands/tablecmds.c	16 May 2007 17:28:20 -0000	1.224
--- src/backend/commands/tablecmds.c	18 May 2007 00:08:51 -0000
*************** ATRewriteTables(List **wqueue)
*** 2285,2292 ****
  			 */
  			ATRewriteTable(tab, OIDNewHeap);
  
! 			/* Swap the physical files of the old and new heaps. */
! 			swap_relation_files(tab->relid, OIDNewHeap);

--- 2285,2297 ----
  			 */
  			ATRewriteTable(tab, OIDNewHeap);
  
! 			/*
! 			 * Swap the physical files of the old and new heaps.  Since we are
! 			 * generating a new heap, we can use RecentXmin for the table's new
! 			 * relfrozenxid because we rewrote all the tuples on
! 			 * ATRewriteTable, so no older Xid remains on the table.
! 			 */
! 			swap_relation_files(tab->relid, OIDNewHeap, RecentXmin);

Index: src/include/commands/cluster.h
===================================================================
RCS file: /home/alvherre/Code/cvs/pgsql/src/include/commands/cluster.h,v
retrieving revision 1.32
diff -c -p -r1.32 cluster.h
*** src/include/commands/cluster.h	13 Mar 2007 00:33:43 -0000	1.32
--- src/include/commands/cluster.h	18 May 2007 00:22:26 -0000
*************** extern void check_index_is_clusterable(R
*** 24,29 ****
  extern void mark_index_clustered(Relation rel, Oid indexOid);
  extern Oid make_new_heap(Oid OIDOldHeap, const char *NewName,
  			  Oid NewTableSpace);
! extern void swap_relation_files(Oid r1, Oid r2);
  
  #endif   /* CLUSTER_H */
--- 24,29 ----
  extern void mark_index_clustered(Relation rel, Oid indexOid);
  extern Oid make_new_heap(Oid OIDOldHeap, const char *NewName,
  			  Oid NewTableSpace);
! extern void swap_relation_files(Oid r1, Oid r2, TransactionId frozenXid);
  
  #endif   /* CLUSTER_H */