From d937a477ce8c520818ffaabc458dd32854be7c68 Mon Sep 17 00:00:00 2001
From: David Rowley <dgrowley@gmail.com>
Date: Wed, 29 May 2024 12:19:03 +1200
Subject: [PATCH v4 3/6] Optimize alignment calculations in tuple form/deform

This converts CompactAttribute.attalign from a char which is directly
derived from pg_attribute.attalign into a uint8 which specifies the
number of bytes to align the column by.  Also, rename the field to
attalignby to make the distinction more clear in code.

This removes the complexity of checking each char value and transforming
that into the appropriate alignment call.  This can just be a simple
TYPEALIGN passing in the number of bytes.
---
 contrib/amcheck/verify_heapam.c        |  6 +--
 contrib/pageinspect/heapfuncs.c        |  6 +--
 src/backend/access/brin/brin_tuple.c   |  8 ++--
 src/backend/access/common/attmap.c     |  2 +-
 src/backend/access/common/heaptuple.c  | 52 +++++++++++++-------------
 src/backend/access/common/indextuple.c | 22 +++++------
 src/backend/access/common/tupdesc.c    | 20 +++++++++-
 src/backend/executor/execExprInterp.c  |  2 +-
 src/backend/executor/execTuples.c      | 19 +++++-----
 src/backend/jit/llvm/llvmjit_deform.c  | 17 +--------
 src/include/access/tupdesc.h           |  2 +-
 src/include/access/tupmacs.h           | 23 ++++++++++++
 12 files changed, 101 insertions(+), 78 deletions(-)

diff --git a/contrib/amcheck/verify_heapam.c b/contrib/amcheck/verify_heapam.c
index 08772de39f..b66eb178b9 100644
--- a/contrib/amcheck/verify_heapam.c
+++ b/contrib/amcheck/verify_heapam.c
@@ -1592,7 +1592,7 @@ check_tuple_attribute(HeapCheckContext *ctx)
 	/* Skip non-varlena values, but update offset first */
 	if (thisatt->attlen != -1)
 	{
-		ctx->offset = att_align_nominal(ctx->offset, thisatt->attalign);
+		ctx->offset = att_nominal_alignby(ctx->offset, thisatt->attalignby);
 		ctx->offset = att_addlength_pointer(ctx->offset, thisatt->attlen,
 											tp + ctx->offset);
 		if (ctx->tuphdr->t_hoff + ctx->offset > ctx->lp_len)
@@ -1608,8 +1608,8 @@ check_tuple_attribute(HeapCheckContext *ctx)
 	}
 
 	/* Ok, we're looking at a varlena attribute. */
-	ctx->offset = att_align_pointer(ctx->offset, thisatt->attalign, -1,
-									tp + ctx->offset);
+	ctx->offset = att_pointer_alignby(ctx->offset, thisatt->attalignby, -1,
+									  tp + ctx->offset);
 
 	/* Get the (possibly corrupt) varlena datum */
 	attdatum = fetchatt(thisatt, tp + ctx->offset);
