Hello, On Wed, 29 Apr 2026 at 11:29, John Naylor <[email protected]> wrote:
>
> Thanks for that. One correction got lost from from the v3 message edits:
>
> errmsg("can not {split/merge} to partition \"%s\" together with
> partition \"%s\"",
> errdetail("lower bound of partition \"%s\" is not equal to the upper
> bound of partition \"%s\"",
>
> Note that this should also change the .sql test comment. That
> emphasizes the need for the follow-on 0002 patch.
>
> Also, I mentioned earlier that I didn't like how these two messages
> are only different by word, but the "split" case sounds awkward this
> way. Also, I didn't really like the fact that one errmsg refers to the
> old partitions, and one refers to the new. My suggestion was:
>
> >> 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.
>
> Another thing that got lost, although less important:
>
> >> - 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.
>
> By "two articles", I meant "The lower bound of ... to the upper bound
> of ...". That's just a suggestion, and not quite a correction like the
> others, but wanted to bring it up while we're changing the text
> anyway.
>
> If it were just something like "Lower bound is not a valid foo", then
> I don't think I'd bother changing it, but with two things, I think it
> sounds better with two "the"s.
>
Thanks for the careful review and for catching the bits I dropped from
the v3 edits. v5 attached, addressing all of the above:
- Use "cannot" everywhere (split, split DEFAULT, split non-DEFAULT,
merge, find partition for split partition row).
- For SPLIT, switch the adjacency error to
errmsg("cannot split partition \"%s\"",
get_rel_name(splitPartOid)),
so it names the old partition, matching the merge wording style.
To make splitPartOid available there, I added an Oid splitPartOid
parameter to check_two_partitions_bounds_range() and pass
InvalidOid from the merge call site (where is_merge is true so
the parameter is unused).
- errdetail now reads "The lower bound of partition \"%s\" is not
equal to the upper bound of partition \"%s\"." (two articles,
capitalized, trailing period).
- errhints now distinguish old vs. new partition bounds:
MERGE: "... requires the old partition bounds to be adjacent."
SPLIT: "... requires the new partition bounds to be adjacent."
- Promote the duplicate secondary errmsg about an existing DEFAULT
partition to errdetail (capitalized, with trailing period).
- Polish the DEFAULT-partition errhint:
"To split a DEFAULT partition, one of the new partitions must
be DEFAULT."
- Change the parser's "DEFAULT partition should be one" to
"cannot specify more than one DEFAULT partition".
0002 is the test-comment cleanup you asked for: the SPLIT/MERGE
regression tests were copying the full ERROR/DETAIL/HINT text into
SQL comments above each failing statement, so any wording change had
to be made twice. The patch replaces those copied lines with a short
"-- ERROR" marker and keeps the descriptive scenario comments.
Thoughts?
Regards,
Ayush
v5-0001-Fix-errmsg-issues-in-ALTER-TABLE-SPLIT-MERGE-PARTITION.patch
Description: Binary data
v5-0002-Simplify-error-comments-in-partition-split-merge-tests.patch
Description: Binary data
