Hi Peter,

On Mon, Jan 22, 2024 at 6:13 PM Peter Eisentraut <pe...@eisentraut.org> wrote:
>
> On 06.12.23 09:23, Peter Eisentraut wrote:
> > The (now) second patch is also still of interest to me, but I have since
> > noticed that I think [0] should be fixed first, but to make that fix
> > simpler, I would like the first patch from here.
> >
> > [0]:
> > https://www.postgresql.org/message-id/flat/24656cec-d6ef-4d15-8b5b-e8dfc9c833a7%40eisentraut.org
>
> The remaining patch in this series needed a rebase and adjustment.
>
> The above precondition still applies.

While working on identity support and now while looking at the
compression problem you referred to, I found MergeAttribute() to be
hard to read. It's hard to follow high level logic in that function
since the function is not modular. I took a stab at modularising a
part of it. Attached is the resulting patch series.

0001 is your patch as is
0002 is pgindent fix and also fixing what I think is a typo/thinko
from 0001. If you are fine with the changes, 0002 should be merged
into 0003.
0003 separates the part of code merging a child attribute to the
corresponding inherited attribute into its own function.
0004 does the same for code merging inherited attributes incrementally.

I have kept 0003 and 0004 separate in case we pick one and not the
other. But they can be committed as a single commit.

The two new functions have some common code and some differences.
Common code is not surprising since merging attributes whether from
child definition or from inheritance parents will have common rules.
Differences are expected in cases when child attributes need to be
treated differently. But the differences may point us to some
yet-unknown bugs; compression being one of those differences. I think
the next step should combine these functions into one so that all the
logic to merge one attribute is at one place. I haven't attempted it;
wanted to propose the idea first.

I can see that this moduralization will lead to another and we will be
able to reduce MergeAttribute() to a set of function calls reflecting
its high level logic and push the detailed implementation into minion
functions like this.

-- 
Best Wishes,
Ashutosh Bapat
From 2553d082b16db2c54f2e1e4a66f1a57ecabf3c1e Mon Sep 17 00:00:00 2001
From: Ashutosh Bapat <ashutosh.ba...@enterprisedb.com>
Date: Wed, 24 Jan 2024 11:15:15 +0530
Subject: [PATCH 2/4] Mark NULL constraint in merged definition instead of new
 definition

This is a typo/thinko in previous commit.

Also fix pgindent issue.

