Subject: Re: table inheritance versus column compression and storage settings

From: Ashutosh Bapat <[email-masked]>
Date: Tue, 5 Dec 2023 09:56:27 +0530

On Mon, Dec 4, 2023 at 4:23 PM Peter Eisentraut <[email-masked]> wrote:

>
> Looking at the code, I suspect these rules were just sort of
> copy-and-pasted from the nearby rules for types and collations.  The
> latter are needed so that a table with inheritance children can present
> a logically consistent view of the data.  But compression and storage
> are physical properties that are not logically visible, so every table
> in an inheritance hierarchy can have their own setting.

Incidentally I was looking at that code yesterday and had the same thoughts.

>
> I think the rules should be approximately like this (both for
> compression and storage):
>
> - A newly created child inherits the settings from the parent.
> - A newly created child can override the settings.
> - Attaching a child changes nothing.

Looks fine to me.

> - When inheriting from multiple parents with different settings, an
> explicit setting in the child is required.

When no explicit setting for child is specified, it will throw an
error as it does today. Right?

-- 
Best Wishes,
Ashutosh Bapat

---

From: Peter Eisentraut <[email-masked]>
Date: Tue, 5 Dec 2023 13:58:10 +0100

> error as it does today. Right?

Yes, it would throw an error, but a different error than today, saying 
something like "the settings in the parents conflict, so you need to 
specify one here to override the conflict".

---

From: Ashutosh Bapat <[email-masked]>
Date: Wed, 31 Jan 2024 13:29:26 +0530

> specify one here to override the conflict".

PFA patch fixing inheritance and compression. It also fixes a crash
reported in [1].

The storage looks more involved. The way it has been coded, the child
always inherits the parent's storage properties.
#create table t1 (a text storage plain);
CREATE TABLE
#create table c1 (b text storage main) inherits(t1);
CREATE TABLE
#create table c1 (a text storage main) inherits(t1);
NOTICE:  merging column "a" with inherited definition
CREATE TABLE
#\d+ t1
                                          Table "public.t1"
 Column | Type | Collation | Nullable | Default | Storage |
Compression | Stats target | Description
--------+------+-----------+----------+---------+---------+-------------+--------------+-------------
 a      | text |           |          |         | plain   |
 |              |
Child tables: c1
Access method: heap
#\d+ c1
                                          Table "public.c1"
 Column | Type | Collation | Nullable | Default | Storage |
Compression | Stats target | Description
--------+------+-----------+----------+---------+---------+-------------+--------------+-------------
 a      | text |           |          |         | plain   |
 |              |
Inherits: t1
Access method: heap

Observe that c1.a did not have storage "main" but instead inherits
"plain" from t1.

According to the code at
https://github.com/postgres/postgres/blob/6a1ea02c491d16474a6214603dce40b5b122d4d1/src/backend/commands/tablecmds.c#L3253,
there is supposed to be a conflict error. But that does not happen
since child's storage specification is in ColumnDef::storage_name
which is never consulted. The ColumnDef::storage_name is converted to
ColumnDef::storage only in BuildDescForRelation(), after
MergeAttribute() has been finished. There are two ways to fix this
1. In MergeChildAttribute() resolve ColumnDef::storage_name to
ColumnDef::storage before comparing it against inherited property. I
don't like this approach since a. we duplicate the conversion logic in
MergeChildAttribute() and BuildDescForRelation(), b. the conversion
happens only for the attributes which are inherited.

2. Deal with it the same way as compression. Get rid of
ColumnDef::storage altogether. Instead set ColumnDef::storage_name at
https://github.com/postgres/postgres/blob/6a1ea02c491d16474a6214603dce40b5b122d4d1/src/backend/commands/tablecmds.c#L2723.

I am inclined to take the second approach. Let me know if you feel otherwise.

[1] https://www.postgresql.org/message-id/[email-masked]

-- 
Best Wishes,
Ashutosh Bapat

---

From: Ashutosh Bapat <[email-masked]>
Date: Wed, 7 Feb 2024 12:47:04 +0530

> Compression | Stats target | Description

Took the second approach. PFA patches
0001 fixes compression inheritance
0002 fixes storage inheritance

The patches may be committed separately or as a single patch. Keeping
them separate in case we decide to commit one but not the other.

We always set storage even if it's not specified, in which case the
column type's default storage is used. This is slightly different from
compression which defaults to the GUC's value if not set. This has led
to slight difference in the tests.

-- 
Best Wishes,
Ashutosh Bapat
From 4d7ba689782d2dfd142ec0f27095ee386543f761 Mon Sep 17 00:00:00 2001
From: Ashutosh Bapat <[email-masked]>
Date: Thu, 1 Feb 2024 12:58:36 +0530
Subject: [PATCH 2/2] Column storage and inheritance

The storage method specified while creating a child table overrides any
parent storage method, even if parents have conflicting storage methods.
If the child doesn't specify a storage method and all its parents use
the same storage method, the child inherits parents' storage method. We
don't allow a child without storage specification when its parents use
conflicting storage methods.

