From 28e4dbba35fc3162c13f5896551921896cf30d1c Mon Sep 17 00:00:00 2001 From: Peter Eisentraut Date: Wed, 12 Jul 2023 16:12:34 +0200 Subject: [PATCH v3 1/9] 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 | 123 ++++++++++++------------------- 1 file changed, 48 insertions(+), 75 deletions(-) diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 8a2c671b66..ff001f5ceb 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -15695,53 +15695,39 @@ 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))); /* * If the parent has a not-null constraint that's not NO INHERIT, @@ -15749,40 +15735,38 @@ MergeAttributesIntoExisting(Relation child_rel, Relation parent_rel) * * Other constraints are checked elsewhere. */ - if (attribute->attnotnull && !childatt->attnotnull) + if (parent_att->attnotnull && !child_att->attnotnull) { HeapTuple contup; contup = findNotNullConstraintAttnum(RelationGetRelid(parent_rel), - attribute->attnum); + parent_att->attnum); if (HeapTupleIsValid(contup) && !((Form_pg_constraint) GETSTRUCT(contup))->connoinherit) ereport(ERROR, errcode(ERRCODE_DATATYPE_MISMATCH), errmsg("column \"%s\" in child table must be marked NOT NULL", - attributeName)); + 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")); @@ -15792,10 +15776,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); @@ -15805,8 +15789,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))); } } @@ -15833,27 +15816,20 @@ 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; Oid parent_relid = RelationGetRelid(parent_rel); - 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(parent_relid)); - 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))) @@ -15877,7 +15853,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))) @@ -15892,10 +15868,12 @@ MergeConstraintsIntoExisting(Relation child_rel, Relation parent_rel) * CHECK constraint are matched by name, NOT NULL ones by * attribute number */ - if (child_con->contype == CONSTRAINT_CHECK && - strcmp(NameStr(parent_con->conname), - NameStr(child_con->conname)) != 0) - continue; + if (child_con->contype == CONSTRAINT_CHECK) + { + if (strcmp(NameStr(parent_con->conname), + NameStr(child_con->conname)) != 0) + continue; + } else if (child_con->contype == CONSTRAINT_NOTNULL) { AttrNumber parent_attno = extractNotNullColumn(parent_tuple); @@ -15908,12 +15886,11 @@ MergeConstraintsIntoExisting(Relation child_rel, Relation parent_rel) } if (child_con->contype == CONSTRAINT_CHECK && - !constraints_equivalent(parent_tuple, child_tuple, tuple_desc)) + !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 CHECK child constraint is "no inherit" then cannot @@ -15931,8 +15908,7 @@ MergeConstraintsIntoExisting(Relation child_rel, Relation parent_rel) 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 @@ -15942,8 +15918,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 @@ -15965,13 +15940,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; @@ -15998,7 +15973,7 @@ MergeConstraintsIntoExisting(Relation child_rel, Relation parent_rel) } systable_endscan(parent_scan); - table_close(catalog_relation, RowExclusiveLock); + table_close(constraintrel, RowExclusiveLock); } /* @@ -16154,11 +16129,9 @@ RemoveInheritance(Relation child_rel, Relation parent_rel, bool expect_detached) List *connames; List *nncolumns; 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), @@ -16166,7 +16139,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\"", @@ -16320,7 +16293,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.42.0