From fcdd42c9507bd048afd5443f74b1b8bad31b7595 Mon Sep 17 00:00:00 2001 From: Peter Eisentraut Date: Thu, 8 Jun 2023 13:13:12 +0200 Subject: [PATCH 05/17] Clean up MergeAttributesIntoExisting() Make variable naming clearer and more consistent. Move some variables to smaller scope. Remove some unnecessary intermediate variables. Try to save some vertical space. Apply analogous changes to nearby MergeConstraintsIntoExisting() and RemoveInheritance() for consistency. --- src/backend/commands/tablecmds.c | 115 +++++++++++-------------------- 1 file changed, 42 insertions(+), 73 deletions(-) diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index a6482a6d72..62b555aa20 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -15112,84 +15112,67 @@ static void MergeAttributesIntoExisting(Relation child_rel, Relation parent_rel) { Relation attrrel; - AttrNumber parent_attno; - int parent_natts; - TupleDesc tupleDesc; - HeapTuple tuple; - bool child_is_partition = false; + TupleDesc parent_desc; attrrel = table_open(AttributeRelationId, RowExclusiveLock); + parent_desc = RelationGetDescr(parent_rel); - tupleDesc = RelationGetDescr(parent_rel); - parent_natts = tupleDesc->natts; - - /* If parent_rel is a partitioned table, child_rel must be a partition */ - if (parent_rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE) - child_is_partition = true; - - for (parent_attno = 1; parent_attno <= parent_natts; parent_attno++) + for (AttrNumber parent_attno = 1; parent_attno <= parent_desc->natts; parent_attno++) { - Form_pg_attribute attribute = TupleDescAttr(tupleDesc, - parent_attno - 1); - char *attributeName = NameStr(attribute->attname); + Form_pg_attribute parent_att = TupleDescAttr(parent_desc, parent_attno - 1); + char *parent_attname = NameStr(parent_att->attname); + HeapTuple tuple; /* Ignore dropped columns in the parent. */ - if (attribute->attisdropped) + if (parent_att->attisdropped) continue; /* Find same column in child (matching on column name). */ - tuple = SearchSysCacheCopyAttName(RelationGetRelid(child_rel), - attributeName); + tuple = SearchSysCacheCopyAttName(RelationGetRelid(child_rel), parent_attname); if (HeapTupleIsValid(tuple)) { - /* Check they are same type, typmod, and collation */ - Form_pg_attribute childatt = (Form_pg_attribute) GETSTRUCT(tuple); + Form_pg_attribute child_att = (Form_pg_attribute) GETSTRUCT(tuple); - if (attribute->atttypid != childatt->atttypid || - attribute->atttypmod != childatt->atttypmod) + if (parent_att->atttypid != child_att->atttypid || + parent_att->atttypmod != child_att->atttypmod) ereport(ERROR, (errcode(ERRCODE_DATATYPE_MISMATCH), errmsg("child table \"%s\" has different type for column \"%s\"", - RelationGetRelationName(child_rel), - attributeName))); + RelationGetRelationName(child_rel), parent_attname))); - if (attribute->attcollation != childatt->attcollation) + if (parent_att->attcollation != child_att->attcollation) ereport(ERROR, (errcode(ERRCODE_COLLATION_MISMATCH), errmsg("child table \"%s\" has different collation for column \"%s\"", - RelationGetRelationName(child_rel), - attributeName))); + RelationGetRelationName(child_rel), parent_attname))); /* * Check child doesn't discard NOT NULL property. (Other * constraints are checked elsewhere.) */ - if (attribute->attnotnull && !childatt->attnotnull) + if (parent_att->attnotnull && !child_att->attnotnull) ereport(ERROR, (errcode(ERRCODE_DATATYPE_MISMATCH), - errmsg("column \"%s\" in child table must be marked NOT NULL", - attributeName))); + errmsg("column \"%s\" in child table must be marked NOT NULL", parent_attname))); /* * Child column must be generated if and only if parent column is. */ - if (attribute->attgenerated && !childatt->attgenerated) + if (parent_att->attgenerated && !child_att->attgenerated) ereport(ERROR, (errcode(ERRCODE_DATATYPE_MISMATCH), - errmsg("column \"%s\" in child table must be a generated column", - attributeName))); - if (childatt->attgenerated && !attribute->attgenerated) + errmsg("column \"%s\" in child table must be a generated column", parent_attname))); + if (child_att->attgenerated && !parent_att->attgenerated) ereport(ERROR, (errcode(ERRCODE_DATATYPE_MISMATCH), - errmsg("column \"%s\" in child table must not be a generated column", - attributeName))); + errmsg("column \"%s\" in child table must not be a generated column", parent_attname))); /* * OK, bump the child column's inheritance count. (If we fail * later on, this change will just roll back.) */ - childatt->attinhcount++; - if (childatt->attinhcount < 0) + child_att->attinhcount++; + if (child_att->attinhcount < 0) ereport(ERROR, errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED), errmsg("too many inheritance parents")); @@ -15199,10 +15182,10 @@ MergeAttributesIntoExisting(Relation child_rel, Relation parent_rel) * is same in all partitions. (Note: there are only inherited * attributes in partitions) */ - if (child_is_partition) + if (parent_rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE) { - Assert(childatt->attinhcount == 1); - childatt->attislocal = false; + Assert(child_att->attinhcount == 1); + child_att->attislocal = false; } CatalogTupleUpdate(attrrel, &tuple->t_self, tuple); @@ -15212,8 +15195,7 @@ MergeAttributesIntoExisting(Relation child_rel, Relation parent_rel) { ereport(ERROR, (errcode(ERRCODE_DATATYPE_MISMATCH), - errmsg("child table is missing column \"%s\"", - attributeName))); + errmsg("child table is missing column \"%s\"", parent_attname))); } } @@ -15240,26 +15222,19 @@ MergeAttributesIntoExisting(Relation child_rel, Relation parent_rel) static void MergeConstraintsIntoExisting(Relation child_rel, Relation parent_rel) { - Relation catalog_relation; - TupleDesc tuple_desc; + Relation constraintrel; SysScanDesc parent_scan; ScanKeyData parent_key; HeapTuple parent_tuple; - bool child_is_partition = false; - - catalog_relation = table_open(ConstraintRelationId, RowExclusiveLock); - tuple_desc = RelationGetDescr(catalog_relation); - /* If parent_rel is a partitioned table, child_rel must be a partition */ - if (parent_rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE) - child_is_partition = true; + constraintrel = table_open(ConstraintRelationId, RowExclusiveLock); /* Outer loop scans through the parent's constraint definitions */ ScanKeyInit(&parent_key, Anum_pg_constraint_conrelid, BTEqualStrategyNumber, F_OIDEQ, ObjectIdGetDatum(RelationGetRelid(parent_rel))); - parent_scan = systable_beginscan(catalog_relation, ConstraintRelidTypidNameIndexId, + parent_scan = systable_beginscan(constraintrel, ConstraintRelidTypidNameIndexId, true, NULL, 1, &parent_key); while (HeapTupleIsValid(parent_tuple = systable_getnext(parent_scan))) @@ -15282,7 +15257,7 @@ MergeConstraintsIntoExisting(Relation child_rel, Relation parent_rel) Anum_pg_constraint_conrelid, BTEqualStrategyNumber, F_OIDEQ, ObjectIdGetDatum(RelationGetRelid(child_rel))); - child_scan = systable_beginscan(catalog_relation, ConstraintRelidTypidNameIndexId, + child_scan = systable_beginscan(constraintrel, ConstraintRelidTypidNameIndexId, true, NULL, 1, &child_key); while (HeapTupleIsValid(child_tuple = systable_getnext(child_scan))) @@ -15293,24 +15268,21 @@ MergeConstraintsIntoExisting(Relation child_rel, Relation parent_rel) if (child_con->contype != CONSTRAINT_CHECK) continue; - if (strcmp(NameStr(parent_con->conname), - NameStr(child_con->conname)) != 0) + if (strcmp(NameStr(parent_con->conname), NameStr(child_con->conname)) != 0) continue; - if (!constraints_equivalent(parent_tuple, child_tuple, tuple_desc)) + if (!constraints_equivalent(parent_tuple, child_tuple, RelationGetDescr(constraintrel))) ereport(ERROR, (errcode(ERRCODE_DATATYPE_MISMATCH), errmsg("child table \"%s\" has different definition for check constraint \"%s\"", - RelationGetRelationName(child_rel), - NameStr(parent_con->conname)))); + RelationGetRelationName(child_rel), NameStr(parent_con->conname)))); /* If the child constraint is "no inherit" then cannot merge */ if (child_con->connoinherit) ereport(ERROR, (errcode(ERRCODE_INVALID_OBJECT_DEFINITION), errmsg("constraint \"%s\" conflicts with non-inherited constraint on child table \"%s\"", - NameStr(child_con->conname), - RelationGetRelationName(child_rel)))); + NameStr(child_con->conname), RelationGetRelationName(child_rel)))); /* * If the child constraint is "not valid" then cannot merge with a @@ -15320,8 +15292,7 @@ MergeConstraintsIntoExisting(Relation child_rel, Relation parent_rel) ereport(ERROR, (errcode(ERRCODE_INVALID_OBJECT_DEFINITION), errmsg("constraint \"%s\" conflicts with NOT VALID constraint on child table \"%s\"", - NameStr(child_con->conname), - RelationGetRelationName(child_rel)))); + NameStr(child_con->conname), RelationGetRelationName(child_rel)))); /* * OK, bump the child constraint's inheritance count. (If we fail @@ -15340,13 +15311,13 @@ MergeConstraintsIntoExisting(Relation child_rel, Relation parent_rel) * inherited only once since it cannot have multiple parents and * it is never considered local. */ - if (child_is_partition) + if (parent_rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE) { Assert(child_con->coninhcount == 1); child_con->conislocal = false; } - CatalogTupleUpdate(catalog_relation, &child_copy->t_self, child_copy); + CatalogTupleUpdate(constraintrel, &child_copy->t_self, child_copy); heap_freetuple(child_copy); found = true; @@ -15363,7 +15334,7 @@ MergeConstraintsIntoExisting(Relation child_rel, Relation parent_rel) } systable_endscan(parent_scan); - table_close(catalog_relation, RowExclusiveLock); + table_close(constraintrel, RowExclusiveLock); } /* @@ -15506,11 +15477,9 @@ RemoveInheritance(Relation child_rel, Relation parent_rel, bool expect_detached) constraintTuple; List *connames; bool found; - bool child_is_partition = false; + bool is_partitioning; - /* If parent_rel is a partitioned table, child_rel must be a partition */ - if (parent_rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE) - child_is_partition = true; + is_partitioning = (parent_rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE); found = DeleteInheritsTuple(RelationGetRelid(child_rel), RelationGetRelid(parent_rel), @@ -15518,7 +15487,7 @@ RemoveInheritance(Relation child_rel, Relation parent_rel, bool expect_detached) RelationGetRelationName(child_rel)); if (!found) { - if (child_is_partition) + if (is_partitioning) ereport(ERROR, (errcode(ERRCODE_UNDEFINED_TABLE), errmsg("relation \"%s\" is not a partition of relation \"%s\"", @@ -15648,7 +15617,7 @@ RemoveInheritance(Relation child_rel, Relation parent_rel, bool expect_detached) drop_parent_dependency(RelationGetRelid(child_rel), RelationRelationId, RelationGetRelid(parent_rel), - child_dependency_type(child_is_partition)); + child_dependency_type(is_partitioning)); /* * Post alter hook of this inherits. Since object_access_hook doesn't take -- 2.41.0