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

bug:

begin;
drop table if exists pks cascade;
create table pks(i int primary key, b int) partition by range (i);
create table pks_34 partition of pks for values from (3) to (6);
create table pks_d partition of pks default;
insert into pks values (0), (1), (3), (4), (5);
commit;
alter table pks_d add constraint cc check(i <> 5);
ALTER TABLE pks SPLIT PARTITION pks_34 INTO
    (PARTITION pks_3 FOR VALUES FROM (3) TO (4),
      PARTITION pks_4 FOR VALUES FROM (4) TO (5));

now pks_d have values(5) for column i, which would violate the
cc check constraint.
------------------------
/*
 * PartitionCmd - info for ALTER TABLE/INDEX ATTACH/DETACH PARTITION commands
 */
typedef struct PartitionCmd
{
    NodeTag        type;
    RangeVar   *name;            /* name of partition to
attach/detach/merge/split */
    PartitionBoundSpec *bound;    /* FOR VALUES, if attaching */
    List       *partlist;        /* list of partitions, for MERGE/SPLIT
                                 * PARTITION command */
    bool        concurrent;
} PartitionCmd;
"ALTER TABLE/INDEX ATTACH/DETACH PARTITION" comments need updated.


SplitPartitionMoveRows
                ereport(ERROR,
                        errcode(ERRCODE_CHECK_VIOLATION),
                        errmsg("can not find partition for split
partition row"),
                        errtable(splitRel));
will this ever be reachable?

in SplitPartitionMoveRows
+ if (sps->bound->is_default)
+ {
+ /* We should not create constraint for detached DEFAULT partition. */
+ defaultPartCtx = pc;
+ }
I am not sure the comment "detached DEFAULT partition" is correct.
the "constraint" is not ideal, better word would be "partition
constraint" or "partition qual".

In ATExecSplitPartition,
we can first create the new partitions and then detach the original partition.
It makes more sense, IMHO.
For example:
ALTER TABLE pks SPLIT PARTITION pks_34 INTO
    (PARTITION pks_3 FOR VALUES FROM (3) TO (4),
     PARTITION pks_4 FOR VALUES FROM (4) TO (6));
In this case, we could first create the relations pks_3 and pks_4, then detach
the partition pks_34

should the newly created partitions be based on the split partition or on the
partitioned table?
In the current v50 implementation, they are based on the partitioned table, but
these newly created partitions based on the split partition also make sense.


+ econtext->ecxt_scantuple = srcslot;
+
+ /* Search partition for current slot srcslot. */
+ foreach(listptr, partContexts)
+ {
+ pc = (SplitPartitionContext *) lfirst(listptr);
+
+ if (pc->partqualstate /* skip DEFAULT partition */ &&
+ ExecCheck(pc->partqualstate, econtext))
+ {
+ found = true;
+ break;
+ }
+ ResetExprContext(econtext);
this ResetExprContext is not needed, if you are evaluate different
ExprState again the same slot. See ExecRelCheck also.


see execute_extension_script
                     control->trusted
                     ? errhint("Must have CREATE privilege on current
database to create this extension.")
                     : errhint("Must be superuser to create this extension.")));
in v50-0002, we can refactor it as like the following
checkPartition(Relation rel, Oid partRelOid, bool is_merge)
{
    Relation    partRel;
    partRel = table_open(partRelOid, NoLock);
    if (partRel->rd_rel->relkind != RELKIND_RELATION)
        ereport(ERROR,
                errcode(ERRCODE_WRONG_OBJECT_TYPE),
                errmsg("\"%s\" is not a table",
RelationGetRelationName(partRel)),
                is_merge
                ? errhint("ALTER TABLE ... MERGE PARTITIONS can only
merge partitions don't have sub-partitions")
                : errhint("ALTER TABLE ... SPLIT PARTITIONS can only
split partitions don't have sub-partitions"));

I did some regress test refactoring to reduce test size.
-SELECT * FROM sales_range;
+SELECT tableoid, * FROM sales_range ORDER BY salesperson_id;

other miscellaneous refactoring is also attached.