[PATCH] Fix duplicate errmsg in ALTER TABLE SPLIT PARTITION

On 4/21/2026 12:34 PM, Ayush Tiwari wrote:

Hi hackers,

The ereport() in transformPartitionCmdForSplit() that fires when
splitting a non-default partition while a default partition already
exists has two errmsg() calls:

    ereport(ERROR,
            errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
            errmsg("can not split non-DEFAULT partition \"%s\"", ...),
            errmsg("new partition cannot be DEFAULT because ..."),
            parser_errposition(...));

The second one should had been errdetail()

The attached patch changes the second errmsg() to errdetail().

Also, I think "can not" can be replaced with "cannot" for consistency
throughout? Havent added is as part of this patch though. Thoughts?

Regards,
Ayush

Good catch. I agree the second one should be a detail. The first letter
has been capitalized, and a period is added. So, the patch looks good to me.

Regards,
Yuchen Li

On Tue, Apr 21, 2026 at 11:34 AM Ayush Tiwari
<ayushtiwari.slg01@gmail.com> wrote:

Hi hackers,

The ereport() in transformPartitionCmdForSplit() that fires when
splitting a non-default partition while a default partition already
exists has two errmsg() calls:

ereport(ERROR,
errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
errmsg("can not split non-DEFAULT partition \"%s\"", ...),
errmsg("new partition cannot be DEFAULT because ..."),
parser_errposition(...));

The second one should had been errdetail()

Seems right.

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.

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.

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

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.

--
John Naylor
Amazon Web Services

Hi,

Thanks for the detailed review.

On Tue, 21 Apr 2026 at 12:15, John Naylor <johncnaylorls@gmail.com> wrote:

On Tue, Apr 21, 2026 at 11:34 AM Ayush Tiwari
<ayushtiwari.slg01@gmail.com> 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.

Regards,
Ayush

On Tue, 21 Apr 2026 at 14:21, Ayush Tiwari <ayushtiwari.slg01@gmail.com>
wrote:

Hi,

Thanks for the detailed review.

On Tue, 21 Apr 2026 at 12:15, John Naylor <johncnaylorls@gmail.com> wrote:

On Tue, Apr 21, 2026 at 11:34 AM Ayush Tiwari
<ayushtiwari.slg01@gmail.com> 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

On Tue, Apr 21, 2026 at 5:37 PM Ayush Tiwari
<ayushtiwari.slg01@gmail.com> wrote:

Reattaching patch with right format for cfbot.

--- 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."),