Ashutosh Bapat
---
 src/backend/commands/tablecmds.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 04e674bbda..cde524083e 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -2726,8 +2726,8 @@ MergeAttributes(List *columns, const List *supers, char relpersistence,
 
 			/*
 			 * Regular inheritance children are independent enough not to
-			 * inherit identity columns.  But partitions are integral part
-			 * of a partitioned table and inherit identity column.
+			 * inherit identity columns.  But partitions are integral part of
+			 * a partitioned table and inherit identity column.
 			 */
 			if (is_partition)
 				newdef->identity = attribute->attidentity;
@@ -2844,7 +2844,7 @@ MergeAttributes(List *columns, const List *supers, char relpersistence,
 				if (bms_is_member(parent_attno, nncols) ||
 					bms_is_member(parent_attno - FirstLowInvalidHeapAttributeNumber,
 								  pkattrs))
-					newdef->is_not_null = true;
+					prevdef->is_not_null = true;
 
 				/*
 				 * Check for GENERATED conflicts
-- 
2.25.1

From 092828a7d7274b48bf33c88863a97585bff80ebb Mon Sep 17 00:00:00 2001
From: Ashutosh Bapat <ashutosh.ba...@enterprisedb.com>
Date: Tue, 23 Jan 2024 17:00:43 +0530
Subject: [PATCH 4/4] Separate function to merge next parent attribute

Partition inherit from only a single parent.  The logic to merge an
attribute from the next parent into the corresponding attribute
inherited from previous parents in MergeAttribute() is only applicable
to regular inheritance children.  This code is isolated enough that it
can be separate into a function by itself.  This separation makes
MergeAttributes() more readable making it easy to follow high level
logic without getting entangled into details.

This separation revealed that the code handling NOT NULL constraints is
duplicated in blocks merging the attribute definition incrementally.
Deduplicate that code.

Author: Ashutosh Bapat
---
 src/backend/commands/tablecmds.c | 352 ++++++++++++++++---------------
 1 file changed, 178 insertions(+), 174 deletions(-)

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 843cc3bff6..3f0066b435 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -360,6 +360,8 @@ static List *MergeAttributes(List *columns, const List *supers, char relpersiste
 							 List **supnotnulls);
 static List *MergeCheckConstraint(List *constraints, const char *name, Node *expr);
 static bool MergeChildAttribute(ColumnDef *newdef, int newcol_attno, List *inh_columns);
+static ColumnDef *MergeInheritedAttribute(ColumnDef *newdef, int parent_attno,
+										  List *inh_columns, AttrMap *newattmap);
 static void MergeAttributesIntoExisting(Relation child_rel, Relation parent_rel, bool ispartition);
 static void MergeConstraintsIntoExisting(Relation child_rel, Relation parent_rel);
 static void StoreCatalogInheritance(Oid relationId, List *supers,
@@ -2704,9 +2706,8 @@ MergeAttributes(List *columns, const List *supers, char relpersistence,
 			Form_pg_attribute attribute = TupleDescAttr(tupleDesc,
 														parent_attno - 1);
 			char	   *attributeName = NameStr(attribute->attname);
-			int			exist_attno;
 			ColumnDef  *newdef;
-			ColumnDef  *savedef;
+			ColumnDef  *mergedef;
 
 			/*
 			 * Ignore dropped columns in the parent.
@@ -2727,194 +2728,64 @@ MergeAttributes(List *columns, const List *supers, char relpersistence,
 
 			/*
 			 * Regular inheritance children are independent enough not to
-			 * inherit identity columns.  But partitions are integral part of
-			 * a partitioned table and inherit identity column.
+			 * inherit identity columns. But partitions are integral part of a
+			 * partitioned table and inherit identity column.
 			 */
 			if (is_partition)
 				newdef->identity = attribute->attidentity;
 
+			mergedef = MergeInheritedAttribute(newdef, parent_attno,
+											   inh_columns, newattmap);
+
 			/*
-			 * Does it match some previously considered column from another
-			 * parent?
+			 * Partitions have only one parent, so conflict should never
+			 * occur.
 			 */
-			exist_attno = findAttrByName(attributeName, inh_columns);
-			if (exist_attno > 0)
-			{
-				ColumnDef  *prevdef;
-				Oid			prevtypeid,
-							newtypeid;
-				int32		prevtypmod,
-							newtypmod;
-				Oid			prevcollid,
-							newcollid;
-
-				/*
-				 * Partitions have only one parent and have no column
-				 * definitions of their own, so conflict should never occur.
-				 */
-				Assert(!is_partition);
-
-				/*
-				 * Yes, try to merge the two column definitions.
-				 */
-				ereport(NOTICE,
-						(errmsg("merging multiple inherited definitions of column \"%s\"",
-								attributeName)));
-				prevdef = list_nth_node(ColumnDef, inh_columns, exist_attno - 1);
-
-				/*
-				 * Must have the same type and typmod
-				 */
-				typenameTypeIdAndMod(NULL, prevdef->typeName, &prevtypeid, &prevtypmod);
-				typenameTypeIdAndMod(NULL, newdef->typeName, &newtypeid, &newtypmod);
-				if (prevtypeid != newtypeid || prevtypmod != newtypmod)
-					ereport(ERROR,
-							(errcode(ERRCODE_DATATYPE_MISMATCH),
-							 errmsg("inherited column \"%s\" has a type conflict",
-									attributeName),
-							 errdetail("%s versus %s",
-									   format_type_with_typemod(prevtypeid, prevtypmod),
-									   format_type_with_typemod(newtypeid, newtypmod))));
-
-				/*
-				 * Must have the same collation
-				 */
-				prevcollid = GetColumnDefCollation(NULL, prevdef, prevtypeid);
-				newcollid = GetColumnDefCollation(NULL, newdef, newtypeid);
-				if (prevcollid != newcollid)
-					ereport(ERROR,
-							(errcode(ERRCODE_COLLATION_MISMATCH),
-							 errmsg("inherited column \"%s\" has a collation conflict",
-									attributeName),
-							 errdetail("\"%s\" versus \"%s\"",
-									   get_collation_name(prevcollid),
-									   get_collation_name(newcollid))));
+			Assert(mergedef == NULL || !is_partition);
 
-				/*
-				 * Copy/check storage parameter
-				 */
-				if (prevdef->storage == 0)
-					prevdef->storage = newdef->storage;
-				else if (prevdef->storage != newdef->storage)
-					ereport(ERROR,
-							(errcode(ERRCODE_DATATYPE_MISMATCH),
-							 errmsg("inherited column \"%s\" has a storage parameter conflict",
-									attributeName),
-							 errdetail("%s versus %s",
-									   storage_name(prevdef->storage),
-									   storage_name(newdef->storage))));
-
-				/*
-				 * Copy/check compression parameter
-				 */
-				if (prevdef->compression == NULL)
-					prevdef->compression = newdef->compression;
-				else if (strcmp(prevdef->compression, newdef->compression) != 0)
-					ereport(ERROR,
-							(errcode(ERRCODE_DATATYPE_MISMATCH),
-							 errmsg("column \"%s\" has a compression method conflict",
-									attributeName),
-							 errdetail("%s versus %s", prevdef->compression, newdef->compression)));
-
-				/*
-				 * In regular inheritance, columns in the parent's primary key
-				 * get an extra not-null constraint.
-				 */
-				if (bms_is_member(parent_attno - FirstLowInvalidHeapAttributeNumber,
-								  pkattrs))
-				{
-					CookedConstraint *nn;
-
-					nn = palloc(sizeof(CookedConstraint));
-					nn->contype = CONSTR_NOTNULL;
-					nn->conoid = InvalidOid;
-					nn->name = NULL;
-					nn->attnum = exist_attno;
-					nn->expr = NULL;
-					nn->skip_validation = false;
-					nn->is_local = false;
-					nn->inhcount = 1;
-					nn->is_no_inherit = false;
-
-					nnconstraints = lappend(nnconstraints, nn);
-				}
-
-				/*
-				 * mark attnotnull if parent has it and it's not NO INHERIT
-				 */
-				if (bms_is_member(parent_attno, nncols) ||
-					bms_is_member(parent_attno - FirstLowInvalidHeapAttributeNumber,
-								  pkattrs))
-					prevdef->is_not_null = true;
-
-				/*
-				 * Check for GENERATED conflicts
-				 */
-				if (prevdef->generated != newdef->generated)
-					ereport(ERROR,
-							(errcode(ERRCODE_DATATYPE_MISMATCH),
-							 errmsg("inherited column \"%s\" has a generation conflict",
-									attributeName)));
-
-				/*
-				 * Default and other constraints are handled below
-				 */
-
-				prevdef->inhcount++;
-				if (prevdef->inhcount < 0)
-					ereport(ERROR,
-							errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
-							errmsg("too many inheritance parents"));
-
-				newattmap->attnums[parent_attno - 1] = exist_attno;
-
-				/* remember for default processing below */
-				savedef = prevdef;
-			}
-			else
+			if (mergedef == NULL)
 			{
 				/*
 				 * No, create a new inherited column
 				 */
 				newdef->inhcount = 1;
 				newdef->is_local = false;
-				/* mark attnotnull if parent has it and it's not NO INHERIT */
-				if (bms_is_member(parent_attno, nncols) ||
-					bms_is_member(parent_attno - FirstLowInvalidHeapAttributeNumber,
-								  pkattrs))
-					newdef->is_not_null = true;
 				inh_columns = lappend(inh_columns, newdef);
 				newattmap->attnums[parent_attno - 1] = ++child_attno;
 
-				/*
-				 * In regular inheritance, columns in the parent's primary key
-				 * get an extra not-null constraint.  Partitioning doesn't
-				 * need this, because the PK itself is going to be cloned to
-				 * the partition.
-				 */
-				if (!is_partition &&
-					bms_is_member(parent_attno -
-								  FirstLowInvalidHeapAttributeNumber,
-								  pkattrs))
-				{
-					CookedConstraint *nn;
-
-					nn = palloc(sizeof(CookedConstraint));
-					nn->contype = CONSTR_NOTNULL;
-					nn->conoid = InvalidOid;
-					nn->name = NULL;
-					nn->attnum = newattmap->attnums[parent_attno - 1];
-					nn->expr = NULL;
-					nn->skip_validation = false;
-					nn->is_local = false;
-					nn->inhcount = 1;
-					nn->is_no_inherit = false;
-
-					nnconstraints = lappend(nnconstraints, nn);
-				}
+				mergedef = newdef;
+			}
+
+			/* mark attnotnull if parent has it and it's not NO INHERIT */
+			if (bms_is_member(parent_attno, nncols) ||
+				bms_is_member(parent_attno - FirstLowInvalidHeapAttributeNumber,
+							  pkattrs))
+				mergedef->is_not_null = true;
 
-				/* remember for default processing below */
-				savedef = newdef;
+			/*
+			 * In regular inheritance, columns in the parent's primary key get
+			 * an extra not-null constraint.  Partitioning doesn't need this,
+			 * because the PK itself is going to be cloned to the partition.
+			 */
+			if (!is_partition &&
+				bms_is_member(parent_attno -
+							  FirstLowInvalidHeapAttributeNumber,
+							  pkattrs))
+			{
+				CookedConstraint *nn;
+
+				nn = palloc(sizeof(CookedConstraint));
+				nn->contype = CONSTR_NOTNULL;
+				nn->conoid = InvalidOid;
+				nn->name = NULL;
+				nn->attnum = newattmap->attnums[parent_attno - 1];
+				nn->expr = NULL;
+				nn->skip_validation = false;
+				nn->is_local = false;
+				nn->inhcount = 1;
+				nn->is_no_inherit = false;
+
+				nnconstraints = lappend(nnconstraints, nn);
 			}
 
 			/*
@@ -2936,7 +2807,7 @@ MergeAttributes(List *columns, const List *supers, char relpersistence,
 				 * all the inherited default expressions for the moment.
 				 */
 				inherited_defaults = lappend(inherited_defaults, this_default);
-				cols_with_defaults = lappend(cols_with_defaults, savedef);
+				cols_with_defaults = lappend(cols_with_defaults, mergedef);
 			}
 		}
 
