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 -- Sent via pgsql-hackers mailing list (email@example.com) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers