Improve compression and storage support with inheritance

A child table can specify a compression or storage method different
from its parents.  This was previously an error.  (But this was
inconsistently enforced because for example the settings could be
changed later using ALTER TABLE.)  This now also allows an explicit
override if multiple parents have different compression or storage
settings, which was previously an error that could not be overridden.

The compression and storage properties remains unchanged in a child
inheriting from parent(s) after its creation, i.e., when using ALTER
TABLE ...  INHERIT.  (This is not changed.)

Before this change, the error detail would mention the first pair of
conflicting parent compression or storage methods.  But with this
change it waits till the child specification is considered by which
time we may have encountered many such conflicting pairs.  Hence the
error detail after this change does not include the conflicting
compression/storage methods.  Those can be obtained from parent
definitions if necessary.  The code to maintain list of all
conflicting methods or even the first conflicting pair does not seem
worth the convenience it offers.  This change is inline with what we
do with conflicting default values.

Before this commit, the specified storage method could be stored in
ColumnDef::storage (CREATE TABLE ... LIKE) or ColumnDef::storage_name
(CREATE TABLE ...).  This caused the MergeChildAttribute() and
MergeInheritedAttribute() to ignore a storage method specified in the
child definition since it looked only at ColumnDef::storage.  This
commit removes ColumnDef::storage and instead uses
ColumnDef::storage_name to save any storage method specification. This
is similar to how compression method specification is handled.

Author: Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>
Discussion: https://www.postgresql.org/message-id/flat/24656cec-d6ef-4d15-8b5b-e8dfc9c833a7@eisentraut.org

1 files changed, 63 insertions, 75 deletions

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 86ea3a228b6..237eeeec67e 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -350,6 +350,16 @@ typedef struct ForeignTruncateInfo
 #define child_dependency_type(child_is_partition)	\
 	((child_is_partition) ? DEPENDENCY_AUTO : DEPENDENCY_NORMAL)
 
+/*
+ * Bogus property string to track conflict in inherited properties of a column.
+ * It is currently used for storage and compression specifications, but may be
+ * used for other string specifications in future. It can be any string which
+ * does not look like a valid compression or storage method. It is meant to be
+ * used by MergeAttributes() and its minions. It is not expected to be stored
+ * on disk.
+ */
+static const char *conflicting_column_property = "*** conflicting column property ***";
+
 static void truncate_check_rel(Oid relid, Form_pg_class reltuple);
 static void truncate_check_perms(Oid relid, Form_pg_class reltuple);
 static void truncate_check_activity(Relation rel);
@@ -360,7 +370,8 @@ static List *MergeAttributes(List *columns, const List *supers, char relpersiste
 							 List **supnotnulls);
 static List *MergeCheckConstraint(List *constraints, const char *name, Node *expr);
 static void MergeChildAttribute(List *inh_columns, int exist_attno, int newcol_attno, const ColumnDef *newdef);
-static ColumnDef *MergeInheritedAttribute(List *inh_columns, int exist_attno, const ColumnDef *newdef);
+static ColumnDef *MergeInheritedAttribute(List *inh_columns, int exist_attno, const ColumnDef *newdef,
+										  bool *have_deferred_conflicts);
 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,
@@ -620,7 +631,6 @@ static ObjectAddress ATExecSetCompression(Relation rel,
 										  const char *column, Node *newValue, LOCKMODE lockmode);
 
 static void index_copy_data(Relation rel, RelFileLocator newrlocator);
-static const char *storage_name(char c);
 
 static void RangeVarCallbackForDropRelation(const RangeVar *rel, Oid relOid,
 											Oid oldRelOid, void *arg);
@@ -1363,9 +1373,7 @@ BuildDescForRelation(const List *columns)
 		att->attidentity = entry->identity;
 		att->attgenerated = entry->generated;
 		att->attcompression = GetAttributeCompression(att->atttypid, entry->compression);
-		if (entry->storage)
-			att->attstorage = entry->storage;
-		else if (entry->storage_name)
+		if (entry->storage_name)
 			att->attstorage = GetAttributeStorage(att->atttypid, entry->storage_name);
 	}
 
@@ -2388,28 +2396,6 @@ truncate_check_activity(Relation rel)
 	CheckTableNotInUse(rel, "TRUNCATE");
 }
 
-/*
- * storage_name
- *	  returns the name corresponding to a typstorage/attstorage enum value
- */
-static const char *
-storage_name(char c)
-{
-	switch (c)
-	{
-		case TYPSTORAGE_PLAIN:
-			return "PLAIN";
-		case TYPSTORAGE_EXTERNAL:
-			return "EXTERNAL";
-		case TYPSTORAGE_EXTENDED:
-			return "EXTENDED";
-		case TYPSTORAGE_MAIN:
-			return "MAIN";
-		default:
-			return "???";
-	}
-}
-
 /*----------
  * MergeAttributes
  *		Returns new schema given initial schema and superclasses.
@@ -2483,7 +2469,7 @@ MergeAttributes(List *columns, const List *supers, char relpersistence,
 	List	   *inh_columns = NIL;
 	List	   *constraints = NIL;
 	List	   *nnconstraints = NIL;
-	bool		have_bogus_defaults = false;
+	bool		have_deferred_conflicts = false;
 	int			child_attno;
 	static Node bogus_marker = {0}; /* marks conflicting defaults */
 	List	   *saved_columns = NIL;