@@ -3451,6 +3322,139 @@ MergeChildAttribute(ColumnDef *newdef, int newcol_attno, List *inh_columns)
 	return true;
 }
 
+/*
+ * MergeInheritedAttribute
+ *		Merge given parent attribute definition into any attribute, with
+ *  the same name, inherited from the previous parents.
+ *
+ * Input arguments:
+ * 'newdef' is the new parent column/attribute definition to be merged.
+ * 'parent_attno' is the attribute number in parent table's schema definition
+ * 'inh_columns' is the list of previously inherited ColumnDefs.
+ * 'newattmap' is attribute map.
+ *
+ * Return value:
+ * True if the given attribute is merged into a previously inherited attribute
+ * with the same name. False if no matching inherited attribute is found. When
+ * returning true, matching ColumnDef in 'inh_columns' list is modified.
+ * 'newattmap' is updated with matching inherited attribute's position. New
+ * attribute's ColumnDef remains unchanged.
+ *
+ * Notes:
+ *  (1) The attribute is merged according to the rules laid out in the prologue
+ *  of MergeAttributes().
+ *  (2) If matching inherited attribute exists but the new attribute can not
+ *  be merged into it, the function throws respective errors.
+ *  (3) A partition inherits from only a single parent. Hence this
+ *  function is applicable only to a regular inheritance.
+ */
+static ColumnDef *
+MergeInheritedAttribute(ColumnDef *newdef, int parent_attno, List *inh_columns,
+						AttrMap *newattmap)
+{
+	char	   *attributeName = newdef->colname;
+	int			exist_attno;
+	ColumnDef  *prevdef;
+	Oid			prevtypeid,
+				newtypeid;
+	int32		prevtypmod,
+				newtypmod;
+	Oid			prevcollid,
+				newcollid;
+
+	/*
+	 * Does it match some previously considered column from another parent?
+	 */
+	exist_attno = findAttrByName(attributeName, inh_columns);
+	if (exist_attno <= 0)
+		return NULL;
+
+	/*
+	 * Yes, try to merge the two column definitions.
+	 */
+	ereport(NOTICE,
+			(errmsg("merging multiple inherited definitions of column \"%s\"",
+					attributeName)));
+	prevdef = list_nth_node(ColumnDef, inh_columns, exist_attno - 1);
+
+	/*
+	 * Must have the same type and typmod
+	 */
+	typenameTypeIdAndMod(NULL, prevdef->typeName, &prevtypeid, &prevtypmod);
+	typenameTypeIdAndMod(NULL, newdef->typeName, &newtypeid, &newtypmod);
+	if (prevtypeid != newtypeid || prevtypmod != newtypmod)
+		ereport(ERROR,
+				(errcode(ERRCODE_DATATYPE_MISMATCH),
+				 errmsg("inherited column \"%s\" has a type conflict",
+						attributeName),
+				 errdetail("%s versus %s",
+						   format_type_with_typemod(prevtypeid, prevtypmod),
+						   format_type_with_typemod(newtypeid, newtypmod))));
+
+	/*
+	 * Must have the same collation
+	 */
+	prevcollid = GetColumnDefCollation(NULL, prevdef, prevtypeid);
+	newcollid = GetColumnDefCollation(NULL, newdef, newtypeid);
+	if (prevcollid != newcollid)
+		ereport(ERROR,
+				(errcode(ERRCODE_COLLATION_MISMATCH),
+				 errmsg("inherited column \"%s\" has a collation conflict",
+						attributeName),
+				 errdetail("\"%s\" versus \"%s\"",
+						   get_collation_name(prevcollid),
+						   get_collation_name(newcollid))));
+
+	/*
+	 * Copy/check storage parameter
+	 */
+	if (prevdef->storage == 0)
+		prevdef->storage = newdef->storage;
+	else if (prevdef->storage != newdef->storage)
+		ereport(ERROR,
+				(errcode(ERRCODE_DATATYPE_MISMATCH),
+				 errmsg("inherited column \"%s\" has a storage parameter conflict",
+						attributeName),
+				 errdetail("%s versus %s",
+						   storage_name(prevdef->storage),
+						   storage_name(newdef->storage))));
+
+	/*
+	 * Copy/check compression parameter
+	 */
+	if (prevdef->compression == NULL)
+		prevdef->compression = newdef->compression;
+	else if (strcmp(prevdef->compression, newdef->compression) != 0)
+		ereport(ERROR,
+				(errcode(ERRCODE_DATATYPE_MISMATCH),
+				 errmsg("column \"%s\" has a compression method conflict",
+						attributeName),
+				 errdetail("%s versus %s", prevdef->compression, newdef->compression)));
+
+	/*
+	 * Check for GENERATED conflicts
+	 */
+	if (prevdef->generated != newdef->generated)
+		ereport(ERROR,
+				(errcode(ERRCODE_DATATYPE_MISMATCH),
+				 errmsg("inherited column \"%s\" has a generation conflict",
+						attributeName)));
+
+	/*
+	 * Default and other constraints are handled by the caller.
+	 */
+
+	prevdef->inhcount++;
+	if (prevdef->inhcount < 0)
+		ereport(ERROR,
+				errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
+				errmsg("too many inheritance parents"));
+
+	newattmap->attnums[parent_attno - 1] = exist_attno;
+
+	return prevdef;
+}
+
 /*
  * StoreCatalogInheritance
  *		Updates the system catalogs with proper inheritance information.
-- 
2.25.1

From 6d6c85d9f08a1d631e4429e9d2c8b3e97b3e1337 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <pe...@eisentraut.org>
Date: Mon, 22 Jan 2024 13:23:43 +0100
Subject: [PATCH 1/4] MergeAttributes: convert pg_attribute back to ColumnDef
 before comparing

MergeAttributes() has a loop to merge multiple inheritance parents
into a column column definition.  The merged-so-far definition is
stored in a ColumnDef node.  If we have to merge columns from multiple
inheritance parents (if the name matches), then we have to check
whether various column properties (type, collation, etc.) match.  The
current code extracts the pg_attribute value of the
currently-considered inheritance parent and compares it against the
merged-so-far ColumnDef value.  If the currently considered column
doesn't match any previously inherited column, we make a new ColumnDef
node from the pg_attribute information and add it to the column list.

This patch rearranges this so that we create the ColumnDef node first
in either case.  Then the code that checks the column properties for
compatibility compares ColumnDef against ColumnDef (instead of
ColumnDef against pg_attribute).  This makes the code more symmetric
and easier to follow.  Also, later in MergeAttributes(), there is a
similar but separate loop that merges the new local column definition
with the combination of the inheritance parents established in the
first loop.  That comparison is already ColumnDef-vs-ColumnDef.  With
this change, both of these can use similar-looking logic.  (A future
project might be to extract these two sets of code into a common
routine that encodes all the knowledge of whether two column
definitions are compatible.  But this isn't currently straightforward
because we want to give different error messages in the two cases.)
Furthermore, by avoiding the use of Form_pg_attribute here, we make it
easier to make changes in the pg_attribute layout without having to
worry about the local needs of tablecmds.c.

Because MergeAttributes() is hugely long, it's sometimes hard to know
where in the function you are currently looking.  To help with that, I
also renamed some variables to make it clearer where you are currently
looking.  The first look is "prev" vs. "new", the second loop is "inh"
vs. "new".

Discussion: https://www.postgresql.org/message-id/flat/52a125e4-ff9a-95f5-9f61-b87cf447e...@eisentraut.org
---
 src/backend/commands/tablecmds.c | 198 ++++++++++++++++---------------
 1 file changed, 102 insertions(+), 96 deletions(-)

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 2a56a4357c..04e674bbda 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -2704,7 +2704,8 @@ MergeAttributes(List *columns, const List *supers, char relpersistence,
 														parent_attno - 1);
 			char	   *attributeName = NameStr(attribute->attname);
 			int			exist_attno;
-			ColumnDef  *def;
+			ColumnDef  *newdef;
+			ColumnDef  *savedef;
 
 			/*
 			 * Ignore dropped columns in the parent.
@@ -2713,14 +2714,38 @@ MergeAttributes(List *columns, const List *supers, char relpersistence,
 				continue;		/* leave newattmap->attnums entry as zero */
 
 			/*
-			 * Does it conflict with some previously inherited column?
+			 * Create new column definition
+			 */
+			newdef = makeColumnDef(attributeName, attribute->atttypid,
+								   attribute->atttypmod, attribute->attcollation);
+			newdef->storage = attribute->attstorage;
+			newdef->generated = attribute->attgenerated;
+			if (CompressionMethodIsValid(attribute->attcompression))
+				newdef->compression =
+					pstrdup(GetCompressionMethodName(attribute->attcompression));
+
+			/*
+			 * Regular inheritance children are independent enough not to
+			 * inherit identity columns.  But partitions are integral part
+			 * of a partitioned table and inherit identity column.
+			 */
+			if (is_partition)
+				newdef->identity = attribute->attidentity;
+
+			/*
+			 * Does it match some previously considered column from another
+			 * parent?
 			 */
 			exist_attno = findAttrByName(attributeName, inh_columns);
 			if (exist_attno > 0)
 			{
-				Oid			defTypeId;
-				int32		deftypmod;
-				Oid			defCollId;
+				ColumnDef  *prevdef;
+				Oid			prevtypeid,
+							newtypeid;
+				int32		prevtypmod,
+							newtypmod;
+				Oid			prevcollid,
+							newcollid;
 
 				/*
 				 * Partitions have only one parent and have no column
@@ -2734,68 +2759,61 @@ MergeAttributes(List *columns, const List *supers, char relpersistence,
 				ereport(NOTICE,
 						(errmsg("merging multiple inherited definitions of column \"%s\"",
 								attributeName)));
-				def = (ColumnDef *) list_nth(inh_columns, exist_attno - 1);
+				prevdef = list_nth_node(ColumnDef, inh_columns, exist_attno - 1);
 
 				/*
 				 * Must have the same type and typmod
 				 */
-				typenameTypeIdAndMod(NULL, def->typeName, &defTypeId, &deftypmod);
-				if (defTypeId != attribute->atttypid ||
-					deftypmod != attribute->atttypmod)
+				typenameTypeIdAndMod(NULL, prevdef->typeName, &prevtypeid, &prevtypmod);
+				typenameTypeIdAndMod(NULL, newdef->typeName, &newtypeid, &newtypmod);
+				if (prevtypeid != newtypeid || prevtypmod != newtypmod)
 					ereport(ERROR,
 							(errcode(ERRCODE_DATATYPE_MISMATCH),
 							 errmsg("inherited column \"%s\" has a type conflict",
 									attributeName),
 							 errdetail("%s versus %s",
-									   format_type_with_typemod(defTypeId,
-																deftypmod),
-									   format_type_with_typemod(attribute->atttypid,
-																attribute->atttypmod))));
+									   format_type_with_typemod(prevtypeid, prevtypmod),
+									   format_type_with_typemod(newtypeid, newtypmod))));
 
 				/*
 				 * Must have the same collation
 				 */