The storage property remains unchanged in a child inheriting from
parent/s after its creation i.e. using ALTER TABLE ... INHERIT.

NOTEs to reviewer (may be included in the final commit message.)

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 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.

Before this commit the error detail would mention the first pair of
conflicting parent storage methods. But with this commit 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 commit does not report conflicting storage methods. Those can be
obtained from parent definitions if necessary. The code to maintain list
of all conflicting methods or that to remember even just the first pair
does not seem worth the effort. This change is inline with what we do
with conflicting default values.

Author: Ashutosh Bapat
Discussion: https://www.postgresql.org/message-id/flat/[email-masked]
---
 doc/src/sgml/ref/create_table.sgml            |  5 +-
 src/backend/catalog/pg_type.c                 | 22 +++++
 src/backend/commands/tablecmds.c              | 86 +++++++------------
 src/backend/nodes/makefuncs.c                 |  2 +-
 src/backend/parser/gram.y                     |  7 +-
 src/backend/parser/parse_utilcmd.c            |  4 +-
 src/include/catalog/pg_type.h                 |  2 +
 src/include/nodes/parsenodes.h                |  3 +-
 .../regress/expected/create_table_like.out    | 12 +--
 src/test/regress/expected/inherit.out         | 38 ++++++++
 src/test/regress/sql/create_table_like.sql    |  2 +-
 src/test/regress/sql/inherit.sql              | 20 +++++
 12 files changed, 131 insertions(+), 72 deletions(-)

diff --git a/doc/src/sgml/ref/create_table.sgml b/doc/src/sgml/ref/create_table.sgml
index 3e5e4b107e..e676a6098c 100644
--- a/doc/src/sgml/ref/create_table.sgml
+++ b/doc/src/sgml/ref/create_table.sgml
@@ -398,7 +398,10 @@ WITH ( MODULUS <replaceable class="parameter">numeric_literal</replaceable>, REM
      </para>
 
      <para>
-      Column <literal>STORAGE</literal> settings are also copied from parent tables.
+      Explicitly specified Column <literal>STORAGE</literal> settings overrides
+      those inherited from parent tables. Otherwise all the parents should have
+      the same Column <literal>STORAGE</literal> setting which will be
+      inherited by the column in the new table, or an error will be reported.
      </para>
 
      <para>
diff --git a/src/backend/catalog/pg_type.c b/src/backend/catalog/pg_type.c
index d7167108fb..cfa3220ea5 100644
--- a/src/backend/catalog/pg_type.c
+++ b/src/backend/catalog/pg_type.c
@@ -957,3 +957,25 @@ 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:
+			return NULL;
+	}
+}
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index dc1e97d3f4..ac4075b988 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -351,12 +351,14 @@ typedef struct ForeignTruncateInfo
 	((child_is_partition) ? DEPENDENCY_AUTO : DEPENDENCY_NORMAL)
 
 /*
- * Bogus compression method name to track conflict in inherited compression
- * property of attributes. It can be any string which does not look like a valid
- * compression method. It is meant to be used by MergeAttributes() and its
- * minions. It is not expected to be stored on disk.
+ * 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.
  */
-#define BOGUS_COMPRESSION_METHOD "*** conflicting compression ***"
+#define 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);
@@ -629,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);
@@ -1372,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);
 	}
 
@@ -2397,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.
@@ -2729,7 +2706,7 @@ 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 =
@@ -3114,12 +3091,20 @@ MergeAttributes(List *columns, const List *supers, char relpersistence,
 			}
 
 			if (def->compression != NULL &&
-				strcmp(def->compression, BOGUS_COMPRESSION_METHOD) == 0)
+				strcmp(def->compression, CONFLICTING_COLUMN_PROPERTY) == 0)
 				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 != NULL &&
+				strcmp(def->storage_name, CONFLICTING_COLUMN_PROPERTY) == 0)
+				ereport(ERROR,
+						(errcode(ERRCODE_DATATYPE_MISMATCH),
+						 errmsg("column \"%s\" inherits conflicting storgae methods",
+								def->colname),
+						 errhint("To resolve the conflict, specify a storage method explicitly.")));
 		}
 	}
 
@@ -3269,18 +3254,11 @@ MergeChildAttribute(List *inh_columns, int exist_attno, int newcol_attno, const
 	inhdef->identity = newdef->identity;
 
 	/*
-	 * Copy storage parameter
+	 * Child storgage 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;
 
 	/*
 	 * Child compression specification, if any, overrides inherited
@@ -3419,16 +3397,14 @@ 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
@@ -3439,7 +3415,7 @@ MergeInheritedAttribute(List *inh_columns,
 			 strcmp(prevdef->compression, newdef->compression) != 0)
 	{
 		/* Note the conflict. */
-		prevdef->compression = BOGUS_COMPRESSION_METHOD;
+		prevdef->compression = CONFLICTING_COLUMN_PROPERTY;
 		*have_deferred_conflicts = true;
 	}
 
