Re: Support specify tablespace for each merged/split partition

On Tue, Aug 6, 2024 at 5:34 PM Amul Sul <sulamul(at)gmail(dot)com> wrote:
>
> On Mon, Aug 5, 2024 at 9:05 PM Junwang Zhao <zhjwpku(at)gmail(dot)com> wrote:
> >
> > 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:
> > > >
> > >[...]
> > > 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