Re: Support specify tablespace for each merged/split partition

Hi Amul,

Thanks for your review.

On Mon, Aug 5, 2024 at 8:38 PM Amul Sul <sulamul(at)gmail(dot)com> wrote:
>
> On Mon, Jul 15, 2024 at 11:19 AM Junwang Zhao <zhjwpku(at)gmail(dot)com> wrote:
> >
> > In [1], it is suggested that it might be a good idea to support
> > specifying the tablespace for each merged/split partition.
> >
> > We can do the following after this feature is supported:
> >
> > CREATE TABLESPACE tblspc LOCATION '/tmp/tblspc';
> > CREATE TABLE t (i int PRIMARY KEY) PARTITION BY RANGE (i);
> > CREATE TABLE tp_0_1 PARTITION OF t FOR VALUES FROM (0) TO (1);
> > CREATE TABLE tp_1_2 PARTITION OF t FOR VALUES FROM (1) TO (2);
> >
> > ALTER TABLE t MERGE PARTITIONS (tp_0_1, tp_1_2) INTO tp_0_2 TABLESPACE tblspc;
> >
> > ALTER TABLE t SPLIT PARTITION tp_0_2 INTO
> > (PARTITION tp_0_1 FOR VALUES FROM (0) TO (1) TABLESPACE tblspc,
> > PARTITION tp_1_2 FOR VALUES FROM (1) TO (2));
> >
> > [1] https://www.postgresql.org/message-id/abaf390b-3320-40a5-8815-ef476db5cfe7@oss.nttdata.com
> >
>
> +1 for this enhancement. Here are few comments for the patch:
>
> - INTO <replaceable class="parameter">partition_name</replaceable>
> + INTO <replaceable
> class="parameter">partition_name</replaceable> [ TABLESPACE
> tablespace_name ]
>
> tablespace_name should be wrapped in the <replaceable> tag, like partition_name.

Will add in next version.

> --
>
> static Relation
> -createPartitionTable(RangeVar *newPartName, Relation modelRel,
> - AlterTableUtilityContext *context)
> +createPartitionTable(RangeVar *newPartName, char *tablespacename,
> +
>
> The comment should mention the tablespace setting in the same way it
> mentions the access method.

I'm not good at wording, can you give some advice?

> --
>
> /*
> - * PartitionCmd - info for ALTER TABLE/INDEX ATTACH/DETACH PARTITION commands
> + * PartitionCmd - info for ALTER TABLE/INDEX
> ATTACH/DETACH/MERGE/SPLIT PARTITION commands
> */
>
> This change should be a separate patch since it makes sense
> independently of your patch. Also, the comments for the "name"
> variable in the same structure need to be updated.

Will be split into a separate patch in the next version.

> --
>
> +SELECT tablename, tablespace FROM pg_tables
> + WHERE tablename IN ('t', 'tp_0_2') AND schemaname = 'partitions_merge_schema'
> + ORDER BY tablename, tablespace;
> + tablename | tablespace
> +-----------+------------------
> + t |
> + tp_0_2 | regress_tblspace
> +(2 rows)
> +
> +SELECT tablename, indexname, tablespace FROM pg_indexes
> + WHERE tablename IN ('t', 'tp_0_2') AND schemaname = 'partitions_merge_schema'
> + ORDER BY tablename, indexname, tablespace;
> + tablename | indexname | tablespace
> +-----------+-------------+------------
> + t | t_pkey |
> + tp_0_2 | tp_0_2_pkey |
> +(2 rows)
> +
>
> This seems problematic to me. The index should be in the same
> tablespace as the table.

I'm not sure about this, it seems to me that partition index will alway
inherit the tablespaceId of its parent(see generateClonedIndexStmt),
do you think we should change the signature of this function?

One thing worth mentioning is that for UNIQUE/PRIMARY KEY,
it allows setting *USING INDEX TABLESPACE tablespace_name*,
I don't think we should change the index tablespace in this case,
what do you think?

> --
>
> Please add the commitfest[1] entry if not already done.
>
> 1] https://commitfest.postgresql.org/

https://commitfest.postgresql.org/49/5157/

I have added you as a reviewer, hope you don't mind.

>
> Regards,
> Amul

--
Regards
Junwang Zhao