From 1b25410e75a2029c3aec7f455bcb48e6cdac241e Mon Sep 17 00:00:00 2001 From: jian he Date: Wed, 18 Jun 2025 10:40:39 +0800 Subject: [PATCH v47 1/1] rename function argument and minor refactor * rename "rel" or "modelRel" to "parent_rel" to improve code readability * comments refactoring * minor tests changes. --- src/backend/commands/tablecmds.c | 98 +++++++++---------- src/test/regress/expected/partition_merge.out | 64 +++++------- src/test/regress/sql/partition_merge.sql | 8 +- 3 files changed, 75 insertions(+), 95 deletions(-) diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index b724e2d7150..d7d0e349b19 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -22158,17 +22158,16 @@ evaluateGeneratedExpressionsAndCheckConstraints(AlteredTableInfo *tab, } /* - * getAttributesList: return list of columns (ColumnDef) like model table - * (modelRel) + * getAttributesList: build a list of columns (ColumnDef) based on parent_rel */ static List * -getAttributesList(Relation modelRel) +getAttributesList(Relation parent_rel) { AttrNumber parent_attno; TupleDesc modelDesc; List *colList = NIL; - modelDesc = RelationGetDescr(modelRel); + modelDesc = RelationGetDescr(parent_rel); for (parent_attno = 1; parent_attno <= modelDesc->natts; parent_attno++) @@ -22214,13 +22213,14 @@ getAttributesList(Relation modelRel) /* - * createTableConstraints: create constraints, default values and generated - * values (prototype is function expandTableLikeClause). - * tab is pending-work queue for newRel, we may need it in moveMergedTablesRows. + * createTableConstraints: + * create check constraints, default values and generated values for newRel + * based on parent_rel. tab is pending-work queue for newRel, we may need it in + * moveMergedTablesRows. */ static void createTableConstraints(List **wqueue, AlteredTableInfo *tab, - Relation modelRel, Relation newRel) + Relation parent_rel, Relation newRel) { TupleDesc tupleDesc; TupleConstr *constr; @@ -22230,7 +22230,7 @@ createTableConstraints(List **wqueue, AlteredTableInfo *tab, List *Constraints = NIL; List *cookedConstraints = NIL; - tupleDesc = RelationGetDescr(modelRel); + tupleDesc = RelationGetDescr(parent_rel); constr = tupleDesc->constr; if (!constr) @@ -22265,13 +22265,13 @@ createTableConstraints(List **wqueue, AlteredTableInfo *tab, NewColumnValue *newval; if (attribute->attgenerated == ATTRIBUTE_GENERATED_VIRTUAL) - this_default = build_generation_expression(modelRel, attribute->attnum); + this_default = build_generation_expression(parent_rel, attribute->attnum); else { this_default = TupleDescGetDefault(tupleDesc, attribute->attnum); if (this_default == NULL) elog(ERROR, "default expression not found for attribute %d of relation \"%s\"", - attribute->attnum, RelationGetRelationName(modelRel)); + attribute->attnum, RelationGetRelationName(parent_rel)); } num = attmap->attnums[parent_attno - 1]; @@ -22288,14 +22288,14 @@ createTableConstraints(List **wqueue, AlteredTableInfo *tab, errmsg("cannot convert whole-row table reference"), errdetail("Generation expression for column \"%s\" contains a whole-row reference to table \"%s\".", NameStr(attribute->attname), - RelationGetRelationName(modelRel))); + RelationGetRelationName(parent_rel))); /* Add a pre-cooked default expression. */ StoreAttrDefault(newRel, num, def, true); /* - * Stored generated column expressions in modelRel might reference - * tableoid. newRel, modelRel tableoid clear is not the same. If + * Stored generated column expressions in parent_rel might reference + * tableoid. newRel, parent_rel tableoid clear is not the same. If * so, these stored generated columns require recomputation for * newRel within moveMergedTablesRows. */ @@ -22345,7 +22345,7 @@ createTableConstraints(List **wqueue, AlteredTableInfo *tab, errmsg("cannot convert whole-row table reference"), errdetail("Constraint \"%s\" contains a whole-row reference to table \"%s\".", ccname, - RelationGetRelationName(modelRel))); + RelationGetRelationName(parent_rel))); constr = makeNode(Constraint); constr->contype = CONSTR_CHECK; @@ -22370,7 +22370,7 @@ createTableConstraints(List **wqueue, AlteredTableInfo *tab, CommandCounterIncrement(); /* - * modelRel check constraint expresssion may reference tableoid, so later in + * parent_rel check constraint expresssion may reference tableoid, so later in * moveMergedTablesRows, we need evulate the check constraint again for the * newRel. We can check weather check constraint contain tableoid reference * or not via pull_varattnos. @@ -22386,8 +22386,7 @@ createTableConstraints(List **wqueue, AlteredTableInfo *tab, pull_varattnos(qual, 1, &attnums); /* - * Add check only if it contains tableoid - * (TableOidAttributeNumber). + * Add check only if it contains tableoid (TableOidAttributeNumber). */ if (bms_is_member(TableOidAttributeNumber - FirstLowInvalidHeapAttributeNumber, attnums)) @@ -22416,7 +22415,7 @@ createTableConstraints(List **wqueue, AlteredTableInfo *tab, * The "include_noinh" argument is false because a partitioned table * cannot have NO INHERIT constraint. */ - nnconstraints = RelationGetNotNullConstraints(RelationGetRelid(modelRel), + nnconstraints = RelationGetNotNullConstraints(RelationGetRelid(parent_rel), false, false); Assert(list_length(nnconstraints) > 0); @@ -22431,20 +22430,18 @@ createTableConstraints(List **wqueue, AlteredTableInfo *tab, /* - * createPartitionTable: create table for a new partition with given name - * (newPartName) like table (modelRel, partitioned table). ownerId is - * determined by the partition on which the operation is performed, so it - * is passed separately. + * createPartitionTable: * - * Also, this function sets the new partition access method same as parent - * table access methods (similarly to CREATE TABLE ... PARTITION OF). It - * checks that parent and child tables have compatible persistence. + * Create a new partition (newPartName) for partitioned table (parent_rel). + * ownerId is determined by the partition on which the operation is performed, + * so it is passed separately. The new partition will inherit the access method + * and persistence type from the parent table. * - * Function returns the created relation (locked in AccessExclusiveLock mode). + * returns the created relation (locked in AccessExclusiveLock mode). */ static Relation createPartitionTable(List **wqueue, RangeVar *newPartName, - Relation modelRel, Oid ownerId) + Relation parent_rel, Oid ownerId) { Relation newRel; Oid newRelId; @@ -22456,19 +22453,19 @@ createPartitionTable(List **wqueue, RangeVar *newPartName, AlteredTableInfo *new_partrel_tab; /* If existing rel is temp, it must belong to this session */ - if (RELATION_IS_OTHER_TEMP(modelRel)) + if (RELATION_IS_OTHER_TEMP(parent_rel)) ereport(ERROR, errcode(ERRCODE_WRONG_OBJECT_TYPE), errmsg("cannot create as partition of temporary relation of another session")); /* Look up inheritance ancestors and generate relation schema. */ - colList = getAttributesList(modelRel); + colList = getAttributesList(parent_rel); /* Create a tuple descriptor from the relation schema. */ descriptor = BuildDescForRelation(colList); /* Look up the access method for new relation. */ - relamId = (modelRel->rd_rel->relam != InvalidOid) ? modelRel->rd_rel->relam : HEAP_TABLE_AM_OID; + relamId = (parent_rel->rd_rel->relam != InvalidOid) ? parent_rel->rd_rel->relam : HEAP_TABLE_AM_OID; /* Look up the namespace in which we are supposed to create the relation. */ namespaceId = @@ -22481,7 +22478,7 @@ createPartitionTable(List **wqueue, RangeVar *newPartName, /* Create the relation. */ newRelId = heap_create_with_catalog(newPartName->relname, namespaceId, - modelRel->rd_rel->reltablespace, + parent_rel->rd_rel->reltablespace, InvalidOid, InvalidOid, InvalidOid, @@ -22497,7 +22494,7 @@ createPartitionTable(List **wqueue, RangeVar *newPartName, (Datum) 0, true, allowSystemTableMods, - false, + true, InvalidOid, NULL); @@ -22522,23 +22519,23 @@ createPartitionTable(List **wqueue, RangeVar *newPartName, * affected by the search_path. If the parent is permanent, so must be * all of its partitions. */ - if (modelRel->rd_rel->relpersistence != RELPERSISTENCE_TEMP && + if (parent_rel->rd_rel->relpersistence != RELPERSISTENCE_TEMP && newRel->rd_rel->relpersistence == RELPERSISTENCE_TEMP) ereport(ERROR, errcode(ERRCODE_WRONG_OBJECT_TYPE), errmsg("cannot create a temporary relation as partition of permanent relation \"%s\"", - RelationGetRelationName(modelRel))); + RelationGetRelationName(parent_rel))); /* Permanent rels cannot be partitions belonging to temporary parent */ if (newRel->rd_rel->relpersistence != RELPERSISTENCE_TEMP && - modelRel->rd_rel->relpersistence == RELPERSISTENCE_TEMP) + parent_rel->rd_rel->relpersistence == RELPERSISTENCE_TEMP) ereport(ERROR, errcode(ERRCODE_WRONG_OBJECT_TYPE), errmsg("cannot create a permanent relation as partition of temporary relation \"%s\"", - RelationGetRelationName(modelRel))); + RelationGetRelationName(parent_rel))); /* Create constraints, default values and generated values */ - createTableConstraints(wqueue, new_partrel_tab, modelRel, newRel); + createTableConstraints(wqueue, new_partrel_tab, parent_rel, newRel); /* * Need to call CommandCounterIncrement, so fresh relcache entry have newly @@ -22831,9 +22828,19 @@ ATExecMergePartitions(List **wqueue, AlteredTableInfo *tab, Relation rel, defaultPartOid = get_default_oid_from_partdesc(RelationGetPartitionDesc(rel, true)); + /* Detach all merged partitions */ + foreach_oid(mergingPartitionOid, mergingPartitions) + { + Relation child_rel; + + child_rel = table_open(mergingPartitionOid, NoLock); + + detachPartitionTable(rel, child_rel, defaultPartOid); + + table_close(child_rel, NoLock); + } + /* - * Detach all merged partitions. - * * Perform a preliminary check to determine whether it's safe to drop all * merging partitions before we actually do so later. After merging rows * into the new partitions via moveMergedTablesRows, all old partitions need @@ -22841,17 +22848,6 @@ ATExecMergePartitions(List **wqueue, AlteredTableInfo *tab, Relation rel, * merge process (moveMergedTablesRows) can be time-consuming, performing an * early check on the drop eligibility of old partitions is preferable. */ - foreach_oid(mergingPartitionOid, mergingPartitions) - { - Relation child_rel; - - child_rel = table_open(mergingPartitionOid, NoLock); - - detachPartitionTable(rel, child_rel, defaultPartOid); - - table_close(child_rel, NoLock); - } - foreach_oid(mergingPartitionOid, mergingPartitions) { ObjectAddress object; diff --git a/src/test/regress/expected/partition_merge.out b/src/test/regress/expected/partition_merge.out index f5e6ec6184a..531f2021aad 100644 --- a/src/test/regress/expected/partition_merge.out +++ b/src/test/regress/expected/partition_merge.out @@ -48,6 +48,12 @@ LINE 1: ...e MERGE PARTITIONS (sales_feb2022, sales_mar2022, partitions... --ERROR, sales_apr_2 already exists ALTER TABLE sales_range MERGE PARTITIONS (sales_feb2022, sales_mar2022, sales_jan2022) INTO sales_apr_2; ERROR: relation "sales_apr_2" already exists +CREATE VIEW jan2022v as SELECT * FROM sales_jan2022; +ALTER TABLE sales_range MERGE PARTITIONS (sales_jan2022, sales_feb2022) INTO sales_dec_jan_feb2022; +ERROR: cannot drop table sales_jan2022 because other objects depend on it +DETAIL: view jan2022v depends on table sales_jan2022 +HINT: Use DROP ... CASCADE to drop the dependent objects too. +DROP VIEW jan2022v; -- NO ERROR: test for custom partitions order, source partitions not in the search_path SET search_path = partitions_merge_schema2, public; ALTER TABLE partitions_merge_schema.sales_range MERGE PARTITIONS ( @@ -587,49 +593,25 @@ EXECUTE get_partition_info('{sales_list}'); sales_others | p | r | f | DEFAULT (3 rows) -SELECT * FROM sales_list; - salesperson_id | salesperson_name | sales_state | sales_amount | sales_date -----------------+------------------+----------------+--------------+------------ - 2 | Smirnoff | New York | 500 | 03-03-2022 - 5 | Deev | Lisbon | 250 | 03-07-2022 - 11 | Muller | Madrid | 650 | 03-05-2022 - 14 | Plato | Lisbon | 950 | 03-05-2022 - 1 | Trump | Bejing | 1000 | 03-01-2022 - 8 | Li | Vladivostok | 1150 | 03-09-2022 - 4 | Ivanov | Warsaw | 750 | 03-04-2022 - 6 | Poirot | Berlin | 1000 | 03-01-2022 - 12 | Smith | Kyiv | 350 | 03-10-2022 - 13 | Gandi | Warsaw | 150 | 03-08-2022 - 3 | Ford | St. Petersburg | 2000 | 03-05-2022 - 7 | May | Helsinki | 1200 | 03-06-2022 - 9 | May | Helsinki | 1200 | 03-11-2022 - 10 | Halder | Oslo | 800 | 03-02-2022 +SELECT tableoid::regclass, * FROM sales_list ORDER BY tableoid, salesperson_id; + tableoid | salesperson_id | salesperson_name | sales_state | sales_amount | sales_date +------------+----------------+------------------+----------------+--------------+------------ + sales_nord | 3 | Ford | St. Petersburg | 2000 | 03-05-2022 + sales_nord | 7 | May | Helsinki | 1200 | 03-06-2022 + sales_nord | 9 | May | Helsinki | 1200 | 03-11-2022 + sales_nord | 10 | Halder | Oslo | 800 | 03-02-2022 + sales_all | 1 | Trump | Bejing | 1000 | 03-01-2022 + sales_all | 2 | Smirnoff | New York | 500 | 03-03-2022 + sales_all | 4 | Ivanov | Warsaw | 750 | 03-04-2022 + sales_all | 5 | Deev | Lisbon | 250 | 03-07-2022 + sales_all | 6 | Poirot | Berlin | 1000 | 03-01-2022 + sales_all | 8 | Li | Vladivostok | 1150 | 03-09-2022 + sales_all | 11 | Muller | Madrid | 650 | 03-05-2022 + sales_all | 12 | Smith | Kyiv | 350 | 03-10-2022 + sales_all | 13 | Gandi | Warsaw | 150 | 03-08-2022 + sales_all | 14 | Plato | Lisbon | 950 | 03-05-2022 (14 rows) -SELECT * FROM sales_nord; - salesperson_id | salesperson_name | sales_state | sales_amount | sales_date -----------------+------------------+----------------+--------------+------------ - 3 | Ford | St. Petersburg | 2000 | 03-05-2022 - 7 | May | Helsinki | 1200 | 03-06-2022 - 9 | May | Helsinki | 1200 | 03-11-2022 - 10 | Halder | Oslo | 800 | 03-02-2022 -(4 rows) - -SELECT * FROM sales_all; - salesperson_id | salesperson_name | sales_state | sales_amount | sales_date -----------------+------------------+-------------+--------------+------------ - 2 | Smirnoff | New York | 500 | 03-03-2022 - 5 | Deev | Lisbon | 250 | 03-07-2022 - 11 | Muller | Madrid | 650 | 03-05-2022 - 14 | Plato | Lisbon | 950 | 03-05-2022 - 1 | Trump | Bejing | 1000 | 03-01-2022 - 8 | Li | Vladivostok | 1150 | 03-09-2022 - 4 | Ivanov | Warsaw | 750 | 03-04-2022 - 6 | Poirot | Berlin | 1000 | 03-01-2022 - 12 | Smith | Kyiv | 350 | 03-10-2022 - 13 | Gandi | Warsaw | 150 | 03-08-2022 -(10 rows) - -- Use indexscan for testing indexes after merging partitions SET enable_seqscan = OFF; SELECT * FROM sales_all WHERE sales_state = 'Warsaw'; diff --git a/src/test/regress/sql/partition_merge.sql b/src/test/regress/sql/partition_merge.sql index 2cdd58b2008..bf8acc5136c 100644 --- a/src/test/regress/sql/partition_merge.sql +++ b/src/test/regress/sql/partition_merge.sql @@ -42,6 +42,10 @@ ALTER TABLE sales_range MERGE PARTITIONS (sales_feb2022, sales_mar2022, partitio --ERROR, sales_apr_2 already exists ALTER TABLE sales_range MERGE PARTITIONS (sales_feb2022, sales_mar2022, sales_jan2022) INTO sales_apr_2; +CREATE VIEW jan2022v as SELECT * FROM sales_jan2022; +ALTER TABLE sales_range MERGE PARTITIONS (sales_jan2022, sales_feb2022) INTO sales_dec_jan_feb2022; +DROP VIEW jan2022v; + -- NO ERROR: test for custom partitions order, source partitions not in the search_path SET search_path = partitions_merge_schema2, public; ALTER TABLE partitions_merge_schema.sales_range MERGE PARTITIONS ( @@ -414,9 +418,7 @@ ALTER TABLE sales_list MERGE PARTITIONS (sales_west, sales_east, sales_central) -- show partitions with conditions: EXECUTE get_partition_info('{sales_list}'); -SELECT * FROM sales_list; -SELECT * FROM sales_nord; -SELECT * FROM sales_all; +SELECT tableoid::regclass, * FROM sales_list ORDER BY tableoid, salesperson_id; -- Use indexscan for testing indexes after merging partitions SET enable_seqscan = OFF; -- 2.34.1