Re: Add SPLIT PARTITION/MERGE PARTITIONS commands

Hi!

This email contains comments to three emails
(2 x 06.06.2025 + 1 x 10.06.2025).

1.
>I am surprised that partition_merge.sql doesn't have much \d+ command.
>so I added two, which is necessary IMHO.

I think that a lot of \d+ commands can make the out-files difficult to
read. But I agree, that test for triggers is useful. Test for triggers
added to test of partition properties.

2.
>Here, StoreConstraints last argument should be set to true?
>see also StoreAttrDefault.

Thanks, this is correct.

>you can use TupleDescGetDefault, build_generation_expression
>to simplify the above code.

Should we use ATTRIBUTE_GENERATED_VIRTUAL [1] and
build_generation_expression [2] here? Function expandTableLikeClause
("CREATE TABLE ... LIKE ..." clause) does not use GENERATED VIRTUAL
yet ...

>Do getAttributesList need to care about pg_attribute.attidentity?
>currently MERGE PARTITION seems to work fine with identity columns,
>this issue i didn't dig deeper.

Probably after commit [3] partition's identity columns shares the
identity space (i.e. underlying sequence) as the corresponding
columns of the partitioned table. So call BuildDescForRelation in
createPartitionTable function should copy pg_attribute.attidentity
for new partition.

>I am wondering right after createPartitionTable,
>do we need a CommandCounterIncrement?
>because later moveMergedTablesRows will use the output of
>createPartitionTable.

We call CommandCounterIncrement in createPartitionTable function right
after heap_create_with_catalog (same code in create_toast_table,
make_new_heap, DefineRelation functions). We need an additional
CommandCounterIncrement call in case we use objects created after this
point. But we probably don't use these objects (in function
moveMergedTablesRows too).

3.
>I only want to allow HEAP_TABLE_AM_OID to be used
>in the merge partition,
>I guess that would avoid unintended consequences.

Thanks for the clarification. Isn't this limitation too strong?
It is very likely that the user will create an AM based on
HEAP_TABLE_AM_OID, in which case the code should work.

>RangeVarGetAndCheckCreationNamespace
>was called first on ATExecMergePartitions, then on
>createPartitionTable. Maybe we can pass the first
>ATExecMergePartitions call result to createPartitionTable to avoid
>calling it twice.

Unfortunately, this is not the case for SPLIT PARTITION. I will think
about it, but I am concerned that the createPartitionTable function
will be passed two related arguments - newPartName and namespaceId
(result of RangeVarGetAndCheckCreationNamespace).

Thanks for
v42-0001-fix-MERGE-PARTITION-with-partitioned-table-not-enforc.no-cfbot!
I forgot about not valid or not enforced constraints.

>cosmetic changes:
>many of the "forach" can change to "foreach_node".
>for example in ATExecMergePartitions.
>we can change
>``foreach(listptr, cmd->partlist)``
>to
>``foreach_node(RangeVar, name, cmd->partlist)`

Changed.

Links.
------
[1] Virtual generated columns,
https://github.com/postgres/postgres/commit/83ea6c54025bea67bcd4949a6d58d3fc11c3e21b
[2] Expand virtual generated columns in the planner,
https://github.com/postgres/postgres/commit/1e4351af329f2949c679a215f63c51d663ecd715
[3] Support identity columns in partitioned tables,
https://github.com/postgres/postgres/commit/699586315704a8268808e3bdba4cb5924a038c49

P.S. 2Junwang Zhao: sorry, I'll write an answer a little later.

--
With best regards,
Dmitry Koval

Postgres Professional: http://postgrespro.com