Re: [PATCH] Fix duplicate errmsg in ALTER TABLE SPLIT PARTITION - Mailing list pgsql-hackers

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