table inheritance versus column compression and storage settings

Started by Peter Eisentrautabout 2 years ago18 messages
#1Peter Eisentraut
peter@eisentraut.org

I found the way that MergeAttributes() handles column compression and
storage settings very weird.

For example, I don't understand the purpose of this error:

create table t1 (a int, b text compression pglz);

create table t1a (b text compression lz4) inherits (t1);
...
ERROR: 42804: column "b" has a compression method conflict
DETAIL: pglz versus lz4

or this:

create table t2 (a int, b text compression lz4);

create table t12 () inherits (t1, t2);
...
ERROR: column "b" has a compression method conflict
DETAIL: pglz versus lz4

And we can't override it in the child, per the first example.

But this works:

create table t1a (a int, b text compression lz4);
alter table t1a inherit t1;

Also, you can change the settings in the child using ALTER TABLE ... SET
COMPRESSION (which is also how pg_dump will represent the above
constructions), so the restrictions at CREATE TABLE time don't seem to
make much sense.

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.

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.
- When inheriting from multiple parents with different settings, an
explicit setting in the child is required.

Thoughts?

#2Ashutosh Bapat
ashutosh.bapat.oss@gmail.com
In reply to: Peter Eisentraut (#1)
Re: table inheritance versus column compression and storage settings

On Mon, Dec 4, 2023 at 4:23 PM Peter Eisentraut <peter@eisentraut.org> 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

#3Peter Eisentraut
peter@eisentraut.org
In reply to: Ashutosh Bapat (#2)
Re: table inheritance versus column compression and storage settings

On 05.12.23 05:26, Ashutosh Bapat wrote:

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

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

#4Ashutosh Bapat
ashutosh.bapat.oss@gmail.com
In reply to: Peter Eisentraut (#3)
Re: table inheritance versus column compression and storage settings

On Tue, Dec 5, 2023 at 6:28 PM Peter Eisentraut <peter@eisentraut.org> wrote:

On 05.12.23 05:26, Ashutosh Bapat wrote:

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

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

PFA patch fixing inheritance and compression. It also fixes a crash
reported in [1]/messages/by-id/b22a6834-aacb-7b18-0424-a3f5fe889667@gmail.com.

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]: /messages/by-id/b22a6834-aacb-7b18-0424-a3f5fe889667@gmail.com

--
Best Wishes,
Ashutosh Bapat

#5Ashutosh Bapat
ashutosh.bapat.oss@gmail.com
In reply to: Ashutosh Bapat (#4)
2 attachment(s)
Re: table inheritance versus column compression and storage settings

On Wed, Jan 31, 2024 at 1:29 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:

On Tue, Dec 5, 2023 at 6:28 PM Peter Eisentraut <peter@eisentraut.org> wrote:

On 05.12.23 05:26, Ashutosh Bapat wrote:

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

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

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.

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

Attachments:

0002-Column-storage-and-inheritance-20240207.patchtext/x-patch; charset=US-ASCII; name=0002-Column-storage-and-inheritance-20240207.patchDownload
From 4d7ba689782d2dfd142ec0f27095ee386543f761 Mon Sep 17 00:00:00 2001
From: Ashutosh Bapat <ashutosh.bapat@enterprisedb.com>
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/24656cec-d6ef-4d15-8b5b-e8dfc9c833a7@eisentraut.org
---
 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

0001-Compression-method-support-with-inheritance-20240207.patchtext/x-patch; charset=US-ASCII; name=0001-Compression-method-support-with-inheritance-20240207.patchDownload
From a5c1ccc5dbbdbb4936dcb97b7fc6998b87e0da2c Mon Sep 17 00:00:00 2001
From: Ashutosh Bapat <ashutosh.bapat@enterprisedb.com>
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/24656cec-d6ef-4d15-8b5b-e8dfc9c833a7@eisentraut.org
---
 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

#6Ashutosh Bapat
ashutosh.bapat.oss@gmail.com
In reply to: Ashutosh Bapat (#5)
2 attachment(s)
Re: table inheritance versus column compression and storage settings

On Wed, Feb 7, 2024 at 12:47 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:

0001 fixes compression inheritance
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

Attachments:

0002-Column-storage-and-inheritance-20240208.patchtext/x-patch; charset=US-ASCII; name=0002-Column-storage-and-inheritance-20240208.patchDownload
From 629db9289371d3e72dc6b0d43811fb6decd94525 Mon Sep 17 00:00:00 2001
From: Ashutosh Bapat <ashutosh.bapat@enterprisedb.com>
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/24656cec-d6ef-4d15-8b5b-e8dfc9c833a7@eisentraut.org
---
 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

0001-Compression-method-support-with-inheritance-20240208.patchtext/x-patch; charset=US-ASCII; name=0001-Compression-method-support-with-inheritance-20240208.patchDownload
From b6885f3d1814d7aa777cd067e62ea55fd91e3b16 Mon Sep 17 00:00:00 2001
From: Ashutosh Bapat <ashutosh.bapat@enterprisedb.com>
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/24656cec-d6ef-4d15-8b5b-e8dfc9c833a7@eisentraut.org
---
 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(-)

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/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)
 
--- test compression with inheritance, error
-CREATE TABLE cminh() INHERITS(cmdata, cmdata1);
-ERROR:  relation "cmdata1" does not exist
-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);
+ERROR:  compression method lz4 not supported
+DETAIL:  This functionality requires the server to be built with lz4 support.
+INSERT INTO cmparent2 VALUES ('cmparent2_' || repeat('1234567890', 1000));
+ERROR:  relation "cmparent2" does not exist
+LINE 1: INSERT INTO cmparent2 VALUES ('cmparent2_' || repeat('123456...
+                    ^
+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
-ERROR:  column "f1" has a compression method conflict
-DETAIL:  pglz versus lz4
+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
+ERROR:  relation "cmparent2" does not exist
+CREATE TABLE cminh(f1 TEXT) INHERITS(cmparent1, cmparent2); --error
+ERROR:  relation "cmparent2" does not exist
+-- 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
+ERROR:  compression method lz4 not supported
+DETAIL:  This functionality requires the server to be built with lz4 support.
+INSERT INTO cminh4 VALUES ('cminh4_' || repeat('1234567890', 1000));
+ERROR:  relation "cminh4" does not exist
+LINE 1: INSERT INTO cminh4 VALUES ('cminh4_' || repeat('1234567890',...
+                    ^
+CREATE TABLE cminh5(f1 TEXT COMPRESSION pglz) INHERITS(cmparent1, cmparent2);
+ERROR:  relation "cmparent2" does not exist
+INSERT INTO cminh5 VALUES ('cminh5_' || repeat('1234567890', 1000));
+ERROR:  relation "cminh5" does not exist
+LINE 1: INSERT INTO cminh5 VALUES ('cminh5_' || repeat('1234567890',...
+                    ^
+SELECT tableoid::regclass, pg_column_compression(f1) FROM cmparent1;
+ tableoid  | pg_column_compression 
+-----------+-----------------------
+ cmparent1 | pglz
+ cminh1    | pglz
+ cminh2    | pglz
+ cminh3    | pglz
+(4 rows)
+
+SELECT tableoid::regclass, pg_column_compression(f1) FROM cmparent2;
+ERROR:  relation "cmparent2" does not exist
+LINE 1: ...ableoid::regclass, pg_column_compression(f1) FROM cmparent2;
+                                                             ^
+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;
+ERROR:  compression method lz4 not supported
+DETAIL:  This functionality requires the server to be built with lz4 support.
+INSERT INTO cminh1 VALUES ('cminh1_lz4_' || repeat('1234567890', 1000));
+SELECT pg_column_compression(f1) FROM cminh1;
+ pg_column_compression 
+-----------------------
+ pglz
+ pglz
+(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);
+ERROR:  compression method lz4 not supported
+DETAIL:  This functionality requires the server to be built with lz4 support.
+INSERT INTO cminh7 VALUES ('cminh7_' || repeat('1234567890', 1000));
+ERROR:  relation "cminh7" does not exist
+LINE 1: INSERT INTO cminh7 VALUES ('cminh7_' || repeat('1234567890',...
+                    ^
+ALTER TABLE cminh7 INHERIT cmparent1;
+ERROR:  relation "cminh7" does not exist
+INSERT INTO cminh7 VALUES ('cminh7_inh_' || repeat('1234567890', 1000));
+ERROR:  relation "cminh7" does not exist
+LINE 1: INSERT INTO cminh7 VALUES ('cminh7_inh_' || repeat('12345678...
+                    ^
+SELECT pg_column_compression(f1) FROM cminh7;
+ERROR:  relation "cminh7" does not exist
+LINE 1: SELECT pg_column_compression(f1) FROM cminh7;
+                                              ^
 -- 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

#7Peter Eisentraut
peter@eisentraut.org
In reply to: Ashutosh Bapat (#6)
Re: table inheritance versus column compression and storage settings

On 08.02.24 08:20, Ashutosh Bapat wrote:

On Wed, Feb 7, 2024 at 12:47 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:

0001 fixes compression inheritance
0002 fixes storage inheritance

The first patch does not update compression_1.out which makes CI
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.

#8Ashutosh Bapat
ashutosh.bapat.oss@gmail.com
In reply to: Peter Eisentraut (#7)
Re: table inheritance versus column compression and storage settings

On Mon, Feb 12, 2024 at 8:48 PM Peter Eisentraut <peter@eisentraut.org> wrote:

On 08.02.24 08:20, Ashutosh Bapat wrote:

On Wed, Feb 7, 2024 at 12:47 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:

0001 fixes compression inheritance
0002 fixes storage inheritance

The first patch does not update compression_1.out which makes CI
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.

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

#9Peter Eisentraut
peter@eisentraut.org
In reply to: Ashutosh Bapat (#8)
Re: table inheritance versus column compression and storage settings

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

#10Tom Lane
tgl@sss.pgh.pa.us
In reply to: Peter Eisentraut (#9)
Re: table inheritance versus column compression and storage settings

Peter Eisentraut <peter@eisentraut.org> writes:

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

I find it surprising that the committed patch does not touch
pg_dump. Is it really true that pg_dump dumps situations with
differing compression/storage settings accurately already?

(Note that it proves little that the pg_upgrade test passes,
since if pg_dump were blind to the settings applicable to a
child table, the second dump would still be blind to them.)

regards, tom lane

#11Tom Lane
tgl@sss.pgh.pa.us
In reply to: Tom Lane (#10)
Re: table inheritance versus column compression and storage settings

I wrote:

I find it surprising that the committed patch does not touch
pg_dump. Is it really true that pg_dump dumps situations with
differing compression/storage settings accurately already?

It's worse than I thought. Run "make installcheck" with
today's HEAD, then:

$ pg_dump -Fc regression >r.dump
$ createdb r2
$ pg_restore -d r2 r.dump
pg_restore: error: could not execute query: ERROR: column "a" inherits conflicting storage methods
HINT: To resolve the conflict, specify a storage method explicitly.
Command was: CREATE TABLE public.stchild4 (
a text
)
INHERITS (public.stparent1, public.stparent2);
ALTER TABLE ONLY public.stchild4 ALTER COLUMN a SET STORAGE MAIN;

pg_restore: error: could not execute query: ERROR: relation "public.stchild4" does not exist
Command was: ALTER TABLE public.stchild4 OWNER TO postgres;

pg_restore: error: could not execute query: ERROR: relation "public.stchild4" does not exist
Command was: COPY public.stchild4 (a) FROM stdin;
pg_restore: warning: errors ignored on restore: 3

What I'd intended to compare was the results of the query added to the
regression tests:

regression=# 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)

r2=# 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
stchild1 | a | p
stchild3 | a | m
stparent2 | a | x
stchild5 | a | p
stchild6 | a | m
(6 rows)

So not only does stchild4 fail to restore altogether, but stchild5
ends with the wrong attstorage.

This patch definitely needs more work.

regards, tom lane

#12Ashutosh Bapat
ashutosh.bapat.oss@gmail.com
In reply to: Tom Lane (#11)
Re: table inheritance versus column compression and storage settings

On Fri, Feb 16, 2024 at 11:54 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

I wrote:

I find it surprising that the committed patch does not touch
pg_dump. Is it really true that pg_dump dumps situations with
differing compression/storage settings accurately already?

It's worse than I thought. Run "make installcheck" with
today's HEAD, then:

$ pg_dump -Fc regression >r.dump
$ createdb r2
$ pg_restore -d r2 r.dump
pg_restore: error: could not execute query: ERROR: column "a" inherits conflicting storage methods
HINT: To resolve the conflict, specify a storage method explicitly.
Command was: CREATE TABLE public.stchild4 (
a text
)
INHERITS (public.stparent1, public.stparent2);
ALTER TABLE ONLY public.stchild4 ALTER COLUMN a SET STORAGE MAIN;

pg_restore: error: could not execute query: ERROR: relation "public.stchild4" does not exist
Command was: ALTER TABLE public.stchild4 OWNER TO postgres;

pg_restore: error: could not execute query: ERROR: relation "public.stchild4" does not exist
Command was: COPY public.stchild4 (a) FROM stdin;
pg_restore: warning: errors ignored on restore: 3

Thanks for the test. Let's call this Problem1. I expected
src/bin/pg_upgrade/t/002_pg_upgrade.pl to fail in this case since it
will execute similar steps as you did. And it actually does, except
that it uses binary-upgrade mode. In that mode, INHERITed tables are
dumped in a different manner
-- For binary upgrade, set up inheritance this way.
ALTER TABLE ONLY "public"."stchild4" INHERIT "public"."stparent1";
ALTER TABLE ONLY "public"."stchild4" INHERIT "public"."stparent2";
... snip ...
ALTER TABLE ONLY "public"."stchild4" ALTER COLUMN "a" SET STORAGE MAIN;

that does not lead to the conflict and pg_upgrade does not fail.

What I'd intended to compare was the results of the query added to the
regression tests:

regression=# 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)

r2=# 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
stchild1 | a | p
stchild3 | a | m
stparent2 | a | x
stchild5 | a | p
stchild6 | a | m
(6 rows)

So not only does stchild4 fail to restore altogether, but stchild5
ends with the wrong attstorage.

With binary-upgrade dump and restore stchild5 gets the correct storage value.

Looks like we need a test which pg_dump s regression database and
restores it without going through pg_upgrade.

I think the fix is easy one. Dump the STORAGE and COMPRESSION clauses
with CREATE TABLE for local attributes. Those for inherited attributes
will be dumped separately.

But that will not fix an existing problem described below. Let's call
it Problem2. With HEAD at commit
57f59396bb51953bb7b957780c7f1b7f67602125 (almost a month back)
$ createdb regression
$ psql -d regression
#create table par1 (a text storage plain);
#create table par2 (a text storage plain);
#create table chld (a text) inherits (par1, par2);
NOTICE: merging multiple inherited definitions of column "a"
NOTICE: merging column "a" with inherited definition
-- parent storages conflict after child creation
#alter table par1 alter column a set storage extended;
#SELECT attrelid::regclass, attname, attstorage FROM pg_attribute
WHERE (attrelid::regclass::name like 'par%'
OR attrelid::regclass::name like 'chld%')
and attname = 'a'
ORDER BY 1, 2;
attrelid | attname | attstorage
----------+---------+------------
par1 | a | x
par2 | a | p
chld | a | x
(3 rows)

$ createdb r2
$ pg_dump -Fc regression > /tmp/r.dump
$ pg_restore -d r2 /tmp/r.dump
pg_restore: error: could not execute query: ERROR: inherited column
"a" has a storage parameter conflict
DETAIL: EXTENDED versus PLAIN
Command was: CREATE TABLE public.chld (
a text
)
INHERITS (public.par1, public.par2);

pg_restore: error: could not execute query: ERROR: relation
"public.chld" does not exist
Command was: ALTER TABLE public.chld OWNER TO ashutosh;

pg_restore: error: could not execute query: ERROR: relation
"public.chld" does not exist
Command was: COPY public.chld (a) FROM stdin;
pg_restore: warning: errors ignored on restore: 3

Fixing this requires that we dump ALTER TABLE ... ALTER COLUMN SET
STORAGE and COMPRESSION commands after all the tables (at least
children) have been created. That seems to break the way we dump the
whole table together right now. OR dump inherited tables like binary
upgrade mode.

--
Best Wishes,
Ashutosh Bapat

#13Peter Eisentraut
peter@eisentraut.org
In reply to: Ashutosh Bapat (#12)
Re: table inheritance versus column compression and storage settings

I have reverted the patch for now (and re-opened the commitfest entry).
We should continue to work on this and see if we can at least try to get
the pg_dump test coverage suitable.

Show quoted text

On 19.02.24 12:34, Ashutosh Bapat wrote:

On Fri, Feb 16, 2024 at 11:54 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

I wrote:

I find it surprising that the committed patch does not touch
pg_dump. Is it really true that pg_dump dumps situations with
differing compression/storage settings accurately already?

It's worse than I thought. Run "make installcheck" with
today's HEAD, then:

$ pg_dump -Fc regression >r.dump
$ createdb r2
$ pg_restore -d r2 r.dump
pg_restore: error: could not execute query: ERROR: column "a" inherits conflicting storage methods
HINT: To resolve the conflict, specify a storage method explicitly.
Command was: CREATE TABLE public.stchild4 (
a text
)
INHERITS (public.stparent1, public.stparent2);
ALTER TABLE ONLY public.stchild4 ALTER COLUMN a SET STORAGE MAIN;

pg_restore: error: could not execute query: ERROR: relation "public.stchild4" does not exist
Command was: ALTER TABLE public.stchild4 OWNER TO postgres;

pg_restore: error: could not execute query: ERROR: relation "public.stchild4" does not exist
Command was: COPY public.stchild4 (a) FROM stdin;
pg_restore: warning: errors ignored on restore: 3

Thanks for the test. Let's call this Problem1. I expected
src/bin/pg_upgrade/t/002_pg_upgrade.pl to fail in this case since it
will execute similar steps as you did. And it actually does, except
that it uses binary-upgrade mode. In that mode, INHERITed tables are
dumped in a different manner
-- For binary upgrade, set up inheritance this way.
ALTER TABLE ONLY "public"."stchild4" INHERIT "public"."stparent1";
ALTER TABLE ONLY "public"."stchild4" INHERIT "public"."stparent2";
... snip ...
ALTER TABLE ONLY "public"."stchild4" ALTER COLUMN "a" SET STORAGE MAIN;

that does not lead to the conflict and pg_upgrade does not fail.

What I'd intended to compare was the results of the query added to the
regression tests:

regression=# 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)

r2=# 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
stchild1 | a | p
stchild3 | a | m
stparent2 | a | x
stchild5 | a | p
stchild6 | a | m
(6 rows)

So not only does stchild4 fail to restore altogether, but stchild5
ends with the wrong attstorage.

With binary-upgrade dump and restore stchild5 gets the correct storage value.

Looks like we need a test which pg_dump s regression database and
restores it without going through pg_upgrade.

I think the fix is easy one. Dump the STORAGE and COMPRESSION clauses
with CREATE TABLE for local attributes. Those for inherited attributes
will be dumped separately.

But that will not fix an existing problem described below. Let's call
it Problem2. With HEAD at commit
57f59396bb51953bb7b957780c7f1b7f67602125 (almost a month back)
$ createdb regression
$ psql -d regression
#create table par1 (a text storage plain);
#create table par2 (a text storage plain);
#create table chld (a text) inherits (par1, par2);
NOTICE: merging multiple inherited definitions of column "a"
NOTICE: merging column "a" with inherited definition
-- parent storages conflict after child creation
#alter table par1 alter column a set storage extended;
#SELECT attrelid::regclass, attname, attstorage FROM pg_attribute
WHERE (attrelid::regclass::name like 'par%'
OR attrelid::regclass::name like 'chld%')
and attname = 'a'
ORDER BY 1, 2;
attrelid | attname | attstorage
----------+---------+------------
par1 | a | x
par2 | a | p
chld | a | x
(3 rows)

$ createdb r2
$ pg_dump -Fc regression > /tmp/r.dump
$ pg_restore -d r2 /tmp/r.dump
pg_restore: error: could not execute query: ERROR: inherited column
"a" has a storage parameter conflict
DETAIL: EXTENDED versus PLAIN
Command was: CREATE TABLE public.chld (
a text
)
INHERITS (public.par1, public.par2);

pg_restore: error: could not execute query: ERROR: relation
"public.chld" does not exist
Command was: ALTER TABLE public.chld OWNER TO ashutosh;

pg_restore: error: could not execute query: ERROR: relation
"public.chld" does not exist
Command was: COPY public.chld (a) FROM stdin;
pg_restore: warning: errors ignored on restore: 3

Fixing this requires that we dump ALTER TABLE ... ALTER COLUMN SET
STORAGE and COMPRESSION commands after all the tables (at least
children) have been created. That seems to break the way we dump the
whole table together right now. OR dump inherited tables like binary
upgrade mode.

#14Ashutosh Bapat
ashutosh.bapat.oss@gmail.com
In reply to: Peter Eisentraut (#13)
Re: table inheritance versus column compression and storage settings

On Tue, Feb 20, 2024 at 3:51 PM Peter Eisentraut <peter@eisentraut.org> wrote:

I have reverted the patch for now (and re-opened the commitfest entry).
We should continue to work on this and see if we can at least try to get
the pg_dump test coverage suitable.

I have started a separate thread for dump/restore test. [1]/messages/by-id/CAExHW5uF5V=Cjecx3_Z=7xfh4rg2Wf61PT+hfquzjBqouRzQJQ@mail.gmail.com.

Using that test, I found an existing bug:
Consider
CREATE TABLE cminh6 (f1 TEXT);
ALTER TABLE cminh6 INHERIT cmparent1;
f1 remains without compression even after inherit per the current code.
But pg_dump dumps it out as
CREATE TABLE cminh6 (f1 TEXT) INHERIT(cmparent1)
Because of this after restoring cminh6::f1 inherits compression of
cmparent1. So before dump cminh6::f1 has no compression and after
restore it has compression.

I am not sure how to fix this. We want inheritance children to have
their on compression. So ALTER TABLE ... INHERIT ... no passing a
compression onto child is fine. CREATE TABLE .... INHERIT ... passing
compression onto the child being created also looks fine since that's
what we do with other attributes. Only solution I see is to introduce
"none" as a special compression method to indicate "no compression"
and store that instead of NULL in attcompression column. That looks
ugly.

Similar is the case with storage.

[1]: /messages/by-id/CAExHW5uF5V=Cjecx3_Z=7xfh4rg2Wf61PT+hfquzjBqouRzQJQ@mail.gmail.com

--
Best Wishes,
Ashutosh Bapat

#15Ashutosh Bapat
ashutosh.bapat.oss@gmail.com
In reply to: Ashutosh Bapat (#14)
Re: table inheritance versus column compression and storage settings

On Fri, Feb 23, 2024 at 6:05 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:

On Tue, Feb 20, 2024 at 3:51 PM Peter Eisentraut <peter@eisentraut.org> wrote:

I have reverted the patch for now (and re-opened the commitfest entry).
We should continue to work on this and see if we can at least try to get
the pg_dump test coverage suitable.

I have started a separate thread for dump/restore test. [1].

Using that test, I found an existing bug:
Consider
CREATE TABLE cminh6 (f1 TEXT);
ALTER TABLE cminh6 INHERIT cmparent1;
f1 remains without compression even after inherit per the current code.
But pg_dump dumps it out as
CREATE TABLE cminh6 (f1 TEXT) INHERIT(cmparent1)
Because of this after restoring cminh6::f1 inherits compression of
cmparent1. So before dump cminh6::f1 has no compression and after
restore it has compression.

I am not sure how to fix this. We want inheritance children to have
their on compression. So ALTER TABLE ... INHERIT ... no passing a
compression onto child is fine. CREATE TABLE .... INHERIT ... passing
compression onto the child being created also looks fine since that's
what we do with other attributes. Only solution I see is to introduce
"none" as a special compression method to indicate "no compression"
and store that instead of NULL in attcompression column. That looks
ugly.

Specifying DEFAULT as COMPRESSION method instead of inventing "none"
works. We should do it only for INHERITed tables.

Similar is the case with storage.

Similar to compression, for inherited tables we have to output STORAGE
clause even if it's default.

--
Best Wishes,
Ashutosh Bapat

#16Ashutosh Bapat
ashutosh.bapat.oss@gmail.com
In reply to: Ashutosh Bapat (#15)
Re: table inheritance versus column compression and storage settings

Hi Peter and Tom,

On Tue, Feb 20, 2024 at 3:51 PM Peter Eisentraut <peter@eisentraut.org>
wrote:

I have reverted the patch for now (and re-opened the commitfest entry).
We should continue to work on this and see if we can at least try to

get

the pg_dump test coverage suitable.

The pg_dump problems arise because we throw an error when parents have
conflicting compression and storage properties. The patch that got
reverted, changed this slightly by allowing a child to override parent's
properties even when they conflict. It still threw an error when child
didn't override and parents conflicted. I guess, MergeAttributes() raises
error when it encounters parents with conflicting properties because it can
not decide which of the conflicting properties the child should inherit.
Instead it could just set the DEFAULT properties when parent properties
conflict but child doesn't override. Thus when compression conflicts,
child's compression would be set to default and when storage conflicts it
will be set to the type's default storage. Child's properties when
specified explicitly would override always. This will solve all the pg_dump
bugs we saw with the reverted patch and also existing bug I reported
earlier.

This change would break backward compatibility but I don't think anybody
would rely on error being thrown when parent properties conflict.

What do you think?

--
Best Wishes,
Ashutosh Bapat

#17Peter Eisentraut
peter@eisentraut.org
In reply to: Ashutosh Bapat (#16)
Re: table inheritance versus column compression and storage settings

On 07.03.24 17:54, Ashutosh Bapat wrote:

The pg_dump problems arise because we throw an error when parents have
conflicting compression and storage properties. The patch that got
reverted, changed this slightly by allowing a child to override parent's
properties even when they conflict. It still threw an error when child
didn't override and parents conflicted. I guess, MergeAttributes()
raises error when it encounters parents with conflicting properties
because it can not decide which of the conflicting properties the child
should inherit. Instead it could just set the DEFAULT properties when
parent properties conflict but child doesn't override. Thus when
compression conflicts, child's compression would be set to default and
when storage conflicts it will be set to the type's default storage.
Child's properties when specified explicitly would override always. This
will solve all the pg_dump bugs we saw with the reverted patch and also
existing bug I reported earlier.

This change would break backward compatibility but I don't think anybody
would rely on error being thrown when parent properties conflict.

What do you think?

At this point in the development cycle, I would rather not undertake
such changes. We have already discovered with the previous attempt that
there are unexpected pitfalls and lacking test coverage. Also, there
isn't even a patch yet. I suggest we drop this for now, or reconsider
it for PG18, as you wish.

#18Ashutosh Bapat
ashutosh.bapat.oss@gmail.com
In reply to: Peter Eisentraut (#17)
Re: table inheritance versus column compression and storage settings

On Thu, Mar 21, 2024 at 3:19 PM Peter Eisentraut <peter@eisentraut.org>
wrote:

On 07.03.24 17:54, Ashutosh Bapat wrote:

The pg_dump problems arise because we throw an error when parents have
conflicting compression and storage properties. The patch that got
reverted, changed this slightly by allowing a child to override parent's
properties even when they conflict. It still threw an error when child
didn't override and parents conflicted. I guess, MergeAttributes()
raises error when it encounters parents with conflicting properties
because it can not decide which of the conflicting properties the child
should inherit. Instead it could just set the DEFAULT properties when
parent properties conflict but child doesn't override. Thus when
compression conflicts, child's compression would be set to default and
when storage conflicts it will be set to the type's default storage.
Child's properties when specified explicitly would override always. This
will solve all the pg_dump bugs we saw with the reverted patch and also
existing bug I reported earlier.

This change would break backward compatibility but I don't think anybody
would rely on error being thrown when parent properties conflict.

What do you think?

At this point in the development cycle, I would rather not undertake
such changes. We have already discovered with the previous attempt that
there are unexpected pitfalls and lacking test coverage. Also, there
isn't even a patch yet. I suggest we drop this for now, or reconsider
it for PG18, as you wish.

I am fine with this. Should I mark the CF entry as RWF?

--
Best Wishes,
Ashutosh Bapat