From 629db9289371d3e72dc6b0d43811fb6decd94525 Mon Sep 17 00:00:00 2001 From: Ashutosh Bapat 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 numeric_literal, REM - Column STORAGE settings are also copied from parent tables. + Explicitly specified Column STORAGE settings overrides + those inherited from parent tables. Otherwise all the parents should have + the same Column STORAGE setting which will be + inherited by the column in the new table, or an error will be reported. 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