Hi, On Thu, 23 Apr 2026 at 12:24, John Naylor <[email protected]> wrote:
> On Thu, Apr 23, 2026 at 11:04 AM Ayush Tiwari > <[email protected]> wrote: > > --- a/src/backend/partitioning/partbounds.c > +++ b/src/backend/partitioning/partbounds.c > @@ -5030,20 +5030,18 @@ check_two_partitions_bounds_range(Relation parent, > if (is_merge) > ereport(ERROR, > errcode(ERRCODE_INVALID_OBJECT_DEFINITION), > - errmsg("can not merge partition \"%s\" together with partition > \"%s\"", > + errmsg("cannot merge non-adjacent partitions \"%s\" and \"%s\"", > second_name->relname, first_name->relname), > - errdetail("lower bound of partition \"%s\" is not equal to the > upper bound of partition \"%s\"", > + errdetail("Lower bound of partition \"%s\" is not equal to upper > bound of partition \"%s\".", > second_name->relname, first_name->relname), > - errhint("ALTER TABLE ... MERGE PARTITIONS requires the partition > bounds to be adjacent."), > > I don't see anything wrong with the original errmsg (aside from the > spelling correction.) -- "merge X together with Y" is not wrong. Nor > the errhint -- it's somewhat redundant, but it's also general, while > the errdetail is specific. > Makes sense. > > > On Thu, 23 Apr 2026 at 08:28, jian he <[email protected]> > wrote: > > > >> I noticed you removed "the", I'm not sure if that's okay since I am > >> not a native English speaker. > >> > > > > Regarding the removal of "the" from "the upper bound"; I dropped it > > to match the style of the errmsg, which says "upper bound of partition" > > without the article. Happy to restore it if someone prefers the > > original phrasing. > > This is not an improvement to my ears. Omitting the article at the > beginning would be okay, since it can be found in technical/newspaper > style, but with two things the errdetail is a bit awkward without an > article for each thing. > > else > ereport(ERROR, > errcode(ERRCODE_INVALID_OBJECT_DEFINITION), > - errmsg("can not split to partition \"%s\" together with partition > \"%s\"", > + errmsg("cannot split non-adjacent partitions \"%s\" and \"%s\"", > second_name->relname, first_name->relname), > > This new language is backwards. > > I would just do > > errmsg("cannot split partition \"%s\"", > get_rel_name(splitPartOid)), > > ...that way the errmsg's mention the old partition(s), whether the > action is splitting or merging. > > - errdetail("lower bound of partition \"%s\" is not equal to the > upper bound of partition \"%s\"", > + errdetail("Lower bound of partition \"%s\" is not equal to upper > bound of partition \"%s\".", > second_name->relname, first_name->relname), > - errhint("ALTER TABLE ... SPLIT PARTITION requires the partition > bounds to be adjacent."), > parser_errposition(pstate, datum->location)); > > Ditto here: Two articles for the errdetail, and the errhint is not a > problem. Although, perhaps it'd be better if the two errhints said > "old/new partition bounds", respectively, for clarity. > > Also, this patch is getting big and unfocused. Let's split out the > removal of copied ERROR messages in the tests to a separate second > patch. > > Attaching separate patches one with edits, and the other for ERRORs. Please review and let me know. Regards, Ayush
v4-0002-Simplify-error-comments-in-partition-split-merge-tests.patch
Description: Binary data
v4-0001-Fix-errmsg-issues-in-ALTER-TABLE-SPLIT-MERGE-PARTITION.patch
Description: Binary data
