Re: [HACKERS] 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