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