From 552680f18ac8b5db47be1167d789591994f6cf84 Mon Sep 17 00:00:00 2001
From: Julien Rouhaud <julien.rouhaud@free.fr>
Date: Tue, 14 Jun 2022 23:48:37 +0800
Subject: [PATCH v1 3/5] POC: Add infrastructure to specify catalog attributes
 logical position.

This introduces a new optional macro ATTNUM(attnum) which added to specify the
logical order of the attributes declared in a struct, if the regular struct
order isn't wanted.

Catalog.pm and genbki.pl will use that information to generate the data in the
correct oder.

Sanity check are performed to make sure that the declared ATTNUMs are a
gapeless sequence from 1 to the maximum number of attributes.

As an example, move relname near the end of pg_class struct and attname near
the end of pg_attribute.

Author: Julien Rouhaud
Discussion: FIXME
---
 src/backend/catalog/Catalog.pm     |  4 ++
 src/backend/catalog/genbki.pl      | 51 ++++++++++++++++++++++
 src/include/catalog/genbki.h       |  2 +
 src/include/catalog/pg_attribute.h | 55 +++++++++++------------
 src/include/catalog/pg_class.h     | 70 +++++++++++++++---------------
 5 files changed, 120 insertions(+), 62 deletions(-)

diff --git a/src/backend/catalog/Catalog.pm b/src/backend/catalog/Catalog.pm
index e91a8e10a8..7e08466534 100644
--- a/src/backend/catalog/Catalog.pm
+++ b/src/backend/catalog/Catalog.pm
@@ -253,6 +253,10 @@ sub ParseHeader
 							pk_cols  => 'oid'
 						  };
 					}
