Hi Fujii, Thanks for your review.
On Wed, Aug 7, 2024 at 9:54 PM Fujii Masao <masao.fu...@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 be updated 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 more appropriate 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
v3-0002-fix-stale-comments.patch
Description: Binary data
v3-0001-support-specify-tablespace-for-each-merged-split.patch
Description: Binary data
