Re: [PATCH] Fix duplicate errmsg in ALTER TABLE SPLIT PARTITION

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 08:19:22
Message-ID: CAJTYsWUzT0xBpJgiCWV2W+gra6_Ck73NhT661OtnOEoZLQixmQ@mail.gmail.com
Views: Whole Thread | Raw Message | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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.

Regards,
Ayush

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Chao Li 2026-04-22 08:57:32 Re: pg_test_timing: fix unit typo and widen diff type
Previous Message Arseniy Mukhin 2026-04-22 08:14:52 Re: amcheck support for BRIN indexes