-				defCollId = GetColumnDefCollation(NULL, def, defTypeId);
-				if (defCollId != attribute->attcollation)
+				prevcollid = GetColumnDefCollation(NULL, prevdef, prevtypeid);
+				newcollid = GetColumnDefCollation(NULL, newdef, newtypeid);
+				if (prevcollid != newcollid)
 					ereport(ERROR,
 							(errcode(ERRCODE_COLLATION_MISMATCH),
 							 errmsg("inherited column \"%s\" has a collation conflict",
 									attributeName),
 							 errdetail("\"%s\" versus \"%s\"",
-									   get_collation_name(defCollId),
-									   get_collation_name(attribute->attcollation))));
+									   get_collation_name(prevcollid),
+									   get_collation_name(newcollid))));
 
 				/*
 				 * Copy/check storage parameter
 				 */
-				if (def->storage == 0)
-					def->storage = attribute->attstorage;
-				else if (def->storage != attribute->attstorage)
+				if (prevdef->storage == 0)
+					prevdef->storage = newdef->storage;
+				else if (prevdef->storage != newdef->storage)
 					ereport(ERROR,
 							(errcode(ERRCODE_DATATYPE_MISMATCH),
 							 errmsg("inherited column \"%s\" has a storage parameter conflict",
 									attributeName),
 							 errdetail("%s versus %s",
-									   storage_name(def->storage),
-									   storage_name(attribute->attstorage))));
+									   storage_name(prevdef->storage),
+									   storage_name(newdef->storage))));
 
 				/*
 				 * Copy/check compression parameter
 				 */
