| From: | John Naylor <johncnaylorls(at)gmail(dot)com> |
|---|---|
| To: | Ayush Tiwari <ayushtiwari(dot)slg01(at)gmail(dot)com> |
| Cc: | jian he <jian(dot)universality(at)gmail(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org>, Daniel Gustafsson <daniel(at)yesql(dot)se> |
| Subject: | Re: [PATCH] Fix duplicate errmsg in ALTER TABLE SPLIT PARTITION |
| Date: | 2026-04-29 05:59:45 |
| Message-ID: | CANWCAZbpVeJrCjc2cTB1s6RDKS2j1=Sd7KWsEud7yG+NjTu_kw@mail.gmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
On Thu, Apr 23, 2026 at 3:23 PM Ayush Tiwari
<ayushtiwari(dot)slg01(at)gmail(dot)com> wrote:
> On Thu, 23 Apr 2026 at 12:24, John Naylor <johncnaylorls(at)gmail(dot)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
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Chao Li | 2026-04-29 06:01:19 | Re: Property graph: fix error handling when dropping non-existent label property |
| Previous Message | Alexandra Wang | 2026-04-29 05:59:28 | Re: Is there value in having optimizer stats for joins/foreignkeys? |