Re: dubious error message from partition.c

On 2017/08/09 3:50, Robert Haas wrote:
> In the original table partitioning commit
> (f0e44751d7175fa3394da2c8f85e3ceb3cdbfe630), we introduced the
> following code, which hasn't changed materially since then:
>
> + if (partition_rbound_cmp(key, lower->datums,
> lower->content, true,
> + upper) >= 0)
> + ereport(ERROR,
> + (errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
> + errmsg("cannot create range partition with empty range"),
> + parser_errposition(pstate, spec->location)));
>
> In retrospect, I'm not thrilled by this error message, for two reasons:
>
> 1. It gives no details, as other nearby messages do. For example,
> further down in the function, we have a message "partition \"%s\"
> would overlap partition \"%s\", which tells you the names of the old
> and new partitions. But this message has no %-escapes at all. It
> tells you neither the name of the partition that you're trying to
> create nor the problematic values. You might object that you can only
> be creating one partition at a time and should therefore know its name
> but (a) you might be running a script and be uncertain which command
> failed and (b) we've talked repeatedly about introducing convenience
> syntax for creating a bunch of partitions along with the parent table,
> and if we do that at some point, then this will become more of an
> issue.

Yeah, the message can sound confusing.

> 2. The message text is, in my opinion, not as clear as it could be.
> The most logical interpretation of that message, ISTM, would be that
> you've specified a lower bound that is equal to the upper bound - and
> you would indeed get this message in that case. However, you'd also
> get it if you specified a lower bound that is GREATER than the upper
> bound, and I think that it's possible somebody might get confused.
> For example:
>
> rhaas=# create table example (a citext) partition by range (a);
> CREATE TABLE
> rhaas=# create table example1 partition of example for values from
> ('Bob') to ('alice');
> ERROR: cannot create range partition with empty range
>
> A user who fails to realize what the comparison semantics of citext
> are might scratch their head at this message. Surely 'Bob' precedes
> 'alice' since 'B' = 66 and 'a' = 97, so what's the matter? This could
> also come up with plain old text if the partition bounds have a
> different collation than the user is expecting.
>
> So, I suggest something like:
>
> "lower bound %s for partition \"%s\" must precede upper bound %s"
>
> e.g. lower bound (Bob) for partition "example1" must precede upper bound (alice)
>
> You could still be confused about why Bob comes before alice (sexism?)
> but you'll at least know which partition is the problem and hopefully
> at least have a clearer notion that the problem is that the system's
> idea about how those bounds are ordered differs from your own.

Hmm, maybe no need to put (Bob) and (alice) in the error message text?

"lower bound for partition \"%s\" must precede upper bound"

Or, we could specify extra information in the detail part in a way that is
perhaps less confusing:

ERROR: invalid range bound specification for partition \"%s\"
DETAIL: specified lower bound %s succeeds upper bound %s

Thoughts?

Thanks,
Amit