diff --git a/contrib/pageinspect/heapfuncs.c b/contrib/pageinspect/heapfuncs.c
index 397259d168..6bdb3c930c 100644
--- a/contrib/pageinspect/heapfuncs.c
+++ b/contrib/pageinspect/heapfuncs.c
@@ -358,8 +358,8 @@ tuple_data_split_internal(Oid relid, char *tupdata,
 
 			if (attr->attlen == -1)
 			{
-				off = att_align_pointer(off, attr->attalign, -1,
-										tupdata + off);
+				off = att_pointer_alignby(off, attr->attalignby, -1,
+										  tupdata + off);
 
 				/*
 				 * As VARSIZE_ANY throws an exception if it can't properly
@@ -377,7 +377,7 @@ tuple_data_split_internal(Oid relid, char *tupdata,
 			}
 			else
 			{
-				off = att_align_nominal(off, attr->attalign);
+				off = att_nominal_alignby(off, attr->attalignby);
 				len = attr->attlen;
 			}
 
diff --git a/src/backend/access/brin/brin_tuple.c b/src/backend/access/brin/brin_tuple.c
index aae646be5d..e65c1efff4 100644
--- a/src/backend/access/brin/brin_tuple.c
+++ b/src/backend/access/brin/brin_tuple.c
@@ -703,13 +703,13 @@ brin_deconstruct_tuple(BrinDesc *brdesc,
 
 			if (thisatt->attlen == -1)
 			{
-				off = att_align_pointer(off, thisatt->attalign, -1,
-										tp + off);
+				off = att_pointer_alignby(off, thisatt->attalignby, -1,
+										  tp + off);
 			}
 			else
 			{
-				/* not varlena, so safe to use att_align_nominal */
-				off = att_align_nominal(off, thisatt->attalign);
+				/* not varlena, so safe to use att_nominal_alignby */
+				off = att_nominal_alignby(off, thisatt->attalignby);
 			}
 
 			values[stored++] = fetchatt(thisatt, tp + off);
diff --git a/src/backend/access/common/attmap.c b/src/backend/access/common/attmap.c
index 0805c4121e..29ce51ec3b 100644
--- a/src/backend/access/common/attmap.c
+++ b/src/backend/access/common/attmap.c
@@ -321,7 +321,7 @@ check_attrmap_match(TupleDesc indesc,
 		if (attrMap->attnums[i] == 0 &&
 			inatt->attisdropped &&
 			inatt->attlen == outatt->attlen &&
-			inatt->attalign == outatt->attalign)
+			inatt->attalignby == outatt->attalignby)
 			continue;
 
 		return false;
diff --git a/src/backend/access/common/heaptuple.c b/src/backend/access/common/heaptuple.c
index d0a922011e..ef601dde6e 100644
--- a/src/backend/access/common/heaptuple.c
+++ b/src/backend/access/common/heaptuple.c
@@ -250,13 +250,13 @@ heap_compute_data_size(TupleDesc tupleDesc,
 			 * we want to flatten the expanded value so that the constructed
 			 * tuple doesn't depend on it
 			 */
-			data_length = att_align_nominal(data_length, atti->attalign);
+			data_length = att_nominal_alignby(data_length, atti->attalignby);
 			data_length += EOH_get_flat_size(DatumGetEOHP(val));
 		}
 		else
 		{
-			data_length = att_align_datum(data_length, atti->attalign,
-										  atti->attlen, val);
+			data_length = att_datum_alignby(data_length, atti->attalignby,
+											atti->attlen, val);
 			data_length = att_addlength_datum(data_length, atti->attlen,
 											  val);
 		}
@@ -307,13 +307,13 @@ fill_val(CompactAttribute *att,
 	}
 
 	/*
-	 * XXX we use the att_align macros on the pointer value itself, not on an
-	 * offset.  This is a bit of a hack.
+	 * XXX we use the att_nominal_alignby macro on the pointer value itself,
+	 * not on an offset.  This is a bit of a hack.
 	 */
 	if (att->attbyval)
 	{
 		/* pass-by-value */
-		data = (char *) att_align_nominal(data, att->attalign);
+		data = (char *) att_nominal_alignby(data, att->attalignby);
 		store_att_byval(data, datum, att->attlen);
 		data_length = att->attlen;
 	}
@@ -333,8 +333,7 @@ fill_val(CompactAttribute *att,
 				 */
 				ExpandedObjectHeader *eoh = DatumGetEOHP(datum);
 
-				data = (char *) att_align_nominal(data,
-												  att->attalign);
+				data = (char *) att_nominal_alignby(data, att->attalignby);
 				data_length = EOH_get_flat_size(eoh);
 				EOH_flatten_into(eoh, data, data_length);
 			}
@@ -362,8 +361,7 @@ fill_val(CompactAttribute *att,
 		else
 		{
 			/* full 4-byte header varlena */
-			data = (char *) att_align_nominal(data,
-											  att->attalign);
+			data = (char *) att_nominal_alignby(data, att->attalignby);
 			data_length = VARSIZE(val);
 			memcpy(data, val, data_length);
 		}
@@ -372,14 +370,14 @@ fill_val(CompactAttribute *att,
 	{
 		/* cstring ... never needs alignment */
 		*infomask |= HEAP_HASVARWIDTH;
-		Assert(att->attalign == TYPALIGN_CHAR);
+		Assert(att->attalignby == sizeof(char));
 		data_length = strlen(DatumGetCString(datum)) + 1;
 		memcpy(data, DatumGetPointer(datum), data_length);
 	}
 	else
 	{
 		/* fixed-length pass-by-reference */
-		data = (char *) att_align_nominal(data, att->attalign);
+		data = (char *) att_nominal_alignby(data, att->attalignby);
 		Assert(att->attlen > 0);
 		data_length = att->attlen;
 		memcpy(data, DatumGetPointer(datum), data_length);
@@ -633,7 +631,7 @@ nocachegetattr(HeapTuple tup,
 			if (att->attlen <= 0)
 				break;
 
-			off = att_align_nominal(off, att->attalign);
+			off = att_nominal_alignby(off, att->attalignby);
 
 			att->attcacheoff = off;
 
@@ -682,19 +680,19 @@ nocachegetattr(HeapTuple tup,
 				 * either an aligned or unaligned value.
 				 */
 				if (usecache &&
-					off == att_align_nominal(off, att->attalign))
+					off == att_nominal_alignby(off, att->attalignby))
 					att->attcacheoff = off;
 				else
 				{
-					off = att_align_pointer(off, att->attalign, -1,
-											tp + off);
+					off = att_pointer_alignby(off, att->attalignby, -1,
+											  tp + off);
 					usecache = false;
 				}
 			}
 			else
 			{
-				/* not varlena, so safe to use att_align_nominal */
-				off = att_align_nominal(off, att->attalign);
+				/* not varlena, so safe to use att_nominal_alignby */
+				off = att_nominal_alignby(off, att->attalignby);
 
 				if (usecache)
 					att->attcacheoff = off;
@@ -897,10 +895,10 @@ expand_tuple(HeapTuple *targetHeapTuple,
 			{
 				CompactAttribute *att = TupleDescCompactAttr(tupleDesc, attnum);
 
-				targetDataLen = att_align_datum(targetDataLen,
-												att->attalign,
-												att->attlen,
-												attrmiss[attnum].am_value);
+				targetDataLen = att_datum_alignby(targetDataLen,
+												  att->attalignby,
+												  att->attlen,
+												  attrmiss[attnum].am_value);
 
 				targetDataLen = att_addlength_pointer(targetDataLen,
 													  att->attlen,
@@ -1395,19 +1393,19 @@ heap_deform_tuple(HeapTuple tuple, TupleDesc tupleDesc,
 			 * an aligned or unaligned value.
 			 */
 			if (!slow &&
-				off == att_align_nominal(off, thisatt->attalign))
+				off == att_nominal_alignby(off, thisatt->attalignby))
 				thisatt->attcacheoff = off;
 			else
 			{
-				off = att_align_pointer(off, thisatt->attalign, -1,
-										tp + off);
+				off = att_pointer_alignby(off, thisatt->attalignby, -1,
+										  tp + off);
 				slow = true;
 			}
 		}
 		else
 		{
-			/* not varlena, so safe to use att_align_nominal */
-			off = att_align_nominal(off, thisatt->attalign);
+			/* not varlena, so safe to use att_nominal_alignby */
+			off = att_nominal_alignby(off, thisatt->attalignby);
 
 			if (!slow)
 				thisatt->attcacheoff = off;
diff --git a/src/backend/access/common/indextuple.c b/src/backend/access/common/indextuple.c
index 37133ed7f8..3947b4a4d8 100644
--- a/src/backend/access/common/indextuple.c
+++ b/src/backend/access/common/indextuple.c
@@ -363,7 +363,7 @@ nocache_index_getattr(IndexTuple tup,
 			if (att->attlen <= 0)
 				break;
 
-			off = att_align_nominal(off, att->attalign);
+			off = att_nominal_alignby(off, att->attalignby);
 
 			att->attcacheoff = off;
 
@@ -412,19 +412,19 @@ nocache_index_getattr(IndexTuple tup,
 				 * either an aligned or unaligned value.
 				 */
 				if (usecache &&
-					off == att_align_nominal(off, att->attalign))
+					off == att_nominal_alignby(off, att->attalignby))
 					att->attcacheoff = off;
 				else
 				{
-					off = att_align_pointer(off, att->attalign, -1,
-											tp + off);
+					off = att_pointer_alignby(off, att->attalignby, -1,
+											  tp + off);
 					usecache = false;
 				}
 			}
 			else
 			{
-				/* not varlena, so safe to use att_align_nominal */
-				off = att_align_nominal(off, att->attalign);
+				/* not varlena, so safe to use att_nominal_alignby */
+				off = att_nominal_alignby(off, att->attalignby);
 
 				if (usecache)
 					att->attcacheoff = off;
@@ -513,19 +513,19 @@ index_deform_tuple_internal(TupleDesc tupleDescriptor,
 			 * an aligned or unaligned value.
 			 */
 			if (!slow &&
-				off == att_align_nominal(off, thisatt->attalign))
+				off == att_nominal_alignby(off, thisatt->attalignby))
 				thisatt->attcacheoff = off;
 			else
 			{
-				off = att_align_pointer(off, thisatt->attalign, -1,
-										tp + off);
+				off = att_pointer_alignby(off, thisatt->attalignby, -1,
+										  tp + off);
 				slow = true;
 			}
 		}
 		else
 		{
-			/* not varlena, so safe to use att_align_nominal */
-			off = att_align_nominal(off, thisatt->attalign);
+			/* not varlena, so safe to use att_nominal_alignby */
+			off = att_nominal_alignby(off, thisatt->attalignby);
 
 			if (!slow)
 				thisatt->attcacheoff = off;
diff --git a/src/backend/access/common/tupdesc.c b/src/backend/access/common/tupdesc.c
index 2f9a9a2636..78c26e769f 100644
--- a/src/backend/access/common/tupdesc.c
+++ b/src/backend/access/common/tupdesc.c
@@ -74,7 +74,25 @@ populate_compact_attribute(TupleDesc tupdesc, int i)
 	dst->attgenerated = (src->attgenerated != '\0');
 	dst->attnotnull = src->attnotnull;
 
-	dst->attalign = src->attalign;
+	switch (src->attalign)
+	{
+		case TYPALIGN_INT:
+			dst->attalignby = ALIGNOF_INT;
+			break;
+		case TYPALIGN_CHAR:
+			dst->attalignby = sizeof(char);
+			break;
+		case TYPALIGN_DOUBLE:
+			dst->attalignby = ALIGNOF_DOUBLE;
+			break;
+		case TYPALIGN_SHORT:
+			dst->attalignby = ALIGNOF_SHORT;
+			break;
+		default:
+			dst->attalignby = 0;
+			elog(ERROR, "invalid attalign value: %c", src->attalign);
+			break;
+	}
 }
 
 /*
diff --git a/src/backend/executor/execExprInterp.c b/src/backend/executor/execExprInterp.c
index 337291dee7..3b603d81a3 100644
--- a/src/backend/executor/execExprInterp.c
+++ b/src/backend/executor/execExprInterp.c
@@ -5086,7 +5086,7 @@ ExecEvalWholeRowVar(ExprState *state, ExprEvalStep *op, ExprContext *econtext)
 			if (slot->tts_isnull[i])
 				continue;		/* null is always okay */
 			if (vattr->attlen != sattr->attlen ||
-				vattr->attalign != sattr->attalign)
+				vattr->attalignby != sattr->attalignby)
 				ereport(ERROR,
 						(errcode(ERRCODE_DATATYPE_MISMATCH),
 						 errmsg("table row type and query-specified row type do not match"),
diff --git a/src/backend/executor/execTuples.c b/src/backend/executor/execTuples.c
index 875515777b..1f36d675a1 100644
--- a/src/backend/executor/execTuples.c
+++ b/src/backend/executor/execTuples.c
@@ -202,12 +202,12 @@ tts_virtual_materialize(TupleTableSlot *slot)
 			 * We want to flatten the expanded value so that the materialized
 			 * slot doesn't depend on it.
 			 */
-			sz = att_align_nominal(sz, att->attalign);
+			sz = att_nominal_alignby(sz, att->attalignby);
 			sz += EOH_get_flat_size(DatumGetEOHP(val));
 		}
 		else
 		{
-			sz = att_align_nominal(sz, att->attalign);
+			sz = att_nominal_alignby(sz, att->attalignby);
 			sz = att_addlength_datum(sz, att->attlen, val);
 		}
 	}
@@ -242,8 +242,7 @@ tts_virtual_materialize(TupleTableSlot *slot)
 			 */
 			ExpandedObjectHeader *eoh = DatumGetEOHP(val);
 
-			data = (char *) att_align_nominal(data,
-											  att->attalign);
+			data = (char *) att_nominal_alignby(data, att->attalignby);
 			data_length = EOH_get_flat_size(eoh);
 			EOH_flatten_into(eoh, data, data_length);
 
@@ -254,7 +253,7 @@ tts_virtual_materialize(TupleTableSlot *slot)
 		{
 			Size		data_length = 0;
 
-			data = (char *) att_align_nominal(data, att->attalign);
+			data = (char *) att_nominal_alignby(data, att->attalignby);
 			data_length = att_addlength_datum(data_length, att->attlen, val);
 
 			memcpy(data, DatumGetPointer(val), data_length);
@@ -1067,19 +1066,19 @@ slot_deform_heap_tuple(TupleTableSlot *slot, HeapTuple tuple, uint32 *offp,
 			 * an aligned or unaligned value.
 			 */
 			if (!slow &&
-				off == att_align_nominal(off, thisatt->attalign))
+				off == att_nominal_alignby(off, thisatt->attalignby))
 				thisatt->attcacheoff = off;
 			else
 			{
-				off = att_align_pointer(off, thisatt->attalign, -1,
-										tp + off);
+				off = att_pointer_alignby(off, thisatt->attalignby, -1,
+										  tp + off);
 				slow = true;
 			}
 		}
 		else
 		{
-			/* not varlena, so safe to use att_align_nominal */
-			off = att_align_nominal(off, thisatt->attalign);
+			/* not varlena, so safe to use att_nominal_alignby */
+			off = att_nominal_alignby(off, thisatt->attalignby);
 
 			if (!slow)
 				thisatt->attcacheoff = off;
diff --git a/src/backend/jit/llvm/llvmjit_deform.c b/src/backend/jit/llvm/llvmjit_deform.c
index f49e7bce7d..88ef2bb06c 100644
--- a/src/backend/jit/llvm/llvmjit_deform.c
+++ b/src/backend/jit/llvm/llvmjit_deform.c
@@ -395,7 +395,7 @@ slot_compile_deform(LLVMJitContext *context, TupleDesc desc,
 	{
 		CompactAttribute *att = TupleDescCompactAttr(desc, attnum);
 		LLVMValueRef v_incby;
-		int			alignto;
+		int			alignto = att->attalignby;
 		LLVMValueRef l_attno = l_int16_const(lc, attnum);
 		LLVMValueRef v_attdatap;
 		LLVMValueRef v_resultp;
@@ -494,21 +494,6 @@ slot_compile_deform(LLVMJitContext *context, TupleDesc desc,
 		}
 		LLVMPositionBuilderAtEnd(b, attcheckalignblocks[attnum]);
 
-		/* determine required alignment */
-		if (att->attalign == TYPALIGN_INT)
-			alignto = ALIGNOF_INT;
-		else if (att->attalign == TYPALIGN_CHAR)
-			alignto = 1;
-		else if (att->attalign == TYPALIGN_DOUBLE)
-			alignto = ALIGNOF_DOUBLE;
-		else if (att->attalign == TYPALIGN_SHORT)
-			alignto = ALIGNOF_SHORT;
-		else
-		{
-			elog(ERROR, "unknown alignment");
-			alignto = 0;
-		}
-
 		/* ------
 		 * Even if alignment is required, we can skip doing it if provably
 		 * unnecessary:
diff --git a/src/include/access/tupdesc.h b/src/include/access/tupdesc.h
index 5256e14268..9854b05c9d 100644
--- a/src/include/access/tupdesc.h
+++ b/src/include/access/tupdesc.h
@@ -62,7 +62,7 @@ typedef struct CompactAttribute
 	bool		attisdropped:1; /* as FormData_pg_attribute.attisdropped */
 	bool		attgenerated:1; /* FormData_pg_attribute.attgenerated != '\0' */
 	bool		attnotnull:1;	/* as FormData_pg_attribute.attnotnull */
-	char		attalign;		/* alignment requirement */
+	uint8		attalignby;		/* alignment requirement in bytes */
 } CompactAttribute;
 
 /*
diff --git a/src/include/access/tupmacs.h b/src/include/access/tupmacs.h
index 622adfa5f8..029324e147 100644
--- a/src/include/access/tupmacs.h
+++ b/src/include/access/tupmacs.h
@@ -91,6 +91,16 @@ fetch_att(const void *T, bool attbyval, int attlen)
 	att_align_nominal(cur_offset, attalign) \
 )
 
+/*
+ * Similar to att_align_datum, but accepts a number of bytes, typically from
+ * CompactAttribute.attalignby to align the Datum by.
+ */
+#define att_datum_alignby(cur_offset, attalignby, attlen, attdatum) \
+	( \
+	((attlen) == -1 && VARATT_IS_SHORT(DatumGetPointer(attdatum))) ? \
+	(uintptr_t) (cur_offset) : \
+	TYPEALIGN(attalignby, cur_offset))
+
 /*
  * att_align_pointer performs the same calculation as att_align_datum,
  * but is used when walking a tuple.  attptr is the current actual data
@@ -112,6 +122,12 @@ fetch_att(const void *T, bool attbyval, int attlen)
 	att_align_nominal(cur_offset, attalign) \
 )
 
+#define att_pointer_alignby(cur_offset, attalignby, attlen, attptr) \
+( \
+	((attlen) == -1 && VARATT_NOT_PAD_BYTE(attptr)) ? \
+	(uintptr_t) (cur_offset) : \
+	TYPEALIGN(attalignby, cur_offset))
+
 /*
  * att_align_nominal aligns the given offset as needed for a datum of alignment
  * requirement attalign, ignoring any consideration of packed varlena datums.
@@ -138,6 +154,13 @@ fetch_att(const void *T, bool attbyval, int attlen)
 	   ))) \
 )
 
+/*
+ * Similar to att_align_nominal, but accepts a number of bytes, typically from
+ * CompactAttribute.attalignby to align the offset by.
+ */
+#define att_nominal_alignby(cur_offset, attalignby) \
+	TYPEALIGN(attalignby, (uintptr_t) cur_offset)
+
 /*
  * att_addlength_datum increments the given offset by the space needed for
  * the given Datum variable.  attdatum is only accessed if we are dealing
-- 
2.34.1

