Re: Add SPLIT PARTITION/MERGE PARTITIONS commands

Hi!
Thanks for the notes and patches!

1.
 >/* list of partitions, for MERGE PARTITION command */
 > ...
 >The field "partlist" comments are not very helpful, IMO.
 >I think the following is more descriptive.
 >/* list of partitions to be merged, used only in ALTER TABLE MERGE
 >PARTITION */

Corrected.


2.
 >+ partOid = RangeVarGetRelidExtended(name,
 >+   AccessExclusiveLock,
 >+   false,
 >+   RangeVarCallbackOwnsRelation,
 >+   NULL);
 >here "false" should be "0"?

Corrected.


3.
 >the comment should be
 >+ /* Ranges of partitions should be adjacent */

Corrected.


4.
 >+static void StoreConstraints(Relation rel, List *cooked_constraints,
 >+ bool is_internal);
 >-static void StoreConstraints(Relation rel, List *cooked_constraints,
 >- bool is_internal);
 >
 >Is this change necessary?

Corrected.


5.
 >i raised this question in [1], you replied at [2].
 >I still think it's not intuitive.
 >parent->relpersistence is fixed. and newRel->relpersistence is the
 >same as the same as newPartName->relpersistence, see
 >heap_create_with_catalog. These relpersistence error checks can
 >happen before heap_create_with_catalog.
 >I added a regress test for src/test/modules/test_ddl_deparse.
 >I refactored regress to make
 >src/test/regress/expected/partition_merge.out less verbose.

I'm sorry, I misunderstood the point.
Applied.


6.
 >/*
 > * PartitionCmd - info for ALTER TABLE/INDEX ATTACH/DETACH PARTITION
 > commands
 > */
 >typedef struct PartitionCmd
 >the above comments also need to be updated?

Updated.
(Previously the comment was updated for SPLIT PARTITION command only.)


7.
 >``+SELECT * FROM sales_all WHERE sales_state = 'Warsaw';``
 >may ultimately fall back to using  seqscan?
 >so we need to use
 >``explain(costs off)`` to see if it use indexscan or not.

Corrected.


8.
 >...
 >+  RelationGetRelationName(parent_rel)));
 >this error is unlikely to happen, we can simply use elog(ERROR, ....),
 >rather than  ereport.

Applied.


9.
 >evaluateGeneratedExpressionsAndCheckConstraints seem not necessary?

The "evaluateGeneratedExpressionsAndCheckConstraints" function is used
for both commands (SPLIT and MERGE), so I prefer to keep it
(probably, code duplication is worse).


10.
 >we should make the MergePartitionsMoveRows code pattern aligned with
 >ATRewriteTable.
 >by comparing these two function, i found that before call
 >table_scan_getnextslot we need to switch memory context to
 >EState->ecxt_per_tuple_memor

Thanks, that is correct. Applied.


11.
 >we may need to change checkPartition.
 > ...
 >IMV, the error message pattern should be something like:
 >ERROR: can not merge relation \"%s\"  with other partitions
 >DETAIL:  "sales_apr2022" is not a table
 >HINT:  ALTER TABLE ... MERGE PARTITIONS can only merge partitions
 >don't have sub-partition

This error is generated if the condition
"if (partRel->rd_rel->relkind!= RELKIND_RELATION)"
is true. But error message pattern
"can not merge relation ... with other partitions"
is correct for RELKIND_PARTITIONED_TABLE only. Separate messages for 
each of the relation types (RELKIND_VIEW, RELKIND_FOREIGN_TABLE, ...) 
looks a bit complicated (see example [1]).
Might be we can replace error message "... is not a table" to
"... is not a simple partition"?


12.
 >+checkPartition(Relation rel, Oid partRelOid)
 >"Partition with OID partRelOid must be locked before function call."
 >we can remove this sentence.
 >otherwise, "function call" seems confusing?

Removed.


13.
 >+ cmd->name->relpersistence = rel->rd_rel->relpersistence;
 >seems wrong?
 >comments "stmt->relation" not sure what it refers to?

Applied and corrected the same for SPLIT PARTITION.


14.
 > ...
 >I propose change it to:
 >ereport(ERROR,
 >        errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
 >        errmsg("can not merge partition \"%s\" together with partition
 >\"%s\"",
 >                second_name->relname, first_name->relname),
 >        errdetail("lower bound of partition \"%s\" is not equal to the
 >upper bound of partition \"%s\"",
 >                  second_name->relname, first_name->relname),
 >        errhint("ALTER TABLE ... MERGE PARTITIONS requires the
 >partition bounds to be adjacent."),
 >        parser_errposition(pstate, datum->location));


Changed.


15.
 >buildExpressionExecutionStates seems not needed, same reason as
 >mentioned before,
 >code pattern aligned with ATRewriteTable.

"buildExpressionExecutionStates" function is used for both commands
(SPLIT and MERGE). Probably, is better to keep this function?


16.
 >while at it, also did some minor changes.

Applied.


Links
-----
[1] 
https://github.com/postgres/postgres/blob/989b2e4d5c95f6b183e76f3eb06d2d360651ccf2/src/backend/commands/copyto.c#L649

-- 
With best regards,
Dmitry Koval

Postgres Professional: http://postgrespro.com