Thread: Support specify tablespace for each merged/split partition
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
--
Regards
Junwang Zhao
Attachment
On Mon, Jul 15, 2024 at 11:19 AM Junwang Zhao <zhjwpku@gmail.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. -- 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. -- /* - * 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. -- +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. -- Please add the commitfest[1] entry if not already done. 1] https://commitfest.postgresql.org/ Regards, Amul
Hi Amul, Thanks for your review. On Mon, Aug 5, 2024 at 8:38 PM Amul Sul <sulamul@gmail.com> wrote: > > On Mon, Jul 15, 2024 at 11:19 AM Junwang Zhao <zhjwpku@gmail.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
On Mon, Aug 5, 2024 at 9:05 PM Junwang Zhao <zhjwpku@gmail.com> wrote:
>
> Hi Amul,
>
> Thanks for your review.
>
> On Mon, Aug 5, 2024 at 8:38 PM Amul Sul <sulamul@gmail.com> wrote:
> >
> > On Mon, Jul 15, 2024 at 11:19 AM Junwang Zhao <zhjwpku@gmail.com> wrote:
> > >
> >[...]
> > 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?
My suggestion is to rewrite the third paragraph as follows, but
someone else might have a better version:
---
The new partitions will also be created in the same tablespace as the parent
if not specified. Also, this function sets the new partition access method
same as parent table access methods (similarly to CREATE TABLE ... PARTITION
OF). It checks that parent and child tables have compatible persistence.
---
> >
> > +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?
>
I think you are correct; my understanding is a bit hazy.
>
> I have added you as a reviewer, hope you don't mind.
Thank you.
Regards,
Amul
On Tue, Aug 6, 2024 at 5:34 PM Amul Sul <sulamul@gmail.com> wrote:
>
> On Mon, Aug 5, 2024 at 9:05 PM Junwang Zhao <zhjwpku@gmail.com> wrote:
> >
> > Hi Amul,
> >
> > Thanks for your review.
> >
> > On Mon, Aug 5, 2024 at 8:38 PM Amul Sul <sulamul@gmail.com> wrote:
> > >
> > > On Mon, Jul 15, 2024 at 11:19 AM Junwang Zhao <zhjwpku@gmail.com> wrote:
> > > >
> > >[...]
> > > 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?
>
> My suggestion is to rewrite the third paragraph as follows, but
> someone else might have a better version:
> ---
> The new partitions will also be created in the same tablespace as the parent
> if not specified. Also, this function sets the new partition access method
> same as parent table access methods (similarly to CREATE TABLE ... PARTITION
> OF). It checks that parent and child tables have compatible persistence.
> ---
I changed to this with minor changes.
> > >
> > > +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?
> >
>
> I think you are correct; my understanding is a bit hazy.
Thanks for your confirmation.
Attached v2 addressed all the problems you mentioned, thanks.
>
> >
> > I have added you as a reviewer, hope you don't mind.
>
> Thank you.
>
> Regards,
> Amul
--
Regards
Junwang Zhao
Attachment
On 2024/08/06 19:28, Junwang Zhao wrote:
> Attached v2 addressed all the problems you mentioned, thanks.
Thanks for updating the patches!
In the ALTER TABLE documentation, v1 patch updated the syntax, but the descriptions for MERGE and SPLIT should also be
updatedto explain the tablespace of new partitions.
+ char *tablespacename; /* name of tablespace, or NULL for default */
PartitionBoundSpec *bound; /* FOR VALUES, if attaching */
This is not the fault of v1 patch, but the comment "if attaching" seems incorrect.
I also noticed the comment for SinglePartitionSpec refers to a different struct name, PartitionDesc. This should be
corrected,and it might be better to include this fix in the v2 patch.
- * PartitionCmd - info for ALTER TABLE/INDEX ATTACH/DETACH PARTITION commands
+ * PartitionCmd - info for ALTER TABLE/INDEX ATTACH/DETACH/MERGE/SPLIT PARTITION commands
How about changing it to "info for ALTER TABLE ATTACH/DETACH/MERGE/SPLIT and ALTER INDEX ATTACH commands" for more
precision?
- RangeVar *name; /* name of partition to attach/detach */
+ RangeVar *name; /* name of partition to
+ * attach/detach/merge/split */
In the case of MERGE, it refers to the name of the partition that the command merges into. So, would "merge into" be
moreappropriate than just "merge" in this comment?
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
Hi Fujii, Thanks for your review. On Wed, Aug 7, 2024 at 9:54 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote: > > > > On 2024/08/06 19:28, Junwang Zhao wrote: > > Attached v2 addressed all the problems you mentioned, thanks. > > Thanks for updating the patches! > > > In the ALTER TABLE documentation, v1 patch updated the syntax, but the descriptions for MERGE and SPLIT should also beupdated to explain the tablespace of new partitions. Updated. > > > + char *tablespacename; /* name of tablespace, or NULL for default */ > PartitionBoundSpec *bound; /* FOR VALUES, if attaching */ > > This is not the fault of v1 patch, but the comment "if attaching" seems incorrect. I checked the gram.y, bound can be DEFAULT, so I think a simple comment like /* a partition bound specification */ may be more proper? > > > I also noticed the comment for SinglePartitionSpec refers to a different struct name, PartitionDesc. This should be corrected,and it might be better to include this fix in the v2 patch. Fixed. > > > - * PartitionCmd - info for ALTER TABLE/INDEX ATTACH/DETACH PARTITION commands > + * PartitionCmd - info for ALTER TABLE/INDEX ATTACH/DETACH/MERGE/SPLIT PARTITION commands > > How about changing it to "info for ALTER TABLE ATTACH/DETACH/MERGE/SPLIT and ALTER INDEX ATTACH commands" for more precision? Yeah, this is more precise, updated. > > > - RangeVar *name; /* name of partition to attach/detach */ > + RangeVar *name; /* name of partition to > + * attach/detach/merge/split */ > > In the case of MERGE, it refers to the name of the partition that the command merges into. So, would "merge into" be moreappropriate than just "merge" in this comment? Agree, changing *merge* to *merge into* directly will unhappy pgindent, so I use comma instead of slash instead. > > Regards, > > -- > Fujii Masao > Advanced Computing Technology Center > Research and Development Headquarters > NTT DATA CORPORATION -- Regards Junwang Zhao
Attachment
Hi Michael, On Tue, Oct 8, 2024 at 11:21 AM Michael Paquier <michael@paquier.xyz> wrote: > > On Thu, Aug 08, 2024 at 09:47:20AM +0800, Junwang Zhao wrote: > > Thanks for your review. > > The SPLIT/MERGE grammar has been reverted in 3890d90c1508, so this > patch concept does not apply anymore. > -- > Michael Yeah, I checked the Commit Fest entry, the status has already been marked as rejected. Thanks for the reminder. -- Regards Junwang Zhao