+					elsif ($attopt =~ /ATTNUM\((\w+)\)/)
+					{
+						$column{attnum} = $1;
+					}
 					else
 					{
 						die
diff --git a/src/backend/catalog/genbki.pl b/src/backend/catalog/genbki.pl
index cc23c5e909..b0b517ee95 100644
--- a/src/backend/catalog/genbki.pl
+++ b/src/backend/catalog/genbki.pl
@@ -847,6 +847,7 @@ sub gen_pg_attribute
 		# Generate entries for user attributes.
 		my $attphysnum          = 0;
 		my $priorfixedwidth = 1;
+		my %mappings;
 		foreach my $attr (@{ $table->{columns} })
 		{
 			$attphysnum++;
@@ -857,6 +858,31 @@ sub gen_pg_attribute
 
 			morph_row_for_pgattr(\%row, $schema, $attr, $priorfixedwidth);
 
+			# Sanity checks for the assigned attnum
+			if ($row{attnum} <= 0)
+			{
+				warn sprintf "Invalid attnum %d for attribute %s of table %s",
+				$row{attnum}, $row{attname}, $table_name;
+				$num_errors++;
+			}
+
+			if ($row{attnum} > $table->{columns})
+			{
+				warn sprintf "Invalid attnum %d for attribute %s of table %s",
+				$row{attnum}, $row{attname}, $table_name;
+				$num_errors++;
+			}
+
+			if (exists $mappings{$row{attnum}})
+			{
+				warn sprintf "Attnum %d used for columns %s and %s in table %s",
+				$row{attnum}, $mappings{$row{attnum}}, $row{attname},
+				$table_name;
+				$num_errors++;
+			}
+
+			$mappings{$row{attnum}} = $row{attname};
+
 			# Update $priorfixedwidth --- must match morph_row_for_pgattr
 			$priorfixedwidth &=
 			  ($row{attnotnull} eq 't'
@@ -872,6 +898,20 @@ sub gen_pg_attribute
 			  join(', ', grep { defined $_ } @row{@attnames});
 		}
 
+		# Complain if there's any "hole" in the attnum sequence.  Prior code
+		# should have already complained about inconsistencies, but this can
+		# save some time by explicitly mentionning which attnums are missing as
+		# a result.
+		for (my $i = 1; $i <= $attphysnum; $i++)
+		{
+			if (not exists($mappings{$i}))
+			{
+				warn sprintf "No attnum %d defined for table %s",
+				$i, $table_name;
+				$num_errors++;
+			}
+		}
+
 		# Generate entries for system attributes.
 		# We only need postgres.bki entries, not schemapg.h entries.
 		if ($table->{bootstrap})
@@ -913,6 +953,17 @@ sub morph_row_for_pgattr
 	my $attname = $attr->{name};
 	my $atttype = $attr->{type};
 
+	# use the attnum previously found in the headers if any, otherwise fallback
+	# to attphysnum
+	if (defined $attr->{attnum})
+	{
+		$row->{attnum} = $attr->{attnum};
+	}
+	else
+	{
+		$row->{attnum} = $row->{attphysnum};
+	}
+
 	$row->{attname} = $attname;
 
 	# Copy the type data from pg_type, and add some type-dependent items
diff --git a/src/include/catalog/genbki.h b/src/include/catalog/genbki.h
index 992b784236..229aa90f6a 100644
--- a/src/include/catalog/genbki.h
+++ b/src/include/catalog/genbki.h
@@ -45,6 +45,8 @@
  */
 #define BKI_LOOKUP(catalog)
 #define BKI_LOOKUP_OPT(catalog)
+/* Specifies the logical attribute number */
+#define ATTNUM(attnum)
 
 /*
  * These lines are processed by genbki.pl to create the statements
diff --git a/src/include/catalog/pg_attribute.h b/src/include/catalog/pg_attribute.h
index d2e8b5f765..1d0c43b82a 100644
--- a/src/include/catalog/pg_attribute.h
+++ b/src/include/catalog/pg_attribute.h
@@ -36,9 +36,8 @@
  */
 CATALOG(pg_attribute,1249,AttributeRelationId) BKI_BOOTSTRAP BKI_ROWTYPE_OID(75,AttributeRelation_Rowtype_Id) BKI_SCHEMA_MACRO
 {
-	Oid			attrelid BKI_LOOKUP(pg_class);	/* OID of relation containing
+	Oid			attrelid BKI_LOOKUP(pg_class) ATTNUM(1);	/* OID of relation containing
 												 * this attribute */
-	NameData	attname;		/* name of attribute */
 
 	/*
 	 * atttypid is the OID of the instance in Catalog Class pg_type that
@@ -50,7 +49,7 @@ CATALOG(pg_attribute,1249,AttributeRelationId) BKI_BOOTSTRAP BKI_ROWTYPE_OID(75,
 	 * attbyval, and attalign to still tell us how large the values in the
 	 * table are.
 	 */
-	Oid			atttypid BKI_LOOKUP_OPT(pg_type);
+	Oid			atttypid BKI_LOOKUP_OPT(pg_type) ATTNUM(3);
 
 	/*
 	 * attstattarget is the target number of statistics datapoints to collect
@@ -59,13 +58,13 @@ CATALOG(pg_attribute,1249,AttributeRelationId) BKI_BOOTSTRAP BKI_ROWTYPE_OID(75,
 	 * that no value has been explicitly set for this column, so ANALYZE
 	 * should use the default setting.
 	 */
-	int32		attstattarget BKI_DEFAULT(-1);
+	int32		attstattarget BKI_DEFAULT(-1) ATTNUM(4);
 
 	/*
 	 * attlen is a copy of the typlen field from pg_type for this attribute.
 	 * See atttypid comments above.
 	 */
-	int16		attlen;
+	int16		attlen ATTNUM(5);
 
 	/*
 	 * attphysnum is the "attribute number" for the attribute:	A value that
@@ -80,18 +79,18 @@ CATALOG(pg_attribute,1249,AttributeRelationId) BKI_BOOTSTRAP BKI_ROWTYPE_OID(75,
 	 *
 	 * Note that (attphysnum - 1) is often used as the index to an array.
 	 */
-	int16		attphysnum;
+	int16		attphysnum ATTNUM(6);
 
 	/* FIXME Same as attphysnum, but for the "logical" order, e.g. for a
 	 * star-expansion.
 	 */
-	int16		attnum;
+	int16		attnum ATTNUM(7);
 
 	/*
 	 * attndims is the declared number of dimensions, if an array type,
 	 * otherwise zero.
 	 */
-	int32		attndims;
+	int32		attndims ATTNUM(8);
 
 	/*
 	 * fastgetattr() uses attcacheoff to cache byte offsets of attributes in
@@ -100,7 +99,7 @@ CATALOG(pg_attribute,1249,AttributeRelationId) BKI_BOOTSTRAP BKI_ROWTYPE_OID(75,
 	 * descriptor, we may then update attcacheoff in the copies. This speeds
 	 * up the attribute walking process.
 	 */
-	int32		attcacheoff BKI_DEFAULT(-1);
+	int32		attcacheoff BKI_DEFAULT(-1) ATTNUM(9);
 
 	/*
 	 * atttypmod records type-specific data supplied at table creation time
@@ -108,19 +107,19 @@ CATALOG(pg_attribute,1249,AttributeRelationId) BKI_BOOTSTRAP BKI_ROWTYPE_OID(75,
 	 * type-specific input and output functions as the third argument. The
 	 * value will generally be -1 for types that do not need typmod.
 	 */
-	int32		atttypmod BKI_DEFAULT(-1);
+	int32		atttypmod BKI_DEFAULT(-1) ATTNUM(10);
 
 	/*
 	 * attbyval is a copy of the typbyval field from pg_type for this
 	 * attribute.  See atttypid comments above.
 	 */
-	bool		attbyval;
+	bool		attbyval ATTNUM(11);
 
 	/*
 	 * attalign is a copy of the typalign field from pg_type for this
 	 * attribute.  See atttypid comments above.
 	 */
-	char		attalign;
+	char		attalign ATTNUM(12);
 
 	/*----------
 	 * attstorage tells for VARLENA attributes, what the heap access
@@ -128,7 +127,7 @@ CATALOG(pg_attribute,1249,AttributeRelationId) BKI_BOOTSTRAP BKI_ROWTYPE_OID(75,
 	 * Possible values are as for pg_type.typstorage (see TYPSTORAGE macros).
 	 *----------
 	 */
-	char		attstorage;
+	char		attstorage ATTNUM(13);
 
 	/*
 	 * attcompression sets the current compression method of the attribute.
@@ -138,25 +137,25 @@ CATALOG(pg_attribute,1249,AttributeRelationId) BKI_BOOTSTRAP BKI_ROWTYPE_OID(75,
 	 * However, this field is ignored whenever attstorage does not allow
 	 * compression.
 	 */
-	char		attcompression BKI_DEFAULT('\0');
+	char		attcompression BKI_DEFAULT('\0') ATTNUM(14);
 
 	/* This flag represents the "NOT NULL" constraint */
-	bool		attnotnull;
+	bool		attnotnull ATTNUM(15);
 
 	/* Has DEFAULT value or not */
-	bool		atthasdef BKI_DEFAULT(f);
+	bool		atthasdef BKI_DEFAULT(f) ATTNUM(16);
 
 	/* Has a missing value or not */
-	bool		atthasmissing BKI_DEFAULT(f);
+	bool		atthasmissing BKI_DEFAULT(f) ATTNUM(17);
 
 	/* One of the ATTRIBUTE_IDENTITY_* constants below, or '\0' */
-	char		attidentity BKI_DEFAULT('\0');
+	char		attidentity BKI_DEFAULT('\0') ATTNUM(18);
 
 	/* One of the ATTRIBUTE_GENERATED_* constants below, or '\0' */
-	char		attgenerated BKI_DEFAULT('\0');
+	char		attgenerated BKI_DEFAULT('\0') ATTNUM(19);
 
 	/* Is dropped (ie, logically invisible) or not */
-	bool		attisdropped BKI_DEFAULT(f);
+	bool		attisdropped BKI_DEFAULT(f) ATTNUM(20);
 
 	/*
 	 * This flag specifies whether this column has ever had a local
@@ -167,31 +166,33 @@ CATALOG(pg_attribute,1249,AttributeRelationId) BKI_BOOTSTRAP BKI_ROWTYPE_OID(75,
 	 * not dropped by a parent's DROP COLUMN even if this causes the column's
 	 * attinhcount to become zero.
 	 */
-	bool		attislocal BKI_DEFAULT(t);
+	bool		attislocal BKI_DEFAULT(t) ATTNUM(21);
 
 	/* Number of times inherited from direct parent relation(s) */
-	int32		attinhcount BKI_DEFAULT(0);
+	int32		attinhcount BKI_DEFAULT(0) ATTNUM(22);
+
+	NameData	attname ATTNUM(2);		/* name of attribute */
 
 	/* attribute's collation, if any */
-	Oid			attcollation BKI_LOOKUP_OPT(pg_collation);
+	Oid			attcollation BKI_LOOKUP_OPT(pg_collation) ATTNUM(23);
 
 #ifdef CATALOG_VARLEN			/* variable-length fields start here */
 	/* NOTE: The following fields are not present in tuple descriptors. */
 
 	/* Column-level access permissions */
-	aclitem		attacl[1] BKI_DEFAULT(_null_);
+	aclitem		attacl[1] BKI_DEFAULT(_null_) ATTNUM(24);
 
 	/* Column-level options */
-	text		attoptions[1] BKI_DEFAULT(_null_);
+	text		attoptions[1] BKI_DEFAULT(_null_) ATTNUM(25);
 
 	/* Column-level FDW options */
-	text		attfdwoptions[1] BKI_DEFAULT(_null_);
+	text		attfdwoptions[1] BKI_DEFAULT(_null_) ATTNUM(26);
 
 	/*
 	 * Missing value for added columns. This is a one element array which lets
 	 * us store a value of the attribute type here.
 	 */
-	anyarray	attmissingval BKI_DEFAULT(_null_);
+	anyarray	attmissingval BKI_DEFAULT(_null_) ATTNUM(27);
 #endif
 } FormData_pg_attribute;
 
diff --git a/src/include/catalog/pg_class.h b/src/include/catalog/pg_class.h
index e1f4eefa22..2911903cb9 100644
--- a/src/include/catalog/pg_class.h
+++ b/src/include/catalog/pg_class.h
@@ -32,59 +32,56 @@
 CATALOG(pg_class,1259,RelationRelationId) BKI_BOOTSTRAP BKI_ROWTYPE_OID(83,RelationRelation_Rowtype_Id) BKI_SCHEMA_MACRO
 {
 	/* oid */
-	Oid			oid;
-
-	/* class name */
-	NameData	relname;
+	Oid			oid ATTNUM(1);
 
 	/* OID of namespace containing this class */
-	Oid			relnamespace BKI_DEFAULT(pg_catalog) BKI_LOOKUP(pg_namespace);
+	Oid			relnamespace BKI_DEFAULT(pg_catalog) BKI_LOOKUP(pg_namespace) ATTNUM(3);
 
 	/* OID of entry in pg_type for relation's implicit row type, if any */
-	Oid			reltype BKI_LOOKUP_OPT(pg_type);
+	Oid			reltype BKI_LOOKUP_OPT(pg_type) ATTNUM(4);
 
 	/* OID of entry in pg_type for underlying composite type, if any */
-	Oid			reloftype BKI_DEFAULT(0) BKI_LOOKUP_OPT(pg_type);
+	Oid			reloftype BKI_DEFAULT(0) BKI_LOOKUP_OPT(pg_type) ATTNUM(5);
 
 	/* class owner */
-	Oid			relowner BKI_DEFAULT(POSTGRES) BKI_LOOKUP(pg_authid);
+	Oid			relowner BKI_DEFAULT(POSTGRES) BKI_LOOKUP(pg_authid) ATTNUM(6);
 
 	/* access method; 0 if not a table / index */
-	Oid			relam BKI_DEFAULT(heap) BKI_LOOKUP_OPT(pg_am);
+	Oid			relam BKI_DEFAULT(heap) BKI_LOOKUP_OPT(pg_am) ATTNUM(7);
 
 	/* identifier of physical storage file */
 	/* relfilenode == 0 means it is a "mapped" relation, see relmapper.c */
-	Oid			relfilenode BKI_DEFAULT(0);
+	Oid			relfilenode BKI_DEFAULT(0) ATTNUM(8);
 
 	/* identifier of table space for relation (0 means default for database) */
-	Oid			reltablespace BKI_DEFAULT(0) BKI_LOOKUP_OPT(pg_tablespace);
+	Oid			reltablespace BKI_DEFAULT(0) BKI_LOOKUP_OPT(pg_tablespace) ATTNUM(9);
 
 	/* # of blocks (not always up-to-date) */
-	int32		relpages BKI_DEFAULT(0);
+	int32		relpages BKI_DEFAULT(0) ATTNUM(10);
 
 	/* # of tuples (not always up-to-date; -1 means "unknown") */
-	float4		reltuples BKI_DEFAULT(-1);
+	float4		reltuples BKI_DEFAULT(-1) ATTNUM(11);
 
 	/* # of all-visible blocks (not always up-to-date) */
-	int32		relallvisible BKI_DEFAULT(0);
+	int32		relallvisible BKI_DEFAULT(0) ATTNUM(12);
 
 	/* OID of toast table; 0 if none */
-	Oid			reltoastrelid BKI_DEFAULT(0) BKI_LOOKUP_OPT(pg_class);
+	Oid			reltoastrelid BKI_DEFAULT(0) BKI_LOOKUP_OPT(pg_class) ATTNUM(13);
 
 	/* T if has (or has had) any indexes */
-	bool		relhasindex BKI_DEFAULT(f);
+	bool		relhasindex BKI_DEFAULT(f) ATTNUM(14);
 
 	/* T if shared across databases */
-	bool		relisshared BKI_DEFAULT(f);
+	bool		relisshared BKI_DEFAULT(f) ATTNUM(15);
 
 	/* see RELPERSISTENCE_xxx constants below */
-	char		relpersistence BKI_DEFAULT(p);
+	char		relpersistence BKI_DEFAULT(p) ATTNUM(16);
 
 	/* see RELKIND_xxx constants below */
-	char		relkind BKI_DEFAULT(r);
+	char		relkind BKI_DEFAULT(r) ATTNUM(17);
 
 	/* number of user attributes */
-	int16		relnatts BKI_DEFAULT(0);	/* genbki.pl will fill this in */
+	int16		relnatts BKI_DEFAULT(0) ATTNUM(18);	/* genbki.pl will fill this in */
 
 	/*
 	 * Class pg_attribute must contain exactly "relnatts" user attributes
@@ -93,51 +90,54 @@ CATALOG(pg_class,1259,RelationRelationId) BKI_BOOTSTRAP BKI_ROWTYPE_OID(83,Relat
 	 */
 
 	/* # of CHECK constraints for class */
-	int16		relchecks BKI_DEFAULT(0);
+	int16		relchecks BKI_DEFAULT(0) ATTNUM(19);
 
 	/* has (or has had) any rules */
-	bool		relhasrules BKI_DEFAULT(f);
+	bool		relhasrules BKI_DEFAULT(f) ATTNUM(20);
 
 	/* has (or has had) any TRIGGERs */
-	bool		relhastriggers BKI_DEFAULT(f);
+	bool		relhastriggers BKI_DEFAULT(f) ATTNUM(21);
 
 	/* has (or has had) child tables or indexes */
-	bool		relhassubclass BKI_DEFAULT(f);
+	bool		relhassubclass BKI_DEFAULT(f) ATTNUM(22);
 
 	/* row security is enabled or not */
-	bool		relrowsecurity BKI_DEFAULT(f);
+	bool		relrowsecurity BKI_DEFAULT(f) ATTNUM(23);
 
 	/* row security forced for owners or not */
-	bool		relforcerowsecurity BKI_DEFAULT(f);
+	bool		relforcerowsecurity BKI_DEFAULT(f) ATTNUM(24);
 
 	/* matview currently holds query results */
-	bool		relispopulated BKI_DEFAULT(t);
+	bool		relispopulated BKI_DEFAULT(t) ATTNUM(25);
 
 	/* see REPLICA_IDENTITY_xxx constants */
-	char		relreplident BKI_DEFAULT(n);
+	char		relreplident BKI_DEFAULT(n) ATTNUM(26);
 
 	/* is relation a partition? */
-	bool		relispartition BKI_DEFAULT(f);
+	bool		relispartition BKI_DEFAULT(f) ATTNUM(27);
 
 	/* link to original rel during table rewrite; otherwise 0 */
-	Oid			relrewrite BKI_DEFAULT(0) BKI_LOOKUP_OPT(pg_class);
+	Oid			relrewrite BKI_DEFAULT(0) BKI_LOOKUP_OPT(pg_class) ATTNUM(28);
 
 	/* all Xids < this are frozen in this rel */
-	TransactionId relfrozenxid BKI_DEFAULT(3);	/* FirstNormalTransactionId */
+	TransactionId relfrozenxid BKI_DEFAULT(3) ATTNUM(29);	/* FirstNormalTransactionId */
+
+	/* class name */
+	NameData	relname ATTNUM(2);
 
 	/* all multixacts in this rel are >= this; it is really a MultiXactId */
-	TransactionId relminmxid BKI_DEFAULT(1);	/* FirstMultiXactId */
+	TransactionId relminmxid BKI_DEFAULT(1) ATTNUM(30);	/* FirstMultiXactId */
 
 #ifdef CATALOG_VARLEN			/* variable-length fields start here */
 	/* NOTE: These fields are not present in a relcache entry's rd_rel field. */
 	/* access permissions */
-	aclitem		relacl[1] BKI_DEFAULT(_null_);
+	aclitem		relacl[1] BKI_DEFAULT(_null_) ATTNUM(31);
 
 	/* access-method-specific options */
-	text		reloptions[1] BKI_DEFAULT(_null_);
+	text		reloptions[1] BKI_DEFAULT(_null_) ATTNUM(32);
 
 	/* partition bound node tree */
-	pg_node_tree relpartbound BKI_DEFAULT(_null_);
+	pg_node_tree relpartbound BKI_DEFAULT(_null_) ATTNUM(33);
 #endif
 } FormData_pg_class;
 
-- 
2.33.1