@@ -2720,11 +2706,10 @@ MergeAttributes(List *columns, const List *supers, char relpersistence,
 			 */
 			newdef = makeColumnDef(attributeName, attribute->atttypid,
 								   attribute->atttypmod, attribute->attcollation);
-			newdef->storage = attribute->attstorage;
+			newdef->storage_name = GetAttributeStorageName(attribute->attstorage);
 			newdef->generated = attribute->attgenerated;
 			if (CompressionMethodIsValid(attribute->attcompression))
-				newdef->compression =
-					pstrdup(GetCompressionMethodName(attribute->attcompression));
+				newdef->compression = GetCompressionMethodName(attribute->attcompression);
 
 			/*
 			 * Regular inheritance children are independent enough not to
@@ -2744,7 +2729,8 @@ MergeAttributes(List *columns, const List *supers, char relpersistence,
 				/*
 				 * Yes, try to merge the two column definitions.
 				 */
-				mergeddef = MergeInheritedAttribute(inh_columns, exist_attno, newdef);
+				mergeddef = MergeInheritedAttribute(inh_columns, exist_attno, newdef,
+													&have_deferred_conflicts);
 
 				newattmap->attnums[parent_attno - 1] = exist_attno;
 
@@ -2867,7 +2853,7 @@ MergeAttributes(List *columns, const List *supers, char relpersistence,
 			else if (!equal(def->cooked_default, this_default))
 			{
 				def->cooked_default = &bogus_marker;
-				have_bogus_defaults = true;
+				have_deferred_conflicts = true;
 			}
 		}
 
@@ -3077,10 +3063,10 @@ MergeAttributes(List *columns, const List *supers, char relpersistence,
 	}
 
 	/*
-	 * If we found any conflicting parent default values, check to make sure
-	 * they were overridden by the child.
+	 * If we found any conflicting parent default values or conflicting parent
+	 * properties, check to make sure they were overridden by the child.
 	 */
-	if (have_bogus_defaults)
+	if (have_deferred_conflicts)
 	{
 		foreach(lc, columns)
 		{
@@ -3101,6 +3087,20 @@ MergeAttributes(List *columns, const List *supers, char relpersistence,
 									def->colname),
 							 errhint("To resolve the conflict, specify a default explicitly.")));
 			}
+
+			if (def->compression == conflicting_column_property)
+				ereport(ERROR,
+						(errcode(ERRCODE_DATATYPE_MISMATCH),
+						 errmsg("column \"%s\" inherits conflicting compression methods",
+								def->colname),
+						 errhint("To resolve the conflict, specify a compression method explicitly.")));
+
+			if (def->storage_name == conflicting_column_property)
+				ereport(ERROR,
+						(errcode(ERRCODE_DATATYPE_MISMATCH),
+						 errmsg("column \"%s\" inherits conflicting storage methods",
+								def->colname),
+						 errhint("To resolve the conflict, specify a storage method explicitly.")));
 		}
 	}
 
@@ -3250,33 +3250,18 @@ MergeChildAttribute(List *inh_columns, int exist_attno, int newcol_attno, const
 	inhdef->identity = newdef->identity;
 
 	/*
-	 * Copy storage parameter
+	 * Child storage specification, if any, overrides inherited storage
+	 * property.
 	 */
-	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))));
+	if (newdef->storage_name != NULL)
+		inhdef->storage_name = newdef->storage_name;
 
 	/*
-	 * Copy compression parameter
+	 * Child compression specification, if any, overrides inherited
+	 * compression property.
 	 */
-	if (inhdef->compression == NULL)
+	if (newdef->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
@@ -3343,6 +3328,10 @@ MergeChildAttribute(List *inh_columns, int exist_attno, int newcol_attno, const
  * 'exist_attno' is the number the existing matching attribute in inh_columns.
  * 'newdef' is the new parent column/attribute definition to be merged.
  *
+ * Output arguments:
+ * 'have_deferred_conflicts' is set to true if there is a conflict in inherited
+ * 		compression properties; remains unchanged otherwise.
+ *
  * The matching ColumnDef in 'inh_columns' list is modified and returned.
  *
  * Notes:
@@ -3356,7 +3345,8 @@ MergeChildAttribute(List *inh_columns, int exist_attno, int newcol_attno, const
 static ColumnDef *
 MergeInheritedAttribute(List *inh_columns,
 						int exist_attno,
-						const ColumnDef *newdef)
+						const ColumnDef *newdef,
+						bool *have_deferred_conflicts)
 {
 	char	   *attributeName = newdef->colname;
 	ColumnDef  *prevdef;
@@ -3403,28 +3393,26 @@ MergeInheritedAttribute(List *inh_columns,
 	/*
 	 * 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))));
+	if (prevdef->storage_name == NULL)
+		prevdef->storage_name = newdef->storage_name;
+	else if (newdef->storage_name != NULL &&
+			 strcmp(prevdef->storage_name, newdef->storage_name) != 0)
+	{
+		prevdef->storage_name = conflicting_column_property;
+		*have_deferred_conflicts = true;
+	}
 
 	/*
 	 * 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)));
+	else if (newdef->compression != NULL &&
+			 strcmp(prevdef->compression, newdef->compression) != 0)
+	{
+		prevdef->compression = conflicting_column_property;
+		*have_deferred_conflicts = true;
+	}
 
 	/*
 	 * Check for GENERATED conflicts