diff --git a/src/backend/nodes/makefuncs.c b/src/backend/nodes/makefuncs.c
index a02332a1ec..86b8dcfe7e 100644
--- a/src/backend/nodes/makefuncs.c
+++ b/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;
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 130f7fc7c3..60b31d9f85 100644
--- a/src/backend/parser/gram.y
+++ b/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;
diff --git a/src/backend/parser/parse_utilcmd.c b/src/backend/parser/parse_utilcmd.c
index 56ac4f516e..62ce9b8c24 100644
--- a/src/backend/parser/parse_utilcmd.c
+++ b/src/backend/parser/parse_utilcmd.c
@@ -1125,9 +1125,9 @@ 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
diff --git a/src/include/catalog/pg_type.h b/src/include/catalog/pg_type.h
index e925969732..38ca117480 100644
--- a/src/include/catalog/pg_type.h
+++ b/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 */
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index 476d55dd24..685a7819d5 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -717,8 +717,7 @@ typedef struct ColumnDef
 	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 */
diff --git a/src/test/regress/expected/create_table_like.out b/src/test/regress/expected/create_table_like.out
index 61956773ff..66f1d91c81 100644
--- a/src/test/regress/expected/create_table_like.out
+++ b/src/test/regress/expected/create_table_like.out
@@ -445,12 +445,10 @@ SELECT s.stxname, objsubid, description FROM pg_description, pg_statistic_ext s
 
 CREATE TABLE inh_error1 () INHERITS (ctlt1, ctlt4);
 NOTICE:  merging multiple inherited definitions of column "a"
-ERROR:  inherited column "a" has a storage parameter conflict
-DETAIL:  MAIN versus EXTENDED
-CREATE TABLE inh_error2 (LIKE ctlt4 INCLUDING STORAGE) INHERITS (ctlt1);
+ERROR:  column "a" inherits conflicting storgae methods
+HINT:  To resolve the conflict, specify a storage method explicitly.
+CREATE TABLE ctlt14_inh_like (LIKE ctlt4 INCLUDING STORAGE) INHERITS (ctlt1);
 NOTICE:  merging column "a" with inherited definition
-ERROR:  column "a" has a storage parameter conflict
-DETAIL:  MAIN versus EXTENDED
 -- Check that LIKE isn't confused by a system catalog of the same name
 CREATE TABLE pg_attrdef (LIKE ctlt1 INCLUDING ALL);
 \d+ public.pg_attrdef
@@ -493,7 +491,9 @@ Statistics objects:
 
 ROLLBACK;
 DROP TABLE ctlt1, ctlt2, ctlt3, ctlt4, ctlt12_storage, ctlt12_comments, ctlt1_inh, ctlt13_inh, ctlt13_like, ctlt_all, ctla, ctlb CASCADE;
-NOTICE:  drop cascades to table inhe
+NOTICE:  drop cascades to 2 other objects
+DETAIL:  drop cascades to table inhe
+drop cascades to table ctlt14_inh_like
 -- LIKE must respect NO INHERIT property of constraints
 CREATE TABLE noinh_con_copy (a int CHECK (a > 0) NO INHERIT);
 CREATE TABLE noinh_con_copy1 (LIKE noinh_con_copy INCLUDING CONSTRAINTS);
diff --git a/src/test/regress/expected/inherit.out b/src/test/regress/expected/inherit.out
index 130a924228..929dad9141 100644
--- a/src/test/regress/expected/inherit.out
+++ b/src/test/regress/expected/inherit.out
@@ -3419,3 +3419,41 @@ UPDATE errtst_parent SET partid = 30, data = data + 10 WHERE partid = 20;
 ERROR:  no partition of relation "errtst_parent" found for row
 DETAIL:  Partition key of the failing row contains (partid) = (30).
 DROP TABLE errtst_parent;