-				if (CompressionMethodIsValid(attribute->attcompression))
-				{
-					const char *compression =
-						GetCompressionMethodName(attribute->attcompression);
-
-					if (def->compression == NULL)
-						def->compression = pstrdup(compression);
-					else if (strcmp(def->compression, compression) != 0)
-						ereport(ERROR,
-								(errcode(ERRCODE_DATATYPE_MISMATCH),
-								 errmsg("column \"%s\" has a compression method conflict",
-										attributeName),
-								 errdetail("%s versus %s", def->compression, compression)));
-				}
+				if (prevdef->compression == NULL)
+					prevdef->compression = newdef->compression;
+				else if (strcmp(prevdef->compression, newdef->compression) != 0)
+					ereport(ERROR,
+							(errcode(ERRCODE_DATATYPE_MISMATCH),
+							 errmsg("column \"%s\" has a compression method conflict",
+									attributeName),
+							 errdetail("%s versus %s", prevdef->compression, newdef->compression)));
 
 				/*
 				 * In regular inheritance, columns in the parent's primary key
@@ -2826,12 +2844,12 @@ MergeAttributes(List *columns, const List *supers, char relpersistence,
 				if (bms_is_member(parent_attno, nncols) ||
 					bms_is_member(parent_attno - FirstLowInvalidHeapAttributeNumber,
 								  pkattrs))
-					def->is_not_null = true;
+					newdef->is_not_null = true;
 
 				/*
 				 * Check for GENERATED conflicts
 				 */
-				if (def->generated != attribute->attgenerated)
+				if (prevdef->generated != newdef->generated)
 					ereport(ERROR,
 							(errcode(ERRCODE_DATATYPE_MISMATCH),
 							 errmsg("inherited column \"%s\" has a generation conflict",
@@ -2841,43 +2859,30 @@ MergeAttributes(List *columns, const List *supers, char relpersistence,
 				 * Default and other constraints are handled below
 				 */
 
-				def->inhcount++;
-				if (def->inhcount < 0)
+				prevdef->inhcount++;
+				if (prevdef->inhcount < 0)
 					ereport(ERROR,
 							errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
 							errmsg("too many inheritance parents"));
 
 				newattmap->attnums[parent_attno - 1] = exist_attno;
+
+				/* remember for default processing below */
+				savedef = prevdef;
 			}
 			else
 			{
 				/*
 				 * No, create a new inherited column
 				 */
-				def = makeColumnDef(attributeName, attribute->atttypid,
-									attribute->atttypmod, attribute->attcollation);
-				def->inhcount = 1;
-				def->is_local = false;
+				newdef->inhcount = 1;
+				newdef->is_local = false;
 				/* mark attnotnull if parent has it and it's not NO INHERIT */
 				if (bms_is_member(parent_attno, nncols) ||
 					bms_is_member(parent_attno - FirstLowInvalidHeapAttributeNumber,
 								  pkattrs))
-					def->is_not_null = true;
-				def->storage = attribute->attstorage;
-				def->generated = attribute->attgenerated;
-
-				/*
-				 * Regular inheritance children are independent enough not to
-				 * inherit identity columns.  But partitions are integral part
-				 * of a partitioned table and inherit identity column.
-				 */
-				if (is_partition)
-					def->identity = attribute->attidentity;
-
-				if (CompressionMethodIsValid(attribute->attcompression))
-					def->compression =
-						pstrdup(GetCompressionMethodName(attribute->attcompression));
-				inh_columns = lappend(inh_columns, def);
+					newdef->is_not_null = true;
+				inh_columns = lappend(inh_columns, newdef);
 				newattmap->attnums[parent_attno - 1] = ++child_attno;
 
 				/*
@@ -2906,6 +2911,9 @@ MergeAttributes(List *columns, const List *supers, char relpersistence,
 
 					nnconstraints = lappend(nnconstraints, nn);
 				}
+
+				/* remember for default processing below */
+				savedef = newdef;
 			}
 
 			/*
@@ -2927,7 +2935,7 @@ MergeAttributes(List *columns, const List *supers, char relpersistence,
 				 * all the inherited default expressions for the moment.
 				 */
 				inherited_defaults = lappend(inherited_defaults, this_default);
-				cols_with_defaults = lappend(cols_with_defaults, def);
+				cols_with_defaults = lappend(cols_with_defaults, savedef);
 			}
 		}
 
@@ -3065,17 +3073,17 @@ MergeAttributes(List *columns, const List *supers, char relpersistence,
 			newcol_attno++;
 
 			/*
-			 * Does it conflict with some previously inherited column?
+			 * Does it match some inherited column?
 			 */
 			exist_attno = findAttrByName(attributeName, inh_columns);
 			if (exist_attno > 0)
 			{
-				ColumnDef  *def;
-				Oid			defTypeId,
-							newTypeId;
-				int32		deftypmod,
+				ColumnDef  *inhdef;
+				Oid			inhtypeid,
+							newtypeid;
+				int32		inhtypmod,
 							newtypmod;
-				Oid			defcollid,
+				Oid			inhcollid,
 							newcollid;
 
 				/*
@@ -3095,77 +3103,75 @@ MergeAttributes(List *columns, const List *supers, char relpersistence,
 					ereport(NOTICE,
 							(errmsg("moving and merging column \"%s\" with inherited definition", attributeName),
 							 errdetail("User-specified column moved to the position of the inherited column.")));
-				def = (ColumnDef *) list_nth(inh_columns, exist_attno - 1);
+				inhdef = list_nth_node(ColumnDef, inh_columns, exist_attno - 1);
 
 				/*
 				 * Must have the same type and typmod
 				 */
-				typenameTypeIdAndMod(NULL, def->typeName, &defTypeId, &deftypmod);
-				typenameTypeIdAndMod(NULL, newdef->typeName, &newTypeId, &newtypmod);
-				if (defTypeId != newTypeId || deftypmod != newtypmod)
+				typenameTypeIdAndMod(NULL, inhdef->typeName, &inhtypeid, &inhtypmod);
+				typenameTypeIdAndMod(NULL, newdef->typeName, &newtypeid, &newtypmod);
+				if (inhtypeid != newtypeid || inhtypmod != newtypmod)
 					ereport(ERROR,
 							(errcode(ERRCODE_DATATYPE_MISMATCH),
 							 errmsg("column \"%s\" has a type conflict",
 									attributeName),
 							 errdetail("%s versus %s",
-									   format_type_with_typemod(defTypeId,
-																deftypmod),
-									   format_type_with_typemod(newTypeId,
-																newtypmod))));
+									   format_type_with_typemod(inhtypeid, inhtypmod),
+									   format_type_with_typemod(newtypeid, newtypmod))));
 
 				/*
 				 * Must have the same collation
 				 */
