On Tue, Aug 6, 2024 at 5:34 PM Amul Sul <sula...@gmail.com> wrote: > > On Mon, Aug 5, 2024 at 9:05 PM Junwang Zhao <zhjw...@gmail.com> wrote: > > > > Hi Amul, > > > > Thanks for your review. > > > > On Mon, Aug 5, 2024 at 8:38 PM Amul Sul <sula...@gmail.com> wrote: > > > > > > On Mon, Jul 15, 2024 at 11:19 AM Junwang Zhao <zhjw...@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
v2-0001-support-specify-tablespace-for-each-merged-split.patch
Description: Binary data
v2-0002-fix-stale-comments.patch
Description: Binary data