+-- storage and inheritance
+CREATE TABLE stparent1 (a TEXT STORAGE plain);
+CREATE TABLE stparent2 (a TEXT);
+-- child inherits parent's storage properties, if they do not conflict
+CREATE TABLE stchild1 (a TEXT) INHERITS (stparent1);
+NOTICE:  merging column "a" with inherited definition
+CREATE TABLE stchild2 (a TEXT) INHERITS (stparent1, stparent2);
+NOTICE:  merging multiple inherited definitions of column "a"
+NOTICE:  merging column "a" with inherited definition
+ERROR:  column "a" inherits conflicting storgae methods
+HINT:  To resolve the conflict, specify a storage method explicitly.
+-- child overrides parent's storage properties even if they conflict
+CREATE TABLE stchild3 (a TEXT STORAGE main) INHERITS (stparent1);
+NOTICE:  merging column "a" with inherited definition
+CREATE TABLE stchild4 (a TEXT STORAGE main) INHERITS (stparent1, stparent2);
+NOTICE:  merging multiple inherited definitions of column "a"
+NOTICE:  merging column "a" with inherited definition
+-- child storage properties are not changed when inheriting after creation.
+CREATE TABLE stchild5 (a TEXT);
+ALTER TABLE stchild5 INHERIT stparent1;
+CREATE TABLE stchild6 (a TEXT STORAGE main);
+ALTER TABLE stchild6 INHERIT stparent1;
+SELECT attrelid::regclass, attname, attstorage FROM pg_attribute
+  WHERE (attrelid::regclass::name like 'stparent%'
+         OR attrelid::regclass::name like 'stchild%')
+        and attname = 'a'
+  ORDER BY 1, 2;
+ attrelid  | attname | attstorage 
+-----------+---------+------------
+ stparent1 | a       | p
+ stparent2 | a       | x
+ stchild1  | a       | p
+ stchild3  | a       | m
+ stchild4  | a       | m
+ stchild5  | a       | x
+ stchild6  | a       | m
+(7 rows)
+
diff --git a/src/test/regress/sql/create_table_like.sql b/src/test/regress/sql/create_table_like.sql
index 4929d373a2..9c5758a72b 100644
--- a/src/test/regress/sql/create_table_like.sql
+++ b/src/test/regress/sql/create_table_like.sql
@@ -168,7 +168,7 @@ SELECT c.relname, objsubid, description FROM pg_description, pg_index i, pg_clas
 SELECT s.stxname, objsubid, description FROM pg_description, pg_statistic_ext s WHERE classoid = 'pg_statistic_ext'::regclass AND objoid = s.oid AND s.stxrelid = 'ctlt_all'::regclass ORDER BY s.stxname, objsubid;
 
 CREATE TABLE inh_error1 () INHERITS (ctlt1, ctlt4);
-CREATE TABLE inh_error2 (LIKE ctlt4 INCLUDING STORAGE) INHERITS (ctlt1);
+CREATE TABLE ctlt14_inh_like (LIKE ctlt4 INCLUDING STORAGE) INHERITS (ctlt1);
 
 -- Check that LIKE isn't confused by a system catalog of the same name
 CREATE TABLE pg_attrdef (LIKE ctlt1 INCLUDING ALL);
diff --git a/src/test/regress/sql/inherit.sql b/src/test/regress/sql/inherit.sql
index 0ef9a61bc1..1965ede282 100644
--- a/src/test/regress/sql/inherit.sql
+++ b/src/test/regress/sql/inherit.sql
@@ -1354,3 +1354,23 @@ UPDATE errtst_parent SET partid = 0, data = data + 10 WHERE partid = 20;
 UPDATE errtst_parent SET partid = 30, data = data + 10 WHERE partid = 20;
 
 DROP TABLE errtst_parent;
+
+-- storage and inheritance
+CREATE TABLE stparent1 (a TEXT STORAGE plain);
+CREATE TABLE stparent2 (a TEXT);
+-- child inherits parent's storage properties, if they do not conflict
+CREATE TABLE stchild1 (a TEXT) INHERITS (stparent1);
+CREATE TABLE stchild2 (a TEXT) INHERITS (stparent1, stparent2);
+-- child overrides parent's storage properties even if they conflict
+CREATE TABLE stchild3 (a TEXT STORAGE main) INHERITS (stparent1);
+CREATE TABLE stchild4 (a TEXT STORAGE main) INHERITS (stparent1, stparent2);
+-- child storage properties are not changed when inheriting after creation.
+CREATE TABLE stchild5 (a TEXT);
+ALTER TABLE stchild5 INHERIT stparent1;
+CREATE TABLE stchild6 (a TEXT STORAGE main);
+ALTER TABLE stchild6 INHERIT stparent1;
+SELECT attrelid::regclass, attname, attstorage FROM pg_attribute
+  WHERE (attrelid::regclass::name like 'stparent%'
+         OR attrelid::regclass::name like 'stchild%')
+        and attname = 'a'
+  ORDER BY 1, 2;
\ No newline at end of file
-- 
2.25.1

From a5c1ccc5dbbdbb4936dcb97b7fc6998b87e0da2c Mon Sep 17 00:00:00 2001
From: Ashutosh Bapat <[email-masked]>
Date: Thu, 25 Jan 2024 15:50:16 +0530
Subject: [PATCH 1/2] Compression method support with inheritance

The compression method specified while creating a child table overrides
any parent compression methods, even if they conflict. If child doesn't
specify a compression method and all its parents use the same
compression method, the child inherits parents' compression method.  A
child must specify a compression method when its parents use conflicting
compression methods.

The compression property, even if empty, remains unchanged in a child
inheriting from parent/s after its creation i.e. when using ALTER TABLE
...  INHERIT.

NOTE To the reviewer (may be included in the final commit message if
necessary):
Before this change the error detail would mention the first pair of
conflicting parent compression method. 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 report conflicting compression 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.

