On Tue, 21 Apr 2026 at 14:21, Ayush Tiwari <[email protected]> wrote:
> Hi, > > Thanks for the detailed review. > > On Tue, 21 Apr 2026 at 12:15, John Naylor <[email protected]> wrote: > >> On Tue, Apr 21, 2026 at 11:34 AM Ayush Tiwari >> <[email protected]> wrote: >> >> >> > Also, I think "can not" can be replaced with "cannot" for consistency >> > throughout? Havent added is as part of this patch though. Thoughts? >> >> Not just consistency, the first spelling is simply wrong, and this >> isn't the only place. >> > > Agreed. I've attached a v2 draft patch fixes all "can not" -> "cannot" > occurrences across > the split/merge partition code: > > - parse_utilcmd.c: "can not split DEFAULT partition" and > "can not split non-DEFAULT partition" > - partbounds.c: "can not merge partition ... together with ..." and > "can not split to partition ... together with ..." > - tablecmds.c: "can not find partition for split partition row" > > >> Taking a brief look at the test files added for this feature >> >> src/test/regress/sql/partition_merge.sql/.out >> src/test/regress/sql/partition_split.sql/.out >> >> ...I noticed that the .sql file has "-- ERROR:" comments that are >> exact copies of the error message, which can't be great for >> maintenance. We don't seem to do that anywhere else, so I'm not sure >> what the motivation was. >> > > Good point. It make sense to remove "-- ERROR:", "-- DETAIL:", and > "-- HINT:" comment lines from both partition_split.sql and > partition_merge.sql (and their corresponding .out files). I've kept the > Descriptive > comments like "-- (space between sections ...)" and > "-- sales_error intersects with ..." . Incorporated this too in patch. > >> >> Case in point, I noticed at least one other grammatical error in a >> message: >> >> $ git grep 'DEFAULT partition should be one' >> src/backend/parser/parse_utilcmd.c: errmsg("DEFAULT partition >> should be one"), >> src/backend/po/de.po:msgid "DEFAULT partition should be one" >> src/test/regress/expected/partition_split.out:-- ERROR DEFAULT >> partition should be one >> src/test/regress/expected/partition_split.out:ERROR: DEFAULT >> partition should be one >> src/test/regress/sql/partition_split.sql:-- ERROR DEFAULT partition >> should be one >> > > Fixed: "DEFAULT partition should be one" is now > "cannot specify more than one DEFAULT partition". > > >> >> That's makes four places that will need to be changed, instead of two, >> and it's easy to overlook the comments. >> >> While I'm looking, >> >> ALTER TABLE sales_range SPLIT PARTITION sales_others INTO >> (PARTITION sales_dec2021 FOR VALUES FROM ('2021-12-01') TO >> ('2022-01-01'), >> PARTITION sales_error FOR VALUES FROM ('2021-12-30') TO ('2022-02-01'), >> PARTITION sales_feb2022 FOR VALUES FROM ('2022-02-01') TO >> ('2022-03-01'), >> PARTITION sales_others DEFAULT); >> >> ERROR: can not split to partition "sales_error" together with >> partition "sales_dec2021" >> >> This seems like it should be >> ERROR: cannot split partition sales_others >> DETAIL: partition "sales_error" overlaps with partition "sales_dec2021" >> >> ...or something like that, maybe others have a better idea, but I find >> the current message confusing. >> >> In short, I think there is a lot more here that needs attention. >> >> > Hmm, I changed the confusing "can not split to partition X > together with partition Y" messages (for both split and merge) to: > > errmsg: "cannot split non-adjacent partitions \"%s\" and \"%s\"" > errdetail: "Lower bound of partition \"%s\" is not equal to upper > bound of partition \"%s\"." > > errmsg: "cannot merge non-adjacent partitions \"%s\" and \"%s\"" > errdetail: "Lower bound of partition \"%s\" is not equal to upper > bound of partition \"%s\"." > > I also removed the > now-redundant errhint lines ("ALTER TABLE ... SPLIT/MERGE PARTITION > requires the partition bounds to be adjacent.") since > the errmsg itself says "non-adjacent". > > Additionally, the errhint for splitting a DEFAULT partition had a > grammar error: "To split DEFAULT partition one of the new partition > must be DEFAULT." is now "To split a DEFAULT partition, one of the new > partitions must be DEFAULT." > > And of course the original fix: the duplicate errmsg() -> errdetail() > in transformPartitionCmdForSplit() with proper capitalization and > trailing period. > > Reattaching patch with right format for cfbot. Regards, Ayush
v2-0001-Fix-errmsg-issues-in-ALTER-TABLE-SPLIT-MERGE-PARTITION.patch
Description: Binary data