-				defcollid = GetColumnDefCollation(NULL, def, defTypeId);
-				newcollid = GetColumnDefCollation(NULL, newdef, newTypeId);
-				if (defcollid != newcollid)
+				inhcollid = GetColumnDefCollation(NULL, inhdef, inhtypeid);
+				newcollid = GetColumnDefCollation(NULL, newdef, newtypeid);
+				if (inhcollid != newcollid)
 					ereport(ERROR,
 							(errcode(ERRCODE_COLLATION_MISMATCH),
 							 errmsg("column \"%s\" has a collation conflict",
 									attributeName),
 							 errdetail("\"%s\" versus \"%s\"",
-									   get_collation_name(defcollid),
+									   get_collation_name(inhcollid),
 									   get_collation_name(newcollid))));
 
 				/*
 				 * Identity is never inherited.  The new column can have an
 				 * identity definition, so we always just take that one.
 				 */
-				def->identity = newdef->identity;
+				inhdef->identity = newdef->identity;
 
 				/*
 				 * Copy storage parameter
 				 */
-				if (def->storage == 0)
-					def->storage = newdef->storage;
-				else if (newdef->storage != 0 && def->storage != newdef->storage)
+				if (inhdef->storage == 0)
+					inhdef->storage = newdef->storage;
+				else if (newdef->storage != 0 && inhdef->storage != newdef->storage)
 					ereport(ERROR,
 							(errcode(ERRCODE_DATATYPE_MISMATCH),
 							 errmsg("column \"%s\" has a storage parameter conflict",
 									attributeName),
 							 errdetail("%s versus %s",
-									   storage_name(def->storage),
+									   storage_name(inhdef->storage),
 									   storage_name(newdef->storage))));
 
 				/*
 				 * Copy compression parameter
 				 */
-				if (def->compression == NULL)
-					def->compression = newdef->compression;
+				if (inhdef->compression == NULL)
+					inhdef->compression = newdef->compression;
 				else if (newdef->compression != NULL)
 				{
-					if (strcmp(def->compression, newdef->compression) != 0)
+					if (strcmp(inhdef->compression, newdef->compression) != 0)
 						ereport(ERROR,
 								(errcode(ERRCODE_DATATYPE_MISMATCH),
 								 errmsg("column \"%s\" has a compression method conflict",
 										attributeName),
-								 errdetail("%s versus %s", def->compression, newdef->compression)));
+								 errdetail("%s versus %s", inhdef->compression, newdef->compression)));
 				}
 
 				/*
 				 * Merge of not-null constraints = OR 'em together
 				 */
-				def->is_not_null |= newdef->is_not_null;
+				inhdef->is_not_null |= newdef->is_not_null;
 
 				/*
 				 * Check for conflicts related to generated columns.
@@ -3182,18 +3188,18 @@ MergeAttributes(List *columns, const List *supers, char relpersistence,
 				 * it results in being able to override the generation
 				 * expression via UPDATEs through the parent.)
 				 */
-				if (def->generated)
+				if (inhdef->generated)
 				{
 					if (newdef->raw_default && !newdef->generated)
 						ereport(ERROR,
 								(errcode(ERRCODE_INVALID_COLUMN_DEFINITION),
 								 errmsg("column \"%s\" inherits from generated column but specifies default",
-										def->colname)));
+										inhdef->colname)));
 					if (newdef->identity)
 						ereport(ERROR,
 								(errcode(ERRCODE_INVALID_COLUMN_DEFINITION),
 								 errmsg("column \"%s\" inherits from generated column but specifies identity",
-										def->colname)));
+										inhdef->colname)));
 				}
 				else
 				{
@@ -3201,7 +3207,7 @@ MergeAttributes(List *columns, const List *supers, char relpersistence,
 						ereport(ERROR,
 								(errcode(ERRCODE_INVALID_COLUMN_DEFINITION),
 								 errmsg("child column \"%s\" specifies generation expression",
-										def->colname),
+										inhdef->colname),
 								 errhint("A child table column cannot be generated unless its parent column is.")));
 				}
 
