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