Revert "Improve compression and storage support with inheritance"

This reverts commit 0413a55.

pg_dump cannot currently dump all the structures that are allowed by
this patch.  This needs more work in pg_dump and more test coverage.

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,13 +398,7 @@ WITH ( MODULUS <replaceable class="parameter">numeric_literal</replaceable>, REM
      </para>
 
      <para>
-      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.
+      Column <literal>STORAGE</literal> settings are also copied from parent tables.
      </para>
 
      <para>

src/backend/catalog/pg_type.c

@@ -974,26 +974,3 @@ 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,16 +350,6 @@ 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);
@@ -370,8 +360,7 @@ 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,
-										  bool *have_deferred_conflicts);
+static ColumnDef *MergeInheritedAttribute(List *inh_columns, int exist_attno, const ColumnDef *newdef);
 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,
@@ -631,6 +620,7 @@ 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);
@@ -1373,7 +1363,9 @@ BuildDescForRelation(const List *columns)
 		att->attidentity = entry->identity;
 		att->attgenerated = entry->generated;
 		att->attcompression = GetAttributeCompression(att->atttypid, entry->compression);
-		if (entry->storage_name)
+		if (entry->storage)
+			att->attstorage = entry->storage;
+		else if (entry->storage_name)
 			att->attstorage = GetAttributeStorage(att->atttypid, entry->storage_name);
 	}
 
@@ -2396,6 +2388,28 @@ 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.
@@ -2469,7 +2483,7 @@ MergeAttributes(List *columns, const List *supers, char relpersistence,
 	List	   *inh_columns = NIL;
 	List	   *constraints = NIL;
 	List	   *nnconstraints = NIL;
-	bool		have_deferred_conflicts = false;
+	bool		have_bogus_defaults = false;
 	int			child_attno;
 	static Node bogus_marker = {0}; /* marks conflicting defaults */
 	List	   *saved_columns = NIL;
@@ -2706,10 +2720,11 @@ MergeAttributes(List *columns, const List *supers, char relpersistence,
 			 */
 			newdef = makeColumnDef(attributeName, attribute->atttypid,
 								   attribute->atttypmod, attribute->attcollation);
-			newdef->storage_name = GetAttributeStorageName(attribute->attstorage);
+			newdef->storage = attribute->attstorage;
 			newdef->generated = attribute->attgenerated;
 			if (CompressionMethodIsValid(attribute->attcompression))
-				newdef->compression = GetCompressionMethodName(attribute->attcompression);
+				newdef->compression =
+					pstrdup(GetCompressionMethodName(attribute->attcompression));
 
 			/*
 			 * Regular inheritance children are independent enough not to
@@ -2729,8 +2744,7 @@ MergeAttributes(List *columns, const List *supers, char relpersistence,
 				/*
 				 * Yes, try to merge the two column definitions.
 				 */
-				mergeddef = MergeInheritedAttribute(inh_columns, exist_attno, newdef,
-													&have_deferred_conflicts);
+				mergeddef = MergeInheritedAttribute(inh_columns, exist_attno, newdef);
 
 				newattmap->attnums[parent_attno - 1] = exist_attno;
 
@@ -2853,7 +2867,7 @@ MergeAttributes(List *columns, const List *supers, char relpersistence,
 			else if (!equal(def->cooked_default, this_default))
 			{
 				def->cooked_default = &bogus_marker;
-				have_deferred_conflicts = true;
+				have_bogus_defaults = true;
 			}
 		}
 
@@ -3063,10 +3077,10 @@ MergeAttributes(List *columns, const List *supers, char relpersistence,
 	}
 
 	/*
-	 * If we found any conflicting parent default values or conflicting parent
-	 * properties, check to make sure they were overridden by the child.
+	 * If we found any conflicting parent default values, check to make sure
+	 * they were overridden by the child.
 	 */
-	if (have_deferred_conflicts)
+	if (have_bogus_defaults)
 	{
 		foreach(lc, columns)
 		{
@@ -3087,20 +3101,6 @@ 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,18 +3250,33 @@ MergeChildAttribute(List *inh_columns, int exist_attno, int newcol_attno, const
 	inhdef->identity = newdef->identity;
 
 	/*
-	 * Child storage specification, if any, overrides inherited storage
-	 * property.
+	 * Copy storage parameter
 	 */
-	if (newdef->storage_name != NULL)
-		inhdef->storage_name = newdef->storage_name;
+	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))));
 
 	/*
-	 * Child compression specification, if any, overrides inherited
-	 * compression property.
+	 * Copy compression parameter
 	 */
-	if (newdef->compression != NULL)
+	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
@@ -3328,10 +3343,6 @@ 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:
@@ -3345,8 +3356,7 @@ MergeChildAttribute(List *inh_columns, int exist_attno, int newcol_attno, const
 static ColumnDef *
 MergeInheritedAttribute(List *inh_columns,
 						int exist_attno,
-						const ColumnDef *newdef,
-						bool *have_deferred_conflicts)
+						const ColumnDef *newdef)
 {
 	char	   *attributeName = newdef->colname;
 	ColumnDef  *prevdef;
@@ -3393,26 +3403,28 @@ MergeInheritedAttribute(List *inh_columns,
 	/*
 	 * Copy/check storage parameter
 	 */
-	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;
-	}
+	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 (newdef->compression != NULL &&
-			 strcmp(prevdef->compression, newdef->compression) != 0)
-	{
-		prevdef->compression = conflicting_column_property;
-		*have_deferred_conflicts = true;
-	}
+	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

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_name = NULL;
+	n->storage = 0;
 	n->raw_default = NULL;
 	n->cooked_default = NULL;
 	n->collClause = NULL;

src/backend/parser/gram.y

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

src/backend/parser/parse_utilcmd.c

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

src/include/catalog/pg_type.h

@@ -404,6 +404,4 @@ 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,12 +712,13 @@ typedef struct ColumnDef
 	NodeTag		type;
 	char	   *colname;		/* name of column */
 	TypeName   *typeName;		/* type of column */
-	const char *compression;	/* compression method for column */
+	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 */
-	const char *storage_name;	/* attstorage setting name or NULL for default */
+	char		storage;		/* attstorage setting, or 0 for default */
+	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 */