Horton

Thread

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

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

  3. 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/

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

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