Thread
Commits
Same data as JSON:
GET /api/v1/messages/:b64id/commits
the thread's linked commits as JSON, with link sources.
API reference →
-
Rationalize error comments in partition split/merge tests
- ecb2508aaf9b 19 (unreleased) landed
-
Message corrections for partition split/merge commands
- 52e629be95fc 19 (unreleased) landed
-
[PATCH] Fix duplicate errmsg in ALTER TABLE SPLIT PARTITION
Ayush Tiwari <ayushtiwari.slg01@gmail.com> — 2026-04-21T04:34:11Z
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 -
Re: [PATCH] Fix duplicate errmsg in ALTER TABLE SPLIT PARTITION
Yuchen Li <liyuchen_xyz@163.com> — 2026-04-21T05:40:41Z
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 -
Re: [PATCH] Fix duplicate errmsg in ALTER TABLE SPLIT PARTITION
John Naylor <johncnaylorls@gmail.com> — 2026-04-21T06:45:46Z
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 -
Re: [PATCH] Fix duplicate errmsg in ALTER TABLE SPLIT PARTITION
Ayush Tiwari <ayushtiwari.slg01@gmail.com> — 2026-04-21T08:51:06Z
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 -
Re: [PATCH] Fix duplicate errmsg in ALTER TABLE SPLIT PARTITION
Ayush Tiwari <ayushtiwari.slg01@gmail.com> — 2026-04-21T09:36:41Z
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 -
Re: [PATCH] Fix duplicate errmsg in ALTER TABLE SPLIT PARTITION
jian he <jian.universality@gmail.com> — 2026-04-22T07:28:35Z
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/ -
Re: [PATCH] Fix duplicate errmsg in ALTER TABLE SPLIT PARTITION
Ayush Tiwari <ayushtiwari.slg01@gmail.com> — 2026-04-22T08:19:22Z
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 -
Re: [PATCH] Fix duplicate errmsg in ALTER TABLE SPLIT PARTITION
Ayush Tiwari <ayushtiwari.slg01@gmail.com> — 2026-04-22T11:06:23Z
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 -
Re: [PATCH] Fix duplicate errmsg in ALTER TABLE SPLIT PARTITION
jian he <jian.universality@gmail.com> — 2026-04-23T02:57:23Z
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/ -
Re: [PATCH] Fix duplicate errmsg in ALTER TABLE SPLIT PARTITION
Ayush Tiwari <ayushtiwari.slg01@gmail.com> — 2026-04-23T04:04:42Z
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
-
Re: [PATCH] Fix duplicate errmsg in ALTER TABLE SPLIT PARTITION
John Naylor <johncnaylorls@gmail.com> — 2026-04-23T06:54:20Z
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 -
Re: [PATCH] Fix duplicate errmsg in ALTER TABLE SPLIT PARTITION
Ayush Tiwari <ayushtiwari.slg01@gmail.com> — 2026-04-23T08:23:26Z
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 -
Re: [PATCH] Fix duplicate errmsg in ALTER TABLE SPLIT PARTITION
Ayush Tiwari <ayushtiwari.slg01@gmail.com> — 2026-04-28T17:34:03Z
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
-
Re: [PATCH] Fix duplicate errmsg in ALTER TABLE SPLIT PARTITION
John Naylor <johncnaylorls@gmail.com> — 2026-04-29T05:59:45Z
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 -
Re: [PATCH] Fix duplicate errmsg in ALTER TABLE SPLIT PARTITION
Ayush Tiwari <ayushtiwari.slg01@gmail.com> — 2026-04-29T10:03:20Z
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 -
Re: [PATCH] Fix duplicate errmsg in ALTER TABLE SPLIT PARTITION
John Naylor <johncnaylorls@gmail.com> — 2026-05-05T07:15:37Z
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 -
Re: [PATCH] Fix duplicate errmsg in ALTER TABLE SPLIT PARTITION
Ayush Tiwari <ayushtiwari.slg01@gmail.com> — 2026-05-05T08:56:46Z
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 -
Re: [PATCH] Fix duplicate errmsg in ALTER TABLE SPLIT PARTITION
jian he <jian.universality@gmail.com> — 2026-05-05T11:52:35Z
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/ -
Re: [PATCH] Fix duplicate errmsg in ALTER TABLE SPLIT PARTITION
John Naylor <johncnaylorls@gmail.com> — 2026-05-07T12:29:19Z
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 -
Re: [PATCH] Fix duplicate errmsg in ALTER TABLE SPLIT PARTITION
Ayush Tiwari <ayushtiwari.slg01@gmail.com> — 2026-05-07T13:06:37Z
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