Re: Add SPLIT PARTITION/MERGE PARTITIONS commands - Mailing list pgsql-hackers

Hi Chao Li!

Thanks for reporting the issues!

1.
 >Nit: "when you need delete" => "when you need to delete"
 >"Must specified" => "must be specified"
 >"We want delete" => "we want to delete"

Replaced.


2.
 >+    depRel = table_open(DependRelationId, RowExclusiveLock);
 >This function looks only performing read-only checks, why do we need
 >RowExclusiveLock? Is AccessShareLock good enough?

performDeletionCheck function is not required for the SPLIT/MERGE 
PARTITION(S) functionality.
Its main goal is perform a preliminary check to determine whether it's 
safe to drop split partition before we actually do so later.
For this purpose, it would be better if the lock is the same as in the 
performDeletion function.


3.
 >AT_MergePartitions, AT_DetachPartitionFinalize and AT_DetachPartition
 >do the same thing, why don't combine them together?

I think AT_MergePartitions and AT_SplitPartitions can be combine.
But I prefer not to change of other conditions as possible 
(AT_DetachPartitionFinalize, AT_DetachPartition, etc.) to avoid rebase 
conflicts.
Changed.


4.
 >+    /* Attach a new partition to the partitioned table. */
 >+    attachPartitionTable(wqueue, rel, attachrel, cmd->bound);
 >I think the comment can be removed as the function name has clearly 
described what it is doing.

Deleted.


5.
 >+static void
 >+attachPartitionTable(List **wqueue, Relation rel, Relation attachrel, 
PartitionBoundSpec *bound)
 >I think bound can be const: const PartitionBoundSpec *bound, to 
indicate read-only on bound.
 >Of course, you need to also update StorePartitionBound() to make bound 
also const ...

I agree, this correction could be done.
But I think it's better to change the argument of the 
StorePartitionBound function in a separate commit, since this fix is not 
related to SPLIT/MERGE PARTITION(S).
And then we can change attachPartitionTable function.


6.
 > Nit: an unneeded empty line.

Empty lines (this and similar ones) removed.


7.
 >+    case CONSTR_CHECK:
 >+
 >+        /*
 >+         * We already expanded virtual expression in
 >+         * createTableConstraints.
 >+         */
 >Nit: an unneeded empty line.

This line was added by pgindent...


8.
 >+    List      *Constraints = NIL;
 >Why this local variable starts with a capital character "C"?

I think it was because of my carelessness. Renamed.


9.
 >+    /* Look up the access method for new relation. */
 >+    relamId = (parent_rel->rd_rel->relam != InvalidOid) ? 
parent_rel->rd_rel->relam : HEAP_TABLE_AM_OID;
 >In this function, "parent_rel->rd_rel" is used in many places, maybe 
we can cache it to a local variable.```

Changed.


10.
 >+        /* Create tuple slot for new partition. */
 >+        srcslot = table_slot_create(mergingPartition, NULL);
 >This comment is quite confusing. Can you rewording to something like:
 >/* Create a source tuple slot for the partition being merged. */

Renamed.


11.
 >Based on the comment of "foreach", deleting cell while interacting is 
unsafe.
 >And nit: "need process" => "need to process"

Corrected.


12.
 >+    /* Detach all merged partitions */
 >+    foreach_oid(mergingPartitionOid, mergingPartitions)
 >Should it be "all merging partitions"?

Corrected.


13.
 >+    if (partRel->rd_rel->relkind != RELKIND_RELATION)
...
 >+    if (!partRel->rd_rel->relispartition)
...
 >I think the first two "if" can be combined. We are trying to check If
 >"partRel" is a partition, when a relation is a partition, its relkind
 >is "r" and "relispartition" is true. Instead, "xx is not a table" is
 >a quite confusing message. ...
 >if (partRel->rd_rel->relkind != RELKIND_RELATION ||
 >  !partRel->rd_rel->relispartition)
 >     ereport(ERROR,
 >       errcode(ERRCODE_WRONG_OBJECT_TYPE),
 >       errmsg("\"%s\" is not a partition of partitioned table \"%s\"",
 > ...

Probably, in this case we will generate a strange error if relkind='f' 
(RELKIND_FOREIGN_TABLE) since it's a partition, but error message will 
be "... is not a partition ...".
Perhaps we should change the error message from "... is not a table" to 
"... is not a ordinary table"?
See comment:
#define      RELKIND_RELATION      'r'    /* ordinary table */


14.
 >+checkPartition(Relation rel, Oid partRelOid)
 >...
 >+    partRel = table_open(partRelOid, NoLock);
 >We can immediately close the table, as data is stored in partRel
 >already, we don't have to defer table close.

It can be done, but I think this violates the style used in PostgreSQL 
(for example, [1], [2], ...).


15.
 > For move rows, the logic of merge partitions and split partition are
 >quite similar. Only difference is that merge partitions takes a fixed
 >dest partition, but split partition use a logic to determine target
 >partition. Maybe we can add a common function to reduce the duplicate
 >code.

The problem is that MERGE PARTITIONS is planned to be optimized further 
to use the merging partition with the maximum number of rows as the new 
merged partition (we minimize the number of moved rows).
This will complicate the logic, and we will have to revert to situation 
with different code for SPLIT and MERGE.


Links.
------
[1] 

https://github.com/postgres/postgres/blob/b0cc0a71e0a0a760f54c72edb8cd000e4555442b/src/backend/commands/cluster.c#L1197
[2] 

https://github.com/postgres/postgres/blob/b0cc0a71e0a0a760f54c72edb8cd000e4555442b/src/backend/commands/cluster.c#L1590

-- 
With best regards,
Dmitry Koval

Postgres Professional: http://postgrespro.com