- errhint("ALTER TABLE ... SPLIT PARTITION requires the partition
bounds to be adjacent."),

I am not so sure these errhint are redundant, maybe the errdeatil is redundant.
I am ok with:
+ errmsg("cannot merge non-adjacent partitions \"%s\" and \"%s\"",
    second_name->relname, first_name->relname),
+ errhint("ALTER TABLE ... MERGE PARTITIONS requires the partition
bounds to be adjacent."),

in partition_split.sql, partition_merge.sql,
I agree with that.sql file has "-- ERROR:" comments that are
exact copies of the error message, is not great. But you suddenly delete all
these comments seems not good.

We can add the `-- ERROR` comment suffix, as used in
contrib/file_fdw/sql/file_fdw.sql
or group them and add a comment like ``-- none of the following should
be accepted``,
as seen in src/test/regress/sql/arrays.sql.

--
jian
https://www.enterprisedb.com/

Hi,

Thanks for taking a look at the patch.

On Wed, 22 Apr 2026 at 12:59, jian he <jian.universality@gmail.com> wrote:

On Tue, Apr 21, 2026 at 5:37 PM Ayush Tiwari
<ayushtiwari.slg01@gmail.com> wrote:

Reattaching patch with right format for cfbot.

I am not so sure these errhint are redundant, maybe the errdeatil is
redundant.
I am ok with:
+ errmsg("cannot merge non-adjacent partitions \"%s\" and \"%s\"",
second_name->relname, first_name->relname),
+ errhint("ALTER TABLE ... MERGE PARTITIONS requires the partition
bounds to be adjacent."),

I see your point, but I'd lean toward keeping errdetail over errhint
here. The errdetail tells the user *which* specific bounds don't match
("lower bound of partition B is not equal to upper bound of partition
A"), which is useful when merging 3+ partitions — the user can identify
the exact problem pair. The errhint ("bounds must be adjacent") mostly
restates what the errmsg ("non-adjacent") already says.

That said, I don't feel strongly about it. I could also keep both
(errdetail + errhint) if you think that's better, though it does get
verbose. What do you think?

in partition_split.sql, partition_merge.sql,
I agree with that.sql file has "-- ERROR:" comments that are
exact copies of the error message, is not great. But you suddenly delete
all
these comments seems not good.

We can add the `-- ERROR` comment suffix, as used in
contrib/file_fdw/sql/file_fdw.sql
or group them and add a comment like ``-- none of the following should
be accepted``,
as seen in src/test/regress/sql/arrays.sql.

Good point, removing them all without replacement does make the tests
harder to skim. I'll update v3 to use short markers like "-- should
fail" instead of the exact error text, similar to what file_fdw.sql
does. That way there's still a visual signal that the statement is
expected to error, without the maintenance burden of duplicating the
exact message.

Regards,
Ayush

On Wed, 22 Apr 2026 at 13:49, Ayush Tiwari <ayushtiwari.slg01@gmail.com>
wrote:

Hi,

Thanks for taking a look at the patch.

On Wed, 22 Apr 2026 at 12:59, jian he <jian.universality@gmail.com> wrote:

On Tue, Apr 21, 2026 at 5:37 PM Ayush Tiwari
<ayushtiwari.slg01@gmail.com> wrote:

Reattaching patch with right format for cfbot.

I am not so sure these errhint are redundant, maybe the errdeatil is
redundant.
I am ok with:
+ errmsg("cannot merge non-adjacent partitions \"%s\" and \"%s\"",
second_name->relname, first_name->relname),
+ errhint("ALTER TABLE ... MERGE PARTITIONS requires the partition
bounds to be adjacent."),

I see your point, but I'd lean toward keeping errdetail over errhint
here. The errdetail tells the user *which* specific bounds don't match
("lower bound of partition B is not equal to upper bound of partition
A"), which is useful when merging 3+ partitions — the user can identify
the exact problem pair. The errhint ("bounds must be adjacent") mostly
restates what the errmsg ("non-adjacent") already says.

That said, I don't feel strongly about it. I could also keep both
(errdetail + errhint) if you think that's better, though it does get
verbose. What do you think?

in partition_split.sql, partition_merge.sql,
I agree with that.sql file has "-- ERROR:" comments that are
exact copies of the error message, is not great. But you suddenly delete
all
these comments seems not good.

We can add the `-- ERROR` comment suffix, as used in
contrib/file_fdw/sql/file_fdw.sql
or group them and add a comment like ``-- none of the following should
be accepted``,
as seen in src/test/regress/sql/arrays.sql.

Good point, removing them all without replacement does make the tests
harder to skim. I'll update v3 to use short markers like "-- should
fail" instead of the exact error text, similar to what file_fdw.sql
does. That way there's still a visual signal that the statement is
expected to error, without the maintenance burden of duplicating the
exact message.

Attaching v3 patch.

Regards,
Ayush

On Wed, Apr 22, 2026 at 7:06 PM Ayush Tiwari
<ayushtiwari.slg01@gmail.com> wrote:

Attaching v3 patch.

hi.

V3 looks good to me.
The error message already conveyed the failure reasoning.
Just adding comment `-- ERROR` to partition_merge.sql, partition_split.sql
is enough, I think.

- 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."),

The errdetail already explicitly explains the failure reason, removing
the errhint should be fine, I think.
I noticed you removed "the", I'm not sure if that's okay since I am
not a native English speaker.

--
jian
https://www.enterprisedb.com/

Hi,

Thanks for the review.

On Thu, 23 Apr 2026 at 08:28, jian he <jian.universality@gmail.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.

Regards,
Ayush

On Thu, Apr 23, 2026 at 11:04 AM Ayush Tiwari
<ayushtiwari.slg01@gmail.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.

On Thu, 23 Apr 2026 at 08:28, jian he <jian.universality@gmail.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.

--
John Naylor
Amazon Web Services

Hi,

On Thu, 23 Apr 2026 at 12:24, John Naylor <johncnaylorls@gmail.com> wrote:

On Thu, Apr 23, 2026 at 11:04 AM Ayush Tiwari
<ayushtiwari.slg01@gmail.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.universality@gmail.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

Hi,

Attaching separate patches one with edits, and the other for ERRORs.
Please review and let me know.

I have registered this patch set in the CommitFest for tracking:
https://commitfest.postgresql.org/patch/6694/

Regards,
Ayush

On Thu, Apr 23, 2026 at 3:23 PM Ayush Tiwari
<ayushtiwari.slg01@gmail.com> wrote:

On Thu, 23 Apr 2026 at 12:24, John Naylor <johncnaylorls@gmail.com> wrote:

Attaching separate patches one with edits, and the other for ERRORs.
Please review and let me know.

[v4]

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.

--
John Naylor
Amazon Web Services

Hello,

On Wed, 29 Apr 2026 at 11:29, John Naylor <johncnaylorls@gmail.com> 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

On Wed, Apr 29, 2026 at 5:03 PM Ayush Tiwari
<ayushtiwari.slg01@gmail.com> wrote:

- 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).

If we have splitPartOid, then the boolean is_merge is redundant and
can be removed, right? To keep the intent clear we can add a local
variable

bool is_merge = (splitPartOid == NULL ? true : false);

Other than that, both patches LGTM.

Observation:

-        errmsg("can not split non-DEFAULT partition \"%s\"",
+        errmsg("cannot split non-DEFAULT partition \"%s\"",
              get_rel_name(splitPartOid)),
-        errmsg("new partition cannot be DEFAULT because...
+        errdetail("New partition cannot be DEFAULT because...

-ERROR:  new partition cannot be DEFAULT because DEFAULT partition
"sales_others" already exists
+ERROR:  cannot split non-DEFAULT partition "sales_all"
 LINE 5:    PARTITION sales_others2 DEFAULT);
                      ^
+DETAIL:  New partition cannot be DEFAULT because DEFAULT partition
"sales_others" already exists.

If there are two errmsg's back-to-back, only the second one displays.
Maybe some automated tooling can detect cases like this going forward?

--
John Naylor
Amazon Web Services

Hi,

On Tue, 5 May 2026 at 12:45, John Naylor <johncnaylorls@gmail.com> wrote:

On Wed, Apr 29, 2026 at 5:03 PM Ayush Tiwari
<ayushtiwari.slg01@gmail.com> wrote:

- 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).

If we have splitPartOid, then the boolean is_merge is redundant and
can be removed, right? To keep the intent clear we can add a local
variable

bool is_merge = (splitPartOid == NULL ? true : false);

Other than that, both patches LGTM.

Thanks for reviewing.

v6 attached, addressing the remaining point. In 0001,
check_two_partitions_bounds_range() no longer takes an is_merge
argument. The merge call site passes InvalidOid, the split call site
passes splitPartOid, and the helper derives a local is_merge value from
that. I used OidIsValid(splitPartOid) rather than a NULL comparison,
since splitPartOid is an Oid.

If there are two errmsg's back-to-back, only the second one displays.
Maybe some automated tooling can detect cases like this going forward?

I agree on this, I do not know much about Linters PG has, can check.

Regards,
Ayush

On Tue, May 5, 2026 at 3:15 PM John Naylor <johncnaylorls@gmail.com> wrote:

Observation:

-        errmsg("can not split non-DEFAULT partition \"%s\"",
+        errmsg("cannot split non-DEFAULT partition \"%s\"",
get_rel_name(splitPartOid)),
-        errmsg("new partition cannot be DEFAULT because...
+        errdetail("New partition cannot be DEFAULT because...

-ERROR:  new partition cannot be DEFAULT because DEFAULT partition
"sales_others" already exists
+ERROR:  cannot split non-DEFAULT partition "sales_all"
LINE 5:    PARTITION sales_others2 DEFAULT);
^
+DETAIL:  New partition cannot be DEFAULT because DEFAULT partition
"sales_others" already exists.

If there are two errmsg's back-to-back, only the second one displays.
Maybe some automated tooling can detect cases like this going forward?

Add an Assert in function errmsg, it will crashes the server when two
errmsg back-to-back:

Assert(edata->message == NULL);
EVALUATE_MESSAGE(edata->domain, message, false, true);

Function FreeErrorDataContents change to
```
if (edata->message)
{
pfree(edata->message);
edata->message = NULL;
}
```

Most of the tests succeeded on my local machines.
Ok: 373
Expected Fail: 0
Fail: 0
Unexpected Pass: 0
Skipped: 23
Timeout: 0

I'm not sure if ereport_domain acts differently when
HAVE_PG_INTEGER_CONSTANT_P is true.

--
jian
https://www.enterprisedb.com/

On Tue, May 5, 2026 at 3:57 PM Ayush Tiwari <ayushtiwari.slg01@gmail.com> wrote:

v6 attached, addressing the remaining point.

I've pushed these with some changes:

Additional corrections:

"can only merge partitions don't have sub-partitions"
-> "that don't...".

"new partition \"%s\" cannot have this value because split partition
\"%s\" does not have"
-> "does not have it"

ERROR: cannot split DEFAULT partition "sales_others"
LINE 2: (PARTITION sales_dec2021 FOR VALUES FROM ('2021-12-01') TO...
^
HINT: To split a DEFAULT partition, one of the new partitions must be DEFAULT.

-> The caret above was pointing to a seemingly-random non-default partition.

"new partitions combined partition bounds..."
-> needed an apostrophe

In 0001,
check_two_partitions_bounds_range() no longer takes an is_merge
argument. The merge call site passes InvalidOid, the split call site
passes splitPartOid, and the helper derives a local is_merge value from
that. I used OidIsValid(splitPartOid) rather than a NULL comparison,
since splitPartOid is an Oid.

In the end, I decided to split this part out into the attached, and
have not committed it since the original wasn't really in error, just
sounded a bit off. I also found a couple other places that could use
wordsmithing as well, but it's not as clear-cut:

ERROR: new partition "sales_west" cannot have this value because
split partition "sales_all" does not have
LINE 2: ...st FOR VALUES IN ('Lisbon', 'New York', 'Madrid', 'Melbourne...
^
-> It seems weird to have "this value" in the errmsg. Sure, the caret
points to the right place, but the message seems better to state "new
partition X contains a value not found in split partition Y". Other
places in the split/merge code do quote values in messages, so maybe
we can here as well? Not sure if it matters much.

"new partition \"%s\" would overlap with another (not split) partition \"%s\"
-> "another (not split)" might be better as "an existing", but I'm
open to other opinions.

--
John Naylor
Amazon Web Services

Hi,

On Thu, 7 May 2026 at 17:59, John Naylor <johncnaylorls@gmail.com> wrote:

On Tue, May 5, 2026 at 3:57 PM Ayush Tiwari <ayushtiwari.slg01@gmail.com>
wrote:

v6 attached, addressing the remaining point.

I've pushed these with some changes:

Additional corrections:

"can only merge partitions don't have sub-partitions"
-> "that don't...".

"new partition \"%s\" cannot have this value because split partition
\"%s\" does not have"
-> "does not have it"

ERROR: cannot split DEFAULT partition "sales_others"
LINE 2: (PARTITION sales_dec2021 FOR VALUES FROM ('2021-12-01') TO...
^
HINT: To split a DEFAULT partition, one of the new partitions must be
DEFAULT.

-> The caret above was pointing to a seemingly-random non-default
partition.

"new partitions combined partition bounds..."
-> needed an apostrophe

In 0001,
check_two_partitions_bounds_range() no longer takes an is_merge
argument. The merge call site passes InvalidOid, the split call site
passes splitPartOid, and the helper derives a local is_merge value from
that. I used OidIsValid(splitPartOid) rather than a NULL comparison,
since splitPartOid is an Oid.

In the end, I decided to split this part out into the attached, and
have not committed it since the original wasn't really in error, just
sounded a bit off. I also found a couple other places that could use
wordsmithing as well, but it's not as clear-cut:

ERROR: new partition "sales_west" cannot have this value because
split partition "sales_all" does not have
LINE 2: ...st FOR VALUES IN ('Lisbon', 'New York', 'Madrid', 'Melbourne...
^
-> It seems weird to have "this value" in the errmsg. Sure, the caret
points to the right place, but the message seems better to state "new
partition X contains a value not found in split partition Y". Other
places in the split/merge code do quote values in messages, so maybe
we can here as well? Not sure if it matters much.

"new partition \"%s\" would overlap with another (not split) partition
\"%s\"
-> "another (not split)" might be better as "an existing", but I'm
open to other opinions.

Thank you so much for the updates and help with this John!

Regards,
Ayush