Author: Ashutosh Bapat
Discussion: https://www.postgresql.org/message-id/flat/[email-masked]
---
 doc/src/sgml/ref/create_table.sgml        |   8 ++
 src/backend/commands/tablecmds.c          |  67 ++++++++------
 src/test/regress/expected/compression.out | 101 ++++++++++++++++++++--
 src/test/regress/sql/compression.sql      |  43 ++++++++-
 4 files changed, 184 insertions(+), 35 deletions(-)

diff --git a/doc/src/sgml/ref/create_table.sgml b/doc/src/sgml/ref/create_table.sgml
index 4cbaaccaf7..3e5e4b107e 100644
--- a/doc/src/sgml/ref/create_table.sgml
+++ b/doc/src/sgml/ref/create_table.sgml
@@ -401,6 +401,14 @@ WITH ( MODULUS <replaceable class="parameter">numeric_literal</replaceable>, REM
       Column <literal>STORAGE</literal> settings are also copied from parent tables.
      </para>
 
+     <para>
+      Explicitly specified <literal>COMPRESSION</literal> settings of a column
+      override any inherited <literal>COMPRESSION</literal> settings of that
+      column. Otheriwse any parents that specify <literal>COMPRESSION</literal>
+      settings for the column must all specify the same settings, which will be
+      inherited by the column in the new table, or an error will be reported.
+     </para>
+
      <para>
       If a column in the parent table is an identity column, that property is
       not inherited.  A column in the child table can be declared identity
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 9f51696740..dc1e97d3f4 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -350,6 +350,14 @@ typedef struct ForeignTruncateInfo
 #define child_dependency_type(child_is_partition)	\
 	((child_is_partition) ? DEPENDENCY_AUTO : DEPENDENCY_NORMAL)
 
+/*
+ * Bogus compression method name to track conflict in inherited compression
+ * property of attributes. It can be any string which does not look like a valid
+ * compression method. It is meant to be used by MergeAttributes() and its
+ * minions. It is not expected to be stored on disk.
+ */
+#define BOGUS_COMPRESSION_METHOD "*** conflicting compression ***"
+
 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 +368,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,
@@ -2483,7 +2492,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;
@@ -2744,7 +2753,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 +2877,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 +3087,11 @@ 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
+	 * compression properties, check to make sure they were overridden by the
+	 * child.
 	 */
-	if (have_bogus_defaults)
+	if (have_deferred_conflicts)
 	{
 		foreach(lc, columns)
 		{
@@ -3101,6 +3112,14 @@ MergeAttributes(List *columns, const List *supers, char relpersistence,
 									def->colname),
 							 errhint("To resolve the conflict, specify a default explicitly.")));
 			}
+
+			if (def->compression != NULL &&
+				strcmp(def->compression, BOGUS_COMPRESSION_METHOD) == 0)
+				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.")));
 		}
 	}
 
@@ -3264,19 +3283,11 @@ MergeChildAttribute(List *inh_columns, int exist_attno, int newcol_attno, const
 						   storage_name(newdef->storage))));
 
 	/*
-	 * 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 +3354,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 +3371,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;
@@ -3419,12 +3435,13 @@ MergeInheritedAttribute(List *inh_columns,
 	 */
 	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)
