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
