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

Author: petere

doc/src/sgml/ref/create_table.sgml

@@ -398,7 +398,13 @@ WITH ( MODULUS <replaceable class="parameter">numeric_literal</replaceable>, REM
      </para>
 
      <para>
-      Column <literal>STORAGE</literal> settings are also copied from parent tables.
+      Column storage and compression settings are inherited from parent
+      tables.  If a column is inherited from multiple tables, the storage
+      settings or any explicit compression settings for the column must be the
+      same in all parent tables, else an error is reported.  Storage or
+      compression settings explicitly specified for the new table override any
+      inherited settings and can also be used to override conflicting
+      inherited settings.
      </para>
 
      <para>

src/backend/catalog/pg_type.c

@@ -974,3 +974,26 @@ makeMultirangeTypeName(const char *rangeTypeName, Oid typeNamespace)
 
 	return pstrdup(buf);
 }
+
+/*
+ * GetAttributeStorageName
+ *	  returns the name corresponding to a typstorage/attstorage enum value.
+ */
+const char *
+GetAttributeStorageName(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:
+			elog(ERROR, "invalid storage type %c", c);
+			return NULL;
+	}
+}

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

src/backend/nodes/makefuncs.c

@@ -500,7 +500,7 @@ makeColumnDef(const char *colname, Oid typeOid, int32 typmod, Oid collOid)
 	n->is_local = true;
 	n->is_not_null = false;
 	n->is_from_type = false;
-	n->storage = 0;
+	n->storage_name = NULL;
 	n->raw_default = NULL;
 	n->cooked_default = NULL;
 	n->collClause = NULL;

src/backend/parser/gram.y

@@ -3754,7 +3754,6 @@ columnDef:	ColId Typename opt_column_storage opt_column_compression create_gener
 					n->is_local = true;
 					n->is_not_null = false;
 					n->is_from_type = false;
-					n->storage = 0;
 					n->raw_default = NULL;
 					n->cooked_default = NULL;
 					n->collOid = InvalidOid;
@@ -3776,7 +3775,7 @@ columnOptions:	ColId ColQualList
 					n->is_local = true;
 					n->is_not_null = false;
 					n->is_from_type = false;
-					n->storage = 0;
+					n->storage_name = NULL;
 					n->raw_default = NULL;
 					n->cooked_default = NULL;
 					n->collOid = InvalidOid;
@@ -3795,7 +3794,7 @@ columnOptions:	ColId ColQualList
 					n->is_local = true;
 					n->is_not_null = false;
 					n->is_from_type = false;
-					n->storage = 0;
+					n->storage_name = NULL;
 					n->raw_default = NULL;
 					n->cooked_default = NULL;
 					n->collOid = InvalidOid;
@@ -13858,7 +13857,7 @@ TableFuncElement:	ColId Typename opt_collate_clause
 					n->is_local = true;
 					n->is_not_null = false;
 					n->is_from_type = false;
-					n->storage = 0;
+					n->storage_name = NULL;
 					n->raw_default = NULL;
 					n->cooked_default = NULL;
 					n->collClause = (CollateClause *) $3;

src/backend/parser/parse_utilcmd.c

@@ -1134,15 +1134,14 @@ transformTableLikeClause(CreateStmtContext *cxt, TableLikeClause *table_like_cla
 
 		/* Likewise, copy storage if requested */
 		if (table_like_clause->options & CREATE_TABLE_LIKE_STORAGE)
-			def->storage = attribute->attstorage;
+			def->storage_name = GetAttributeStorageName(attribute->attstorage);
 		else
-			def->storage = 0;
+			def->storage_name = NULL;
 
 		/* Likewise, copy compression if requested */
 		if ((table_like_clause->options & CREATE_TABLE_LIKE_COMPRESSION) != 0
 			&& CompressionMethodIsValid(attribute->attcompression))
-			def->compression =
-				pstrdup(GetCompressionMethodName(attribute->attcompression));
+			def->compression = GetCompressionMethodName(attribute->attcompression);
 		else
 			def->compression = NULL;
 

src/include/catalog/pg_type.h

@@ -404,4 +404,6 @@ extern bool moveArrayTypeName(Oid typeOid, const char *typeName,
 extern char *makeMultirangeTypeName(const char *rangeTypeName,
 									Oid typeNamespace);
 
+extern const char *GetAttributeStorageName(char storage);
+
 #endif							/* PG_TYPE_H */

src/include/nodes/parsenodes.h

@@ -712,13 +712,12 @@ typedef struct ColumnDef
 	NodeTag		type;
 	char	   *colname;		/* name of column */
 	TypeName   *typeName;		/* type of column */
-	char	   *compression;	/* compression method for column */
+	const char *compression;	/* compression method for column */
 	int			inhcount;		/* number of times column is inherited */
 	bool		is_local;		/* column has local (non-inherited) def'n */
 	bool		is_not_null;	/* NOT NULL constraint specified? */
 	bool		is_from_type;	/* column definition came from table type */
-	char		storage;		/* attstorage setting, or 0 for default */
-	char	   *storage_name;	/* attstorage setting name or NULL for default */
+	const char *storage_name;	/* attstorage setting name or NULL for default */
 	Node	   *raw_default;	/* default value (untransformed parse tree) */
 	Node	   *cooked_default; /* default value (transformed expr tree) */
 	char		identity;		/* attidentity setting */