@@ -3210,12 +3216,12 @@ MergeAttributes(List *columns, const List *supers, char relpersistence,
 				 */
 				if (newdef->raw_default != NULL)
 				{
-					def->raw_default = newdef->raw_default;
-					def->cooked_default = newdef->cooked_default;
+					inhdef->raw_default = newdef->raw_default;
+					inhdef->cooked_default = newdef->cooked_default;
 				}
 
 				/* Mark the column as locally defined */
-				def->is_local = true;
+				inhdef->is_local = true;
 			}
 			else
 			{
-- 
2.25.1

From e8993ab2e00db5f8260ef9d9cafe9fbe069d54a2 Mon Sep 17 00:00:00 2001
From: Ashutosh Bapat <ashutosh.ba...@enterprisedb.com>
Date: Tue, 23 Jan 2024 12:47:46 +0530
Subject: [PATCH 3/4] Separate function to merge a child attribute into
 matching inherited attribute

The logic to merge a child attribute into matching inherited attribute
in MergeAttribute() is only applicable to regular inheritance child. The
code is isolated and coherent enough that it can be separated into a
function of its own. This separation also makes MergeAttribute() more
readable by making it easier to follow high level logic without getting
entangled into details.

Author: Ashutosh Bapat
---
 src/backend/commands/tablecmds.c | 337 +++++++++++++++++--------------
 1 file changed, 184 insertions(+), 153 deletions(-)

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index cde524083e..843cc3bff6 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -359,6 +359,7 @@ static List *MergeAttributes(List *columns, const List *supers, char relpersiste
 							 bool is_partition, List **supconstr,
 							 List **supnotnulls);
 static List *MergeCheckConstraint(List *constraints, const char *name, Node *expr);
+static bool MergeChildAttribute(ColumnDef *newdef, int newcol_attno, List *inh_columns);
 static void MergeAttributesIntoExisting(Relation child_rel, Relation parent_rel, bool ispartition);
 static void MergeConstraintsIntoExisting(Relation child_rel, Relation parent_rel);
 static void StoreCatalogInheritance(Oid relationId, List *supers,
@@ -3066,167 +3067,21 @@ MergeAttributes(List *columns, const List *supers, char relpersistence,
 
 		foreach(lc, columns)
 		{
-			ColumnDef  *newdef = lfirst(lc);
-			char	   *attributeName = newdef->colname;
-			int			exist_attno;
+			ColumnDef  *newdef = lfirst_node(ColumnDef, lc);
 
 			newcol_attno++;
 
 			/*
-			 * Does it match some inherited column?
+			 * Partitions have only one parent and have no column definitions
+			 * of their own, so conflict should never occur.
 			 */
-			exist_attno = findAttrByName(attributeName, inh_columns);
-			if (exist_attno > 0)
-			{
-				ColumnDef  *inhdef;
-				Oid			inhtypeid,
-							newtypeid;
-				int32		inhtypmod,
-							newtypmod;
-				Oid			inhcollid,
-							newcollid;
-
-				/*
-				 * Partitions have only one parent and have no column
-				 * definitions of their own, so conflict should never occur.
-				 */
-				Assert(!is_partition);
-
-				/*
-				 * Yes, try to merge the two column definitions.
-				 */
-				if (exist_attno == newcol_attno)
-					ereport(NOTICE,
-							(errmsg("merging column \"%s\" with inherited definition",
-									attributeName)));
-				else
-					ereport(NOTICE,
-							(errmsg("moving and merging column \"%s\" with inherited definition", attributeName),
-							 errdetail("User-specified column moved to the position of the inherited column.")));
-				inhdef = list_nth_node(ColumnDef, inh_columns, exist_attno - 1);
-
-				/*
-				 * Must have the same type and typmod
-				 */
-				typenameTypeIdAndMod(NULL, inhdef->typeName, &inhtypeid, &inhtypmod);
-				typenameTypeIdAndMod(NULL, newdef->typeName, &newtypeid, &newtypmod);
-				if (inhtypeid != newtypeid || inhtypmod != newtypmod)
-					ereport(ERROR,
-							(errcode(ERRCODE_DATATYPE_MISMATCH),
-							 errmsg("column \"%s\" has a type conflict",
-									attributeName),
-							 errdetail("%s versus %s",
-									   format_type_with_typemod(inhtypeid, inhtypmod),
-									   format_type_with_typemod(newtypeid, newtypmod))));
-
-				/*
-				 * Must have the same collation
-				 */
-				inhcollid = GetColumnDefCollation(NULL, inhdef, inhtypeid);
-				newcollid = GetColumnDefCollation(NULL, newdef, newtypeid);
-				if (inhcollid != newcollid)
-					ereport(ERROR,
-							(errcode(ERRCODE_COLLATION_MISMATCH),
-							 errmsg("column \"%s\" has a collation conflict",
-									attributeName),
-							 errdetail("\"%s\" versus \"%s\"",
-									   get_collation_name(inhcollid),
-									   get_collation_name(newcollid))));
-
-				/*
-				 * Identity is never inherited.  The new column can have an
-				 * identity definition, so we always just take that one.
-				 */
-				inhdef->identity = newdef->identity;
-
-				/*
-				 * Copy storage parameter
-				 */
-				if (inhdef->storage == 0)
-					inhdef->storage = newdef->storage;
-				else if (newdef->storage != 0 && inhdef->storage != newdef->storage)
-					ereport(ERROR,
-							(errcode(ERRCODE_DATATYPE_MISMATCH),
-							 errmsg("column \"%s\" has a storage parameter conflict",
-									attributeName),
-							 errdetail("%s versus %s",
-									   storage_name(inhdef->storage),
-									   storage_name(newdef->storage))));
-
-				/*
-				 * Copy compression parameter
-				 */
-				if (inhdef->compression == NULL)
-					inhdef->compression = newdef->compression;
-				else if (newdef->compression != NULL)
-				{
-					if (strcmp(inhdef->compression, newdef->compression) != 0)
-						ereport(ERROR,
-								(errcode(ERRCODE_DATATYPE_MISMATCH),
-								 errmsg("column \"%s\" has a compression method conflict",
-										attributeName),
-								 errdetail("%s versus %s", inhdef->compression, newdef->compression)));
-				}
-
-				/*
-				 * Merge of not-null constraints = OR 'em together
-				 */
-				inhdef->is_not_null |= newdef->is_not_null;
-
-				/*
-				 * Check for conflicts related to generated columns.
-				 *
-				 * If the parent column is generated, the child column will be
-				 * made a generated column if it isn't already.  If it is a
-				 * generated column, we'll take its generation expression in
-				 * preference to the parent's.  We must check that the child
-				 * column doesn't specify a default value or identity, which
-				 * matches the rules for a single column in parse_utilcmd.c.
-				 *
-				 * Conversely, if the parent column is not generated, the
-				 * child column can't be either.  (We used to allow that, but
-				 * it results in being able to override the generation
-				 * expression via UPDATEs through the parent.)
-				 */
-				if (inhdef->generated)
-				{
-					if (newdef->raw_default && !newdef->generated)
-						ereport(ERROR,
-								(errcode(ERRCODE_INVALID_COLUMN_DEFINITION),
-								 errmsg("column \"%s\" inherits from generated column but specifies default",
-										inhdef->colname)));
-					if (newdef->identity)
-						ereport(ERROR,
-								(errcode(ERRCODE_INVALID_COLUMN_DEFINITION),
-								 errmsg("column \"%s\" inherits from generated column but specifies identity",
-										inhdef->colname)));
-				}
-				else
-				{
-					if (newdef->generated)
-						ereport(ERROR,
-								(errcode(ERRCODE_INVALID_COLUMN_DEFINITION),
-								 errmsg("child column \"%s\" specifies generation expression",
-										inhdef->colname),
-								 errhint("A child table column cannot be generated unless its parent column is.")));
-				}
-
-				/*
-				 * If new def has a default, override previous default
-				 */
-				if (newdef->raw_default != NULL)
-				{
-					inhdef->raw_default = newdef->raw_default;
-					inhdef->cooked_default = newdef->cooked_default;
-				}
+			Assert(!is_partition);
 
-				/* Mark the column as locally defined */
-				inhdef->is_local = true;
-			}
-			else
+			if (!MergeChildAttribute(newdef, newcol_attno, inh_columns))
 			{
 				/*
-				 * No, attach new column to result columns
+				 * No inherited attribute, attach new column to result
+				 * columns.
 				 */
 				inh_columns = lappend(inh_columns, newdef);
 			}
@@ -3419,6 +3274,182 @@ MergeCheckConstraint(List *constraints, const char *name, Node *expr)
 	return lappend(constraints, newcon);
 }
 
+/*
+ * MergeChildAttribute
+ *		Merge given child attribute definition into any inherited attribute with
+ *  the same name.
+ *
+ * Input arguments:
+ * 'newdef' is the column/attribute definition from the child table.
+ * 'newcol_attno' is the attribute number in child table's schema definition
+ * 'inh_columns' is the list of inherited ColumnDefs.
+ *
+ * Return value:
+ * True if the given attribute is merged into an inherited attribute with the
+ * same name. False if no matching inherited attribute is found. When returning
+ * true, matching ColumnDef in 'inh_columns' list is modified. Child
+ * attribute's ColumnDef remains unchanged.
+ *
+ * Notes:
+ *  (1) The attribute is merged according to the rules laid out in the prologue
+ *  of MergeAttributes().
+ *  (2) If matching inherited attribute exists but the child attribute can not
+ *  be merged into it, the function throws respective errors.
+ *  (3) A partition can not have its own column definitions. Hence this
+ *  function is applicable only to a regular inheritance child.
+ */
+static bool
+MergeChildAttribute(ColumnDef *newdef, int newcol_attno, List *inh_columns)
+{
+	char	   *attributeName = newdef->colname;
+	int			exist_attno;
+	ColumnDef  *inhdef;
+	Oid			inhtypeid,
+				newtypeid;
+	int32		inhtypmod,
+				newtypmod;
+	Oid			inhcollid,
+				newcollid;
+
+	/*
+	 * Does it match some inherited column?
+	 */
+	exist_attno = findAttrByName(attributeName, inh_columns);
+	if (exist_attno <= 0)
+		return false;
+
+	/*
+	 * Yes, try to merge the two column definitions.
+	 */
+	if (exist_attno == newcol_attno)
+		ereport(NOTICE,
+				(errmsg("merging column \"%s\" with inherited definition",
+						attributeName)));
+	else
+		ereport(NOTICE,
+				(errmsg("moving and merging column \"%s\" with inherited definition", attributeName),
+				 errdetail("User-specified column moved to the position of the inherited column.")));
+	inhdef = list_nth_node(ColumnDef, inh_columns, exist_attno - 1);
+
+	/*
+	 * Must have the same type and typmod
+	 */
+	typenameTypeIdAndMod(NULL, inhdef->typeName, &inhtypeid, &inhtypmod);
+	typenameTypeIdAndMod(NULL, newdef->typeName, &newtypeid, &newtypmod);
+	if (inhtypeid != newtypeid || inhtypmod != newtypmod)
+		ereport(ERROR,
+				(errcode(ERRCODE_DATATYPE_MISMATCH),
+				 errmsg("column \"%s\" has a type conflict",
+						attributeName),
+				 errdetail("%s versus %s",
+						   format_type_with_typemod(inhtypeid, inhtypmod),
+						   format_type_with_typemod(newtypeid, newtypmod))));
+
+	/*
+	 * Must have the same collation
+	 */
+	inhcollid = GetColumnDefCollation(NULL, inhdef, inhtypeid);
+	newcollid = GetColumnDefCollation(NULL, newdef, newtypeid);
+	if (inhcollid != newcollid)
+		ereport(ERROR,
+				(errcode(ERRCODE_COLLATION_MISMATCH),
+				 errmsg("column \"%s\" has a collation conflict",
+						attributeName),
+				 errdetail("\"%s\" versus \"%s\"",
+						   get_collation_name(inhcollid),
+						   get_collation_name(newcollid))));
+
+	/*
+	 * Identity is never inherited by a regular inheritance child. Pick
+	 * child's identity definition if there's one.
+	 */
+	inhdef->identity = newdef->identity;
+
+	/*
+	 * Copy storage parameter
+	 */
+	if (inhdef->storage == 0)
+		inhdef->storage = newdef->storage;
+	else if (newdef->storage != 0 && inhdef->storage != newdef->storage)
+		ereport(ERROR,
+				(errcode(ERRCODE_DATATYPE_MISMATCH),
+				 errmsg("column \"%s\" has a storage parameter conflict",
+						attributeName),
+				 errdetail("%s versus %s",
+						   storage_name(inhdef->storage),
+						   storage_name(newdef->storage))));
+
+	/*
+	 * Copy compression parameter
+	 */
+	if (inhdef->compression == NULL)
+		inhdef->compression = newdef->compression;
+	else if (newdef->compression != NULL)
+	{
+		if (strcmp(inhdef->compression, newdef->compression) != 0)
+			ereport(ERROR,
+					(errcode(ERRCODE_DATATYPE_MISMATCH),
+					 errmsg("column \"%s\" has a compression method conflict",
+							attributeName),
+					 errdetail("%s versus %s", inhdef->compression, newdef->compression)));
+	}
+
+	/*
+	 * Merge of not-null constraints = OR 'em together
+	 */
+	inhdef->is_not_null |= newdef->is_not_null;
+
+	/*
+	 * Check for conflicts related to generated columns.
+	 *
+	 * If the parent column is generated, the child column will be made a
+	 * generated column if it isn't already.  If it is a generated column,
+	 * we'll take its generation expression in preference to the parent's.  We
+	 * must check that the child column doesn't specify a default value or
+	 * identity, which matches the rules for a single column in
+	 * parse_utilcmd.c.
+	 *
+	 * Conversely, if the parent column is not generated, the child column
+	 * can't be either.  (We used to allow that, but it results in being able
+	 * to override the generation expression via UPDATEs through the parent.)
+	 */
+	if (inhdef->generated)
+	{
+		if (newdef->raw_default && !newdef->generated)
+			ereport(ERROR,
+					(errcode(ERRCODE_INVALID_COLUMN_DEFINITION),
+					 errmsg("column \"%s\" inherits from generated column but specifies default",
+							inhdef->colname)));
+		if (newdef->identity)
+			ereport(ERROR,
+					(errcode(ERRCODE_INVALID_COLUMN_DEFINITION),
+					 errmsg("column \"%s\" inherits from generated column but specifies identity",
+							inhdef->colname)));
+	}
+	else
+	{
+		if (newdef->generated)
+			ereport(ERROR,
+					(errcode(ERRCODE_INVALID_COLUMN_DEFINITION),
+					 errmsg("child column \"%s\" specifies generation expression",
+							inhdef->colname),
+					 errhint("A child table column cannot be generated unless its parent column is.")));
+	}
+
+	/*
+	 * If new def has a default, override previous default
+	 */
+	if (newdef->raw_default != NULL)
+	{
+		inhdef->raw_default = newdef->raw_default;
+		inhdef->cooked_default = newdef->cooked_default;
+	}
+
+	/* Mark the column as locally defined */
+	inhdef->is_local = true;
+
+	return true;
+}
 
 /*
  * StoreCatalogInheritance
-- 
2.25.1

Reply via email to