Re: [PATCH] Fix duplicate errmsg in ALTER TABLE SPLIT PARTITION

Hi,

On Thu, 23 Apr 2026 at 12:24, John Naylor <johncnaylorls(at)gmail(dot)com> wrote:

> On Thu, Apr 23, 2026 at 11:04 AM Ayush Tiwari
> <ayushtiwari(dot)slg01(at)gmail(dot)com> 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 <jian(dot)universality(at)gmail(dot)com>
> 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