Code review for HeapTupleHeader changes. Add version number to page headers

(overlaying low byte of page size) and add HEAP_HASOID bit to t_infomask,
per earlier discussion.  Simplify scheme for overlaying fields in tuple
header (no need for cmax to live in more than one place).  Don't try to
clear infomask status bits in tqual.c --- not safe to do it there.  Don't
try to force output table of a SELECT INTO to have OIDs, either.  Get rid
of unnecessarily complex three-state scheme for TupleDesc.tdhasoids, which
has already caused one recent failure.  Improve documentation.

src/backend/access/common/heaptuple.c

@@ -9,7 +9,7 @@
  *
  *
  * IDENTIFICATION
- *	  $Header: /cvsroot/pgsql/src/backend/access/common/heaptuple.c,v 1.80 2002/08/25 17:20:00 tgl Exp $
+ *	  $Header: /cvsroot/pgsql/src/backend/access/common/heaptuple.c,v 1.81 2002/09/02 01:05:03 tgl Exp $
  *
  * NOTES
  *	  The old interface functions have been converted to macros
@@ -80,7 +80,7 @@ DataFill(char *data,
 		bitmask = CSIGNBIT;
 	}
 
-	*infomask &= HEAP_XACT_MASK;
+	*infomask &= ~(HEAP_HASNULL | HEAP_HASVARWIDTH | HEAP_HASEXTENDED);
 
 	for (i = 0; i < numberOfAttributes; i++)
 	{
@@ -584,8 +584,6 @@ heap_formtuple(TupleDesc tupleDescriptor,
 		elog(ERROR, "heap_formtuple: numberOfAttributes %d exceeds limit %d",
 			 numberOfAttributes, MaxTupleAttributeNumber);
 
-	AssertTupleDescHasOidIsValid(tupleDescriptor);
-
 	for (i = 0; i < numberOfAttributes; i++)
 	{
 		if (nulls[i] != ' ')
@@ -600,7 +598,7 @@ heap_formtuple(TupleDesc tupleDescriptor,
 	if (hasnull)
 		len += BITMAPLEN(numberOfAttributes);
 
-	if (tupleDescriptor->tdhasoid == WITHOID)
+	if (tupleDescriptor->tdhasoid)
 		len += sizeof(Oid);
 
 	hoff = len = MAXALIGN(len); /* align user data safely */
@@ -626,6 +624,9 @@ heap_formtuple(TupleDesc tupleDescriptor,
 			 &td->t_infomask,
 			 (hasnull ? td->t_bits : NULL));
 
+	if (tupleDescriptor->tdhasoid)
+		td->t_infomask |= HEAP_HASOID;
+
 	td->t_infomask |= HEAP_XMAX_INVALID;
 
 	return tuple;
@@ -651,7 +652,6 @@ heap_modifytuple(HeapTuple tuple,
 	char	   *nulls;
 	bool		isNull;
 	HeapTuple	newTuple;
-	uint8		infomask;
 
 	/*
 	 * sanity checks
@@ -702,18 +702,10 @@ heap_modifytuple(HeapTuple tuple,
 							  nulls);
 
 	/*
-	 * copy the header except for t_len, t_natts, t_hoff, t_bits,
-	 * t_infomask
+	 * copy the identification info of the old tuple: t_ctid, t_self,
+	 * and OID (if any)
 	 */
-	infomask = newTuple->t_data->t_infomask;
-	/*
-	 * copy t_xmin, t_cid, t_xmax, t_ctid, t_natts, t_infomask
-	 */
-	memmove((char *) newTuple->t_data,	/* XXX */
-			(char *) tuple->t_data,
-			offsetof(HeapTupleHeaderData, t_hoff));	/* XXX */
-	newTuple->t_data->t_infomask = infomask;
-	newTuple->t_data->t_natts = numberOfAttributes;
+	newTuple->t_data->t_ctid = tuple->t_data->t_ctid;
 	newTuple->t_self = tuple->t_self;
 	newTuple->t_tableOid = tuple->t_tableOid;
 	if (relation->rd_rel->relhasoids)
@@ -776,11 +768,11 @@ heap_addheader(int natts,		/* max domain index */
 	tuple->t_datamcxt = CurrentMemoryContext;
 	tuple->t_data = td = (HeapTupleHeader) ((char *) tuple + HEAPTUPLESIZE);
 
-	MemSet((char *) td, 0, len);
+	MemSet((char *) td, 0, hoff);
 
 	td->t_hoff = hoff;
 	td->t_natts = natts;
-	td->t_infomask = HEAP_XMAX_INVALID; /* XXX sufficient? */
+	td->t_infomask = withoid ? (HEAP_XMAX_INVALID | HEAP_HASOID) : HEAP_XMAX_INVALID;
 
 	memcpy((char *) td + hoff, structure, structlen);
 

src/backend/access/common/tupdesc.c

@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *	  $Header: /cvsroot/pgsql/src/backend/access/common/tupdesc.c,v 1.87 2002/08/30 19:23:18 tgl Exp $
+ *	  $Header: /cvsroot/pgsql/src/backend/access/common/tupdesc.c,v 1.88 2002/09/02 01:05:03 tgl Exp $
  *
  * NOTES
  *	  some of the executor utility code such as "ExecTypeFromTL" should be
@@ -37,7 +37,7 @@
  * ----------------------------------------------------------------
  */
 TupleDesc
-CreateTemplateTupleDesc(int natts, hasoid_t withoid)
+CreateTemplateTupleDesc(int natts, bool hasoid)
 {
 	uint32		size;
 	TupleDesc	desc;
@@ -59,7 +59,7 @@ CreateTemplateTupleDesc(int natts, hasoid_t withoid)
 	MemSet(desc->attrs, 0, size);
 
 	desc->natts = natts;
-	desc->tdhasoid = withoid;
+	desc->tdhasoid = hasoid;
 
 	return desc;
 }
@@ -67,11 +67,12 @@ CreateTemplateTupleDesc(int natts, hasoid_t withoid)
 /* ----------------------------------------------------------------
  *		CreateTupleDesc
  *
- *		This function allocates a new TupleDesc from Form_pg_attribute array
+ *		This function allocates a new TupleDesc pointing to a given
+ *		Form_pg_attribute array
  * ----------------------------------------------------------------
  */
 TupleDesc
-CreateTupleDesc(int natts, Form_pg_attribute *attrs)
+CreateTupleDesc(int natts, bool hasoid, Form_pg_attribute *attrs)
 {
 	TupleDesc	desc;
 
@@ -84,7 +85,7 @@ CreateTupleDesc(int natts, Form_pg_attribute *attrs)
 	desc->attrs = attrs;
 	desc->natts = natts;
 	desc->constr = NULL;
-	desc->tdhasoid = UNDEFOID;
+	desc->tdhasoid = hasoid;
 
 	return desc;
 }
@@ -129,7 +130,6 @@ CreateTupleDescCopy(TupleDesc tupdesc)
  *
  *		This function creates a new TupleDesc by copying from an existing
  *		TupleDesc (with Constraints)
- *
  * ----------------------------------------------------------------
  */
 TupleDesc
@@ -477,6 +477,9 @@ TupleDescInitEntry(TupleDesc desc,
  * BuildDescForRelation
  *
  * Given a relation schema (list of ColumnDef nodes), build a TupleDesc.
+ *
+ * Note: the default assumption is no OIDs; caller may modify the returned
+ * TupleDesc if it wants OIDs.
  */
 TupleDesc
 BuildDescForRelation(List *schema)
@@ -497,7 +500,7 @@ BuildDescForRelation(List *schema)
 	 * allocate a new tuple descriptor
 	 */
 	natts = length(schema);
-	desc = CreateTemplateTupleDesc(natts, UNDEFOID);
+	desc = CreateTemplateTupleDesc(natts, false);
 	constr->has_not_null = false;
 
 	attnum = 0;
@@ -667,7 +670,7 @@ TypeGetTupleDesc(Oid typeoid, List *colaliases)
 		/* OK, get the column alias */
 		attname = strVal(lfirst(colaliases));
 
-		tupdesc = CreateTemplateTupleDesc(1, WITHOUTOID);
+		tupdesc = CreateTemplateTupleDesc(1, false);
 		TupleDescInitEntry(tupdesc,
 						   (AttrNumber) 1,
 						   attname,

src/backend/access/heap/heapam.c

@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *	  $Header: /cvsroot/pgsql/src/backend/access/heap/heapam.c,v 1.146 2002/08/29 00:17:02 tgl Exp $
+ *	  $Header: /cvsroot/pgsql/src/backend/access/heap/heapam.c,v 1.147 2002/09/02 01:05:03 tgl Exp $
  *
  *
  * INTERFACE ROUTINES
@@ -1116,6 +1116,10 @@ heap_insert(Relation relation, HeapTuple tup, CommandId cid)
 
 	if (relation->rd_rel->relhasoids)
 	{
+#ifdef NOT_USED
+		/* this is redundant with an Assert in HeapTupleSetOid */
+		Assert(tup->t_data->t_infomask & HEAP_HASOID);
+#endif
 		/*
 		 * If the object id of this tuple has already been assigned, trust
 		 * the caller.	There are a couple of ways this can happen.  At
@@ -1125,22 +1129,21 @@ heap_insert(Relation relation, HeapTuple tup, CommandId cid)
 		 * to support a persistent object store (objects need to contain
 		 * pointers to one another).
 		 */
-		AssertTupleDescHasOid(relation->rd_att);
 		if (!OidIsValid(HeapTupleGetOid(tup)))
 			HeapTupleSetOid(tup, newoid());
 		else
 			CheckMaxObjectId(HeapTupleGetOid(tup));
 	}
+	else
+	{
+		/* check there is not space for an OID */
+		Assert(!(tup->t_data->t_infomask & HEAP_HASOID));
+	}
 
 	tup->t_data->t_infomask &= ~(HEAP_XACT_MASK);
+	tup->t_data->t_infomask |= HEAP_XMAX_INVALID;
 	HeapTupleHeaderSetXmin(tup->t_data, GetCurrentTransactionId());
 	HeapTupleHeaderSetCmin(tup->t_data, cid);
-	HeapTupleHeaderSetXmaxInvalid(tup->t_data);
-	/*
-	 * Do *not* set Cmax!  This would overwrite Cmin.
-	 */
-	/* HeapTupleHeaderSetCmax(tup->t_data, FirstCommandId); */
-	tup->t_data->t_infomask |= HEAP_XMAX_INVALID;
 	tup->t_tableOid = relation->rd_id;
 
 #ifdef TUPLE_TOASTER_ACTIVE
@@ -1181,13 +1184,7 @@ heap_insert(Relation relation, HeapTuple tup, CommandId cid)
 		rdata[0].len = SizeOfHeapInsert;
 		rdata[0].next = &(rdata[1]);
 
-		if (relation->rd_rel->relhasoids)
-		{
-			AssertTupleDescHasOid(relation->rd_att);
-			xlhdr.t_oid = HeapTupleGetOid(tup);
-		}
-		else
-			xlhdr.t_oid = InvalidOid;
+		xlhdr.t_oid = HeapTupleGetOid(tup);
 		xlhdr.t_natts = tup->t_data->t_natts;
 		xlhdr.t_hoff = tup->t_data->t_hoff;
 		xlhdr.mask = tup->t_data->t_infomask;
@@ -1234,10 +1231,6 @@ heap_insert(Relation relation, HeapTuple tup, CommandId cid)
 	 */
 	CacheInvalidateHeapTuple(relation, tup);
 
-	if (!relation->rd_rel->relhasoids)
-		return InvalidOid;
-		
-	AssertTupleDescHasOid(relation->rd_att);
 	return HeapTupleGetOid(tup);
 }
 
@@ -1343,7 +1336,9 @@ l1:
 
 	/* store transaction information of xact deleting the tuple */
 	tp.t_data->t_infomask &= ~(HEAP_XMAX_COMMITTED |
-							 HEAP_XMAX_INVALID | HEAP_MARKED_FOR_UPDATE);
+							   HEAP_XMAX_INVALID |
+							   HEAP_MARKED_FOR_UPDATE |
+							   HEAP_MOVED);
 	HeapTupleHeaderSetXmax(tp.t_data, GetCurrentTransactionId());
 	HeapTupleHeaderSetCmax(tp.t_data, cid);
 	/* Make sure there is no forward chain link in t_ctid */
@@ -1543,14 +1538,22 @@ l2:
 	/* Fill in OID and transaction status data for newtup */
 	if (relation->rd_rel->relhasoids)
 	{
-		AssertTupleDescHasOid(relation->rd_att);
+#ifdef NOT_USED
+		/* this is redundant with an Assert in HeapTupleSetOid */
+		Assert(newtup->t_data->t_infomask & HEAP_HASOID);
+#endif
 		HeapTupleSetOid(newtup, HeapTupleGetOid(&oldtup));
 	}
+	else
+	{
+		/* check there is not space for an OID */
+		Assert(!(newtup->t_data->t_infomask & HEAP_HASOID));
+	}
+
 	newtup->t_data->t_infomask &= ~(HEAP_XACT_MASK);
 	newtup->t_data->t_infomask |= (HEAP_XMAX_INVALID | HEAP_UPDATED);
 	HeapTupleHeaderSetXmin(newtup->t_data, GetCurrentTransactionId());
 	HeapTupleHeaderSetCmin(newtup->t_data, cid);
-	HeapTupleHeaderSetXmaxInvalid(newtup->t_data);
 
 	/*
 	 * If the toaster needs to be activated, OR if the new tuple will not
@@ -1586,7 +1589,8 @@ l2:
 
 		oldtup.t_data->t_infomask &= ~(HEAP_XMAX_COMMITTED |
 									   HEAP_XMAX_INVALID |
-									   HEAP_MARKED_FOR_UPDATE);
+									   HEAP_MARKED_FOR_UPDATE |
+									   HEAP_MOVED);
 		oldtup.t_data->t_infomask |= HEAP_XMAX_UNLOGGED;
 		HeapTupleHeaderSetXmax(oldtup.t_data, GetCurrentTransactionId());
 		HeapTupleHeaderSetCmax(oldtup.t_data, cid);
@@ -1677,7 +1681,8 @@ l2:
 	{
 		oldtup.t_data->t_infomask &= ~(HEAP_XMAX_COMMITTED |
 									   HEAP_XMAX_INVALID |
-									   HEAP_MARKED_FOR_UPDATE);
+									   HEAP_MARKED_FOR_UPDATE |
+									   HEAP_MOVED);
 		HeapTupleHeaderSetXmax(oldtup.t_data, GetCurrentTransactionId());
 		HeapTupleHeaderSetCmax(oldtup.t_data, cid);
 	}
@@ -1852,7 +1857,9 @@ l3:
 	((PageHeader) BufferGetPage(*buffer))->pd_sui = ThisStartUpID;
 
 	/* store transaction information of xact marking the tuple */
-	tuple->t_data->t_infomask &= ~(HEAP_XMAX_COMMITTED | HEAP_XMAX_INVALID);
+	tuple->t_data->t_infomask &= ~(HEAP_XMAX_COMMITTED |
+								   HEAP_XMAX_INVALID |
+								   HEAP_MOVED);
 	tuple->t_data->t_infomask |= HEAP_MARKED_FOR_UPDATE;
 	HeapTupleHeaderSetXmax(tuple->t_data, GetCurrentTransactionId());
 	HeapTupleHeaderSetCmax(tuple->t_data, cid);
@@ -2032,13 +2039,7 @@ log_heap_update(Relation reln, Buffer oldbuf, ItemPointerData from,
 	rdata[1].len = 0;
 	rdata[1].next = &(rdata[2]);
 
-	if (reln->rd_rel->relhasoids)
-	{
-		AssertTupleDescHasOid(reln->rd_att);
-		xlhdr.hdr.t_oid = HeapTupleGetOid(newtup);
-	}
-	else
-		xlhdr.hdr.t_oid = InvalidOid;
+	xlhdr.hdr.t_oid = HeapTupleGetOid(newtup);
 	xlhdr.hdr.t_natts = newtup->t_data->t_natts;
 	xlhdr.hdr.t_hoff = newtup->t_data->t_hoff;
 	xlhdr.hdr.mask = newtup->t_data->t_infomask;
@@ -2197,12 +2198,10 @@ heap_xlog_delete(bool redo, XLogRecPtr lsn, XLogRecord *record)
 
 	if (redo)
 	{
-		/*
-		 * On redo from WAL we cannot rely on a tqual-routine
-		 * to have reset HEAP_MOVED.
-		 */
-		htup->t_infomask &= ~(HEAP_MOVED | HEAP_XMAX_COMMITTED |
-							  HEAP_XMAX_INVALID | HEAP_MARKED_FOR_UPDATE);
+		htup->t_infomask &= ~(HEAP_XMAX_COMMITTED |
+							  HEAP_XMAX_INVALID |
+							  HEAP_MARKED_FOR_UPDATE |
+							  HEAP_MOVED);
 		HeapTupleHeaderSetXmax(htup, record->xl_xid);
 		HeapTupleHeaderSetCmax(htup, FirstCommandId);
 		/* Make sure there is no forward chain link in t_ctid */
@@ -2284,14 +2283,9 @@ heap_xlog_insert(bool redo, XLogRecPtr lsn, XLogRecord *record)
 		htup->t_infomask = HEAP_XMAX_INVALID | xlhdr.mask;
 		HeapTupleHeaderSetXmin(htup, record->xl_xid);
 		HeapTupleHeaderSetCmin(htup, FirstCommandId);
-		HeapTupleHeaderSetXmaxInvalid(htup);
-		HeapTupleHeaderSetCmax(htup, FirstCommandId);
 		htup->t_ctid = xlrec->target.tid;
 		if (reln->rd_rel->relhasoids)
-		{
-			AssertTupleDescHasOid(reln->rd_att);
 			HeapTupleHeaderSetOid(htup, xlhdr.t_oid);
-		}
 
 		offnum = PageAddItem(page, (Item) htup, newlen, offnum,
 							 LP_USED | OverwritePageMode);
@@ -2372,8 +2366,9 @@ heap_xlog_update(bool redo, XLogRecPtr lsn, XLogRecord *record, bool move)
 	{
 		if (move)
 		{
-			htup->t_infomask &=
-				~(HEAP_XMIN_COMMITTED | HEAP_XMIN_INVALID | HEAP_MOVED_IN);
+			htup->t_infomask &= ~(HEAP_XMIN_COMMITTED |
+								  HEAP_XMIN_INVALID |
+								  HEAP_MOVED_IN);
 			htup->t_infomask |= HEAP_MOVED_OFF;
 			HeapTupleHeaderSetXvac(htup, record->xl_xid);
 			/* Make sure there is no forward chain link in t_ctid */
@@ -2381,12 +2376,10 @@ heap_xlog_update(bool redo, XLogRecPtr lsn, XLogRecord *record, bool move)
 		}
 		else
 		{
-			/*
-			 * On redo from WAL we cannot rely on a tqual-routine
-			 * to have reset HEAP_MOVED.
-			 */
-			htup->t_infomask &= ~(HEAP_MOVED | HEAP_XMAX_COMMITTED |
-							 HEAP_XMAX_INVALID | HEAP_MARKED_FOR_UPDATE);
+			htup->t_infomask &= ~(HEAP_XMAX_COMMITTED |
+								  HEAP_XMAX_INVALID |
+								  HEAP_MARKED_FOR_UPDATE |
+								  HEAP_MOVED);
 			HeapTupleHeaderSetXmax(htup, record->xl_xid);
 			HeapTupleHeaderSetCmax(htup, FirstCommandId);
 			/* Set forward chain link in t_ctid */
@@ -2466,10 +2459,8 @@ newsame:;
 		htup->t_natts = xlhdr.t_natts;
 		htup->t_hoff = xlhdr.t_hoff;
 		if (reln->rd_rel->relhasoids)
-		{
-			AssertTupleDescHasOid(reln->rd_att);
 			HeapTupleHeaderSetOid(htup, xlhdr.t_oid);
-		}
+
 		if (move)
 		{
 			TransactionId xid[2];   /* xmax, xmin */
@@ -2479,7 +2470,8 @@ newsame:;
 			       (char *) xlrec + hsize, 2 * sizeof(TransactionId));
 			htup->t_infomask = xlhdr.mask;
 			htup->t_infomask &= ~(HEAP_XMIN_COMMITTED |
-								  HEAP_XMIN_INVALID | HEAP_MOVED_OFF);
+								  HEAP_XMIN_INVALID |
+								  HEAP_MOVED_OFF);
 			htup->t_infomask |= HEAP_MOVED_IN;
 			HeapTupleHeaderSetXmin(htup, xid[1]);
 			HeapTupleHeaderSetXmax(htup, xid[0]);
@@ -2490,8 +2482,6 @@ newsame:;
 			htup->t_infomask = HEAP_XMAX_INVALID | xlhdr.mask;
 			HeapTupleHeaderSetXmin(htup, record->xl_xid);
 			HeapTupleHeaderSetCmin(htup, FirstCommandId);
-			HeapTupleHeaderSetXmaxInvalid(htup);
-			HeapTupleHeaderSetCmax(htup, FirstCommandId);
 		}
 		/* Make sure there is no forward chain link in t_ctid */
 		htup->t_ctid = xlrec->newtid;

src/backend/access/heap/tuptoaster.c

@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *	  $Header: /cvsroot/pgsql/src/backend/access/heap/tuptoaster.c,v 1.34 2002/08/06 02:36:33 tgl Exp $
+ *	  $Header: /cvsroot/pgsql/src/backend/access/heap/tuptoaster.c,v 1.35 2002/09/02 01:05:03 tgl Exp $
  *
  *
  * INTERFACE ROUTINES
@@ -726,7 +726,7 @@ toast_insert_or_update(Relation rel, HeapTuple newtup, HeapTuple oldtup)
 		new_len = offsetof(HeapTupleHeaderData, t_bits);
 		if (has_nulls)
 			new_len += BITMAPLEN(numAttrs);
-		if (rel->rd_rel->relhasoids)
+		if (olddata->t_infomask & HEAP_HASOID)
 			new_len += sizeof(Oid);
 		new_len = MAXALIGN(new_len);
 		Assert(new_len == olddata->t_hoff);