| From: | Ayush Tiwari <ayushtiwari(dot)slg01(at)gmail(dot)com> |
|---|---|
| To: | jian he <jian(dot)universality(at)gmail(dot)com> |
| Cc: | John Naylor <johncnaylorls(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-22 11:06:23 |
| Message-ID: | CAJTYsWXv3Uwqmx1K3WohueEJNtOQ0N3q0dzTVRx6z7RowW4a6A@mail.gmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
On Wed, 22 Apr 2026 at 13:49, Ayush Tiwari <ayushtiwari(dot)slg01(at)gmail(dot)com>
wrote:
> Hi,
>
> Thanks for taking a look at the patch.
>
> On Wed, 22 Apr 2026 at 12:59, jian he <jian(dot)universality(at)gmail(dot)com> wrote:
>
>> On Tue, Apr 21, 2026 at 5:37 PM Ayush Tiwari
>> <ayushtiwari(dot)slg01(at)gmail(dot)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
| Attachment | Content-Type | Size |
|---|---|---|
| v3-0001-Fix-errmsg-issues-in-ALTER-TABLE-SPLIT-MERGE-PARTITION.patch | application/octet-stream | 61.8 KB |
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Dean Rasheed | 2026-04-22 11:07:20 | Re: [BUG]: WHERE CURRENT OF cursor fail on tables that have virtual generated columns |
| Previous Message | Matheus Alcantara | 2026-04-22 11:05:47 | Re: MERGE PARTITIONS and DEPENDS ON EXTENSION. |