Hi Dmitry,
On Tue, Jun 10, 2025 at 6:48 AM Dmitry Koval <d.ko...@postgrespro.ru> wrote:
>
> Hi, Jian He!
>
> Thanks for the suggestions and patches!
> This email contains comments to three emails (05/06/2025).
> I hope to read two emails (for 06/06/2025) tomorrow.
>
> 1.
> >What should we do when any to be merged partition has constraints?
> >...
> >Maybe this is expected, but we need to mention it somewhere and have
> >some tests on it saying that MERGE PARTITIONS will effectively drop
> >the partitions, so if any object depends on that partition
> >then MERGE PARTITIONS can not be done.
>
> Added following phrases to the documentation (I hope this should be
> enough?):
>
> If merged partitions have individual constraints, those constraints will
> be dropped because command uses partitioned table as a model to create
> the constraints.
> If merged partitions have some objects dependent on them, the command
> can not be done (CASCADE is not used, an error will be returned).
>
>
> 2.
> > ... so this error check can be performed as early as the
> >transformPartitionCmdForMerge stage?
>
> Function createPartitionTable will be used for various other cases
> besides MERGE PARTITIONS: for SPLIT PARTITION, for PARTITION BY
> REFERENCE (I hope).
> So I think it's better to minimize the amount of code and not move the
> same one check into different functions (transformPartitionCmdForMerge,
> transformPartitionCmdForSplit, ...).
>
>
> 3.
> >i think, we can do the following way:
> >if (modelRel->rd_rel->relam)
> > elog(ERROR, "error");
> >relamId = modelRel->rd_rel->relam;
>
> Can you clarify what is reason to change the current AM-logic for
> creating a new partition?
>
> + /* Look up the access method for new relation. */
> + relamId = (modelRel->rd_rel->relam != InvalidOid) ?
> modelRel->rd_rel->relam : HEAP_TABLE_AM_OID;
>
> (If AM is set for a partitioned table, then use it, otherwise use AM for
> heap tables.)
>
>
> 4.
> > Attached is some refactoring in moveMergedTablesRows, hope it's
> straightforward.
>
> Thanks, these changes are useful.
>
>
> 5.
> >bug in transformPartitionCmdForMerge "equal(name, name2))"
> > ...
> >ALTER TABLE sales_range MERGE PARTITIONS (sales_feb2022,
> >public.sales_feb2022) INTO sales_feb_mar2022;
> >ERROR: lower bound of partition "sales_feb2022" conflicts with upper
> >bound of previous partition "sales_feb2022"
> >in this context. "sales_feb2022" is the same as "public.sales_feb2022".
>
> Added check and test for this case.
>
>
> 6.
> >When using ALTER TABLE ... MERGE PARTITIONS, some of the new
> >partition's properties will not be inherited from to be merged
> >partitions; instead, they will be directly copied from the root
> >partitioned table.
> >So we need to test this behavior.
> >The attached test file is for test table properties:
> >(COMMENTS, COMPRESSION, DEFAULTS, GENERATED, STATISTICS, STORAGE).
>
> Some tests already exist (GENERATED, DEFAULTS) - see
> partition_merge.sql, lines after:
>
> +-- Test for:
> +-- * composite partition key;
> +-- * GENERATED column;
> +-- * column with DEFAULT value.
> ...
>
> But the complex test is probably also interesting.
> Test added.
>
> --
>
> Similar changes are made for the second commit (SPLIT PARTITION).
>
> --
> With best regards,
> Dmitry Koval
>
> Postgres Professional: http://postgrespro.com
I had one trivial comment and a question if you don't mind.
+ /* If existing rel is temp, it must belong to this session */
+ if (modelRel->rd_rel->relpersistence == RELPERSISTENCE_TEMP &&
+ !modelRel->rd_islocaltemp)
+ ereport(ERROR,
+ errcode(ERRCODE_WRONG_OBJECT_TYPE),
+ errmsg("cannot create as partition of temporary relation of another
session"));
Would it be better to use RELATION_IS_OTHER_TEMP in this case?
I noticed that while other parts of tablecmds.c don’t use the macro,
all other files consistently use RELATION_IS_OTHER_TEMP.
+ /*
+ * We intended to create the partition with the same persistence as the
+ * parent table, but we still need to recheck because that might be
+ * affected by the search_path. If the parent is permanent, so must be
+ * all of its partitions.
+ */
I have trouble understanding how this is possible, can you kindly
give me some guidance on this logic?
--
Regards
Junwang Zhao