+	{
+		/* Note the conflict. */
+		prevdef->compression = BOGUS_COMPRESSION_METHOD;
+		*have_deferred_conflicts = true;
+	}
 
 	/*
 	 * Check for GENERATED conflicts
diff --git a/src/test/regress/expected/compression.out b/src/test/regress/expected/compression.out
index 834b7555cb..c22a71f8bd 100644
--- a/src/test/regress/expected/compression.out
+++ b/src/test/regress/expected/compression.out
@@ -223,15 +223,102 @@ SELECT pg_column_compression(f1) FROM cmpart2;
  pglz
 (1 row)
 
--- test compression with inheritance, error
-CREATE TABLE cminh() INHERITS(cmdata, cmdata1);
+-- test compression with inheritance
+CREATE TABLE cmparent1 (f1 TEXT COMPRESSION pglz);
+INSERT INTO cmparent1 VALUES ('cmparent1_' || repeat('1234567890', 1000));
+CREATE TABLE cmparent2 (f1 TEXT COMPRESSION lz4);
+INSERT INTO cmparent2 VALUES ('cmparent2_' || repeat('1234567890', 1000));
+CREATE TABLE ncmparent (f1 TEXT); -- parent without compression
+INSERT INTO ncmparent VALUES ('ncmparent_' || repeat('1234567890', 1000));
+CREATE TABLE cminh1(f1 TEXT) INHERITS(cmparent1);
+NOTICE:  merging column "f1" with inherited definition
+INSERT INTO cminh1 VALUES ('cminh1_' || repeat('1234567890', 1000));
+CREATE TABLE cminh2(f1 TEXT) INHERITS(ncmparent, cmparent1);
+NOTICE:  merging multiple inherited definitions of column "f1"
+NOTICE:  merging column "f1" with inherited definition
+INSERT INTO cminh2 VALUES ('cminh2_' || repeat('1234567890', 1000));
+CREATE TABLE cminh3(f1 TEXT) INHERITS(cmparent1, ncmparent);
+NOTICE:  merging multiple inherited definitions of column "f1"
+NOTICE:  merging column "f1" with inherited definition
+INSERT INTO cminh3 VALUES ('cminh3_' || repeat('1234567890', 1000));
+-- conflicting compression methods from parents
+CREATE TABLE cminh() INHERITS(cmparent1, cmparent2); --error
+NOTICE:  merging multiple inherited definitions of column "f1"
+ERROR:  column "f1" inherits conflicting compression methods
+HINT:  To resolve the conflict, specify a compression method explicitly.
+CREATE TABLE cminh(f1 TEXT) INHERITS(cmparent1, cmparent2); --error
 NOTICE:  merging multiple inherited definitions of column "f1"
-ERROR:  column "f1" has a compression method conflict
-DETAIL:  pglz versus lz4
-CREATE TABLE cminh(f1 TEXT COMPRESSION lz4) INHERITS(cmdata);
 NOTICE:  merging column "f1" with inherited definition
-ERROR:  column "f1" has a compression method conflict
-DETAIL:  pglz versus lz4
+ERROR:  column "f1" inherits conflicting compression methods
+HINT:  To resolve the conflict, specify a compression method explicitly.
+-- child compression specification takes precedence, even if parent's
+-- compression conflict
+CREATE TABLE cminh4(f1 TEXT COMPRESSION lz4) INHERITS(cmparent1);
+NOTICE:  merging column "f1" with inherited definition
+INSERT INTO cminh4 VALUES ('cminh4_' || repeat('1234567890', 1000));
+CREATE TABLE cminh5(f1 TEXT COMPRESSION pglz) INHERITS(cmparent1, cmparent2);
+NOTICE:  merging multiple inherited definitions of column "f1"
+NOTICE:  merging column "f1" with inherited definition
+INSERT INTO cminh5 VALUES ('cminh5_' || repeat('1234567890', 1000));
+SELECT tableoid::regclass, pg_column_compression(f1) FROM cmparent1;
+ tableoid  | pg_column_compression 
+-----------+-----------------------
+ cmparent1 | pglz
+ cminh1    | pglz
+ cminh2    | pglz
+ cminh3    | pglz
+ cminh4    | lz4
+ cminh5    | pglz
+(6 rows)
+
+SELECT tableoid::regclass, pg_column_compression(f1) FROM cmparent2;
+ tableoid  | pg_column_compression 
+-----------+-----------------------
+ cmparent2 | lz4
+ cminh5    | pglz
+(2 rows)
+
+SELECT tableoid::regclass, pg_column_compression(f1) FROM ncmparent;
+ tableoid  | pg_column_compression 
+-----------+-----------------------
+ ncmparent | pglz
+ cminh2    | pglz
+ cminh3    | pglz
+(3 rows)
+
+-- ALTER compression specification in child
+ALTER TABLE cminh1 ALTER COLUMN f1 SET COMPRESSION lz4;
+INSERT INTO cminh1 VALUES ('cminh1_lz4_' || repeat('1234567890', 1000));
+SELECT pg_column_compression(f1) FROM cminh1;
+ pg_column_compression 
+-----------------------
+ pglz
+ lz4
+(2 rows)
+
+-- INHERIT through ALTER TABLE
+CREATE TABLE cminh6 (f1 TEXT);
+INSERT INTO cminh6 VALUES ('cminh6_' || repeat('1234567890', 1000));
+ALTER TABLE cminh6 INHERIT cmparent1;
+INSERT INTO cminh6 VALUES ('cminh6_inh_' || repeat('1234567890', 1000));
+SELECT pg_column_compression(f1) FROM cminh6;
+ pg_column_compression 
+-----------------------
+ pglz
+ pglz
+(2 rows)
+
+CREATE TABLE cminh7 (f1 TEXT COMPRESSION lz4);
+INSERT INTO cminh7 VALUES ('cminh7_' || repeat('1234567890', 1000));
+ALTER TABLE cminh7 INHERIT cmparent1;
+INSERT INTO cminh7 VALUES ('cminh7_inh_' || repeat('1234567890', 1000));
+SELECT pg_column_compression(f1) FROM cminh7;
+ pg_column_compression 
+-----------------------
+ lz4
+ lz4
+(2 rows)
+
 -- test default_toast_compression GUC
 SET default_toast_compression = '';
 ERROR:  invalid value for parameter "default_toast_compression": ""
diff --git a/src/test/regress/sql/compression.sql b/src/test/regress/sql/compression.sql
index 7179a5002e..ad8e7afd2a 100644
--- a/src/test/regress/sql/compression.sql
+++ b/src/test/regress/sql/compression.sql
@@ -93,9 +93,46 @@ INSERT INTO cmpart VALUES (repeat('123456789', 4004));
 SELECT pg_column_compression(f1) FROM cmpart1;
 SELECT pg_column_compression(f1) FROM cmpart2;
 
--- test compression with inheritance, error
-CREATE TABLE cminh() INHERITS(cmdata, cmdata1);
-CREATE TABLE cminh(f1 TEXT COMPRESSION lz4) INHERITS(cmdata);
+-- test compression with inheritance
+CREATE TABLE cmparent1 (f1 TEXT COMPRESSION pglz);
+INSERT INTO cmparent1 VALUES ('cmparent1_' || repeat('1234567890', 1000));
+CREATE TABLE cmparent2 (f1 TEXT COMPRESSION lz4);
+INSERT INTO cmparent2 VALUES ('cmparent2_' || repeat('1234567890', 1000));
+CREATE TABLE ncmparent (f1 TEXT); -- parent without compression
+INSERT INTO ncmparent VALUES ('ncmparent_' || repeat('1234567890', 1000));
+CREATE TABLE cminh1(f1 TEXT) INHERITS(cmparent1);
+INSERT INTO cminh1 VALUES ('cminh1_' || repeat('1234567890', 1000));
+CREATE TABLE cminh2(f1 TEXT) INHERITS(ncmparent, cmparent1);
+INSERT INTO cminh2 VALUES ('cminh2_' || repeat('1234567890', 1000));
+CREATE TABLE cminh3(f1 TEXT) INHERITS(cmparent1, ncmparent);
+INSERT INTO cminh3 VALUES ('cminh3_' || repeat('1234567890', 1000));
+-- conflicting compression methods from parents
+CREATE TABLE cminh() INHERITS(cmparent1, cmparent2); --error
+CREATE TABLE cminh(f1 TEXT) INHERITS(cmparent1, cmparent2); --error
+-- child compression specification takes precedence, even if parent's
+-- compression conflict
+CREATE TABLE cminh4(f1 TEXT COMPRESSION lz4) INHERITS(cmparent1);
+INSERT INTO cminh4 VALUES ('cminh4_' || repeat('1234567890', 1000));
+CREATE TABLE cminh5(f1 TEXT COMPRESSION pglz) INHERITS(cmparent1, cmparent2);
+INSERT INTO cminh5 VALUES ('cminh5_' || repeat('1234567890', 1000));
+SELECT tableoid::regclass, pg_column_compression(f1) FROM cmparent1;
+SELECT tableoid::regclass, pg_column_compression(f1) FROM cmparent2;
+SELECT tableoid::regclass, pg_column_compression(f1) FROM ncmparent;
+-- ALTER compression specification in child
+ALTER TABLE cminh1 ALTER COLUMN f1 SET COMPRESSION lz4;
+INSERT INTO cminh1 VALUES ('cminh1_lz4_' || repeat('1234567890', 1000));
+SELECT pg_column_compression(f1) FROM cminh1;
+-- INHERIT through ALTER TABLE
+CREATE TABLE cminh6 (f1 TEXT);
+INSERT INTO cminh6 VALUES ('cminh6_' || repeat('1234567890', 1000));
+ALTER TABLE cminh6 INHERIT cmparent1;
+INSERT INTO cminh6 VALUES ('cminh6_inh_' || repeat('1234567890', 1000));
+SELECT pg_column_compression(f1) FROM cminh6;
+CREATE TABLE cminh7 (f1 TEXT COMPRESSION lz4);
+INSERT INTO cminh7 VALUES ('cminh7_' || repeat('1234567890', 1000));
+ALTER TABLE cminh7 INHERIT cmparent1;
+INSERT INTO cminh7 VALUES ('cminh7_inh_' || repeat('1234567890', 1000));
+SELECT pg_column_compression(f1) FROM cminh7;
 
 -- test default_toast_compression GUC
 SET default_toast_compression = '';
-- 
2.25.1

---

From: Ashutosh Bapat <[email-masked]>
Date: Thu, 8 Feb 2024 12:50:12 +0530

> 0002 fixes storage inheritance

The first patch does not update compression_1.out which makes CI
unhappy. Here's patchset fixing that.

-- 
Best Wishes,
Ashutosh Bapat
From 629db9289371d3e72dc6b0d43811fb6decd94525 Mon Sep 17 00:00:00 2001
From: Ashutosh Bapat <[email-masked]>
Date: Thu, 1 Feb 2024 12:58:36 +0530
Subject: [PATCH 2/2] Column storage and inheritance

Author: Ashutosh Bapat

      <para>
 
      <para>
 
 	return pstrdup(buf);
 }
+
+/*
 
 /*

 

-/*
- * storage_name

 
 	/*
 
 	/*
 
 	/*

 

 
 ROLLBACK;

 

From b6885f3d1814d7aa777cd067e62ea55fd91e3b16 Mon Sep 17 00:00:00 2001
From: Ashutosh Bapat <[email-masked]>
Date: Thu, 25 Jan 2024 15:50:16 +0530
Subject: [PATCH 1/2] Compression method support with inheritance

Author: Ashutosh Bapat
Discussion: https://www.postgresql.org/message-id/flat/[email-masked]
---
 doc/src/sgml/ref/create_table.sgml          |   8 ++
 src/backend/commands/tablecmds.c            |  67 +++++++-----
 src/test/regress/expected/compression.out   | 101 ++++++++++++++++--
 src/test/regress/expected/compression_1.out | 111 ++++++++++++++++++--
 src/test/regress/sql/compression.sql        |  43 +++++++-
 5 files changed, 289 insertions(+), 41 deletions(-)

+     <para>
 
+/*

 
 
 	/*

 	/*
 
 	/*
 
 	/*
 
diff --git a/src/test/regress/expected/compression_1.out b/src/test/regress/expected/compression_1.out
index ddcd137c49..d70155d5d0 100644
--- a/src/test/regress/expected/compression_1.out
+++ b/src/test/regress/expected/compression_1.out
@@ -216,13 +216,112 @@ SELECT pg_column_compression(f1) FROM cmpart2;
 -----------------------
 (0 rows)
 
-ERROR:  relation "cmdata1" does not exist
+ERROR:  compression method lz4 not supported
+DETAIL:  This functionality requires the server to be built with lz4 support.
+ERROR:  relation "cmparent2" does not exist
+LINE 1: INSERT INTO cmparent2 VALUES ('cmparent2_' || repeat('123456...
+                    ^
+ERROR:  relation "cmparent2" does not exist
+ERROR:  relation "cmparent2" does not exist
+ERROR:  compression method lz4 not supported
+DETAIL:  This functionality requires the server to be built with lz4 support.
+ERROR:  relation "cminh4" does not exist
+LINE 1: INSERT INTO cminh4 VALUES ('cminh4_' || repeat('1234567890',...
+                    ^
+ERROR:  relation "cmparent2" does not exist
+ERROR:  relation "cminh5" does not exist
+LINE 1: INSERT INTO cminh5 VALUES ('cminh5_' || repeat('1234567890',...
+                    ^
+(4 rows)
+ERROR:  relation "cmparent2" does not exist
+LINE 1: ...ableoid::regclass, pg_column_compression(f1) FROM cmparent2;
+                                                             ^
+ERROR:  compression method lz4 not supported
+DETAIL:  This functionality requires the server to be built with lz4 support.
+ERROR:  compression method lz4 not supported
+DETAIL:  This functionality requires the server to be built with lz4 support.
+ERROR:  relation "cminh7" does not exist
+LINE 1: INSERT INTO cminh7 VALUES ('cminh7_' || repeat('1234567890',...
+                    ^
+ERROR:  relation "cminh7" does not exist
+ERROR:  relation "cminh7" does not exist
+LINE 1: INSERT INTO cminh7 VALUES ('cminh7_inh_' || repeat('12345678...
+                    ^
+ERROR:  relation "cminh7" does not exist
+LINE 1: SELECT pg_column_compression(f1) FROM cminh7;
+                                              ^

---

From: Peter Eisentraut <[email-masked]>
Date: Mon, 12 Feb 2024 16:18:12 +0100

> unhappy. Here's patchset fixing that.

The changed behavior looks good to me.  The tests are good, the code 
changes are pretty straightforward.

Did you by any change check that pg_dump dumps the resulting structures 
correctly?  I notice in tablecmds.c that ALTER COLUMN SET STORAGE 
recurses but ALTER COLUMN SET COMPRESSION does not.  I don't understand 
why that is, and I wonder whether it affects pg_dump.

---

From: Ashutosh Bapat <[email-masked]>
Date: Tue, 13 Feb 2024 18:19:24 +0530

> why that is, and I wonder whether it affects pg_dump.

I used src/bin/pg_upgrade/t/002_pg_upgrade.pl to test dump and restore
by leaving back the new objects created in compression.sql and
inherit.sql.

COMPRESSION is set using ALTER TABLE ONLY so it affects only the
parent and should not propagate to children. A child inherits the
parent first and then changes compression property. For example
```
CREATE TABLE public.cmparent1 (
    f1 text
);
ALTER TABLE ONLY public.cmparent1 ALTER COLUMN f1 SET COMPRESSION pglz;

CREATE TABLE public.cminh1 (
    f1 text
)
INHERITS (public.cmparent1);
ALTER TABLE ONLY public.cminh1 ALTER COLUMN f1 SET COMPRESSION lz4;
```

Same is true with the STORAGE parameter. Example
```
CREATE TABLE public.stparent1 (
    a text
);
ALTER TABLE ONLY public.stparent1 ALTER COLUMN a SET STORAGE PLAIN;

CREATE TABLE public.stchild1 (
    a text
)
INHERITS (public.stparent1);
ALTER TABLE ONLY public.stchild1 ALTER COLUMN a SET STORAGE PLAIN;
```

I don't think pg_dump would be affected by the difference you noted.

-- 
Best Wishes,
Ashutosh Bapat

---

From: Peter Eisentraut <[email-masked]>
Date: Fri, 16 Feb 2024 14:18:08 +0100

I have committed this.  It is great to get this behavior fixed and also 
to get the internals more consistent.  Thanks.