Horton

Thread

Commits

  1. Improve the error message when creating an empty range partition.

  1. dubious error message from partition.c

    Robert Haas <robertmhaas@gmail.com> — 2017-08-08T18:50:36Z

    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.

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.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  2. Re: dubious error message from partition.c

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2017-08-09T00:31:17Z

    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

  3. Re: dubious error message from partition.c

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-08-09T03:34:13Z

    Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> writes:
> On 2017/08/09 3:50, Robert Haas wrote:
>> 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.
>> ...
>> So, I suggest something like:
>> "lower bound %s for partition \"%s\" must precede upper bound %s"

> 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

+1 for doing it more or less like that.  One of our basic message style
guidelines is that primary error texts shouldn't be very long, and it'd be
easy to break that rule if we embed data values in it.

A small suggestion is that it'd be better to write it like "Specified
upper bound \"%s\" precedes lower bound \"%s\"."  I think "succeeds" has
more alternate meanings than "precedes", so the wording you have seems
more confusing than it needs to be.  (Of course, the situation could be
the opposite in other languages, but translators have the ability to
reverse the ordering if they need to.)

Or you could just go with "follows" instead of "succeeds".

			regards, tom lane

  4. Re: dubious error message from partition.c

    David G. Johnston <david.g.johnston@gmail.com> — 2017-08-09T04:03:00Z

    On Tue, Aug 8, 2017 at 8:34 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

> A small suggestion is that it'd be better to write it like "Specified
> upper bound \"%s\" precedes lower bound \"%s\"."  I think "succeeds" has
> more alternate meanings than "precedes", so the wording you have seems
> more confusing than it needs to be.  (Of course, the situation could be
> the opposite in other languages, but translators have the ability to
> reverse the ordering if they need to.)
>
> Or you could just go with "follows" instead of "succeeds".
>

​"exceeds" seems to be the word the original sentence was looking for.  If
using a single word I kinda like reversing the direction and using
"precedes"​ though.  "follows" makes it sound like a puppy :)

"is greater than" is a bit more verbose but an option as well - one that
seems more natural in this space - and yes I've reverted back to
lower->upper with this wording.  I think the hard "x" in exceeds is what
turned me off to it.

David J.

  5. Re: dubious error message from partition.c

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2017-08-09T05:49:49Z

    On 2017/08/09 13:03, David G. Johnston wrote:
> On Tue, Aug 8, 2017 at 8:34 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 
>> A small suggestion is that it'd be better to write it like "Specified
>> upper bound \"%s\" precedes lower bound \"%s\"."  I think "succeeds" has
>> more alternate meanings than "precedes", so the wording you have seems
>> more confusing than it needs to be.  (Of course, the situation could be
>> the opposite in other languages, but translators have the ability to
>> reverse the ordering if they need to.)
>>
>> Or you could just go with "follows" instead of "succeeds".
>>
> 
> ​"exceeds" seems to be the word the original sentence was looking for.  If
> using a single word I kinda like reversing the direction and using
> "precedes"​ though.  "follows" makes it sound like a puppy :)
> 
> "is greater than" is a bit more verbose but an option as well - one that
> seems more natural in this space - and yes I've reverted back to
> lower->upper with this wording.  I think the hard "x" in exceeds is what
> turned me off to it.

I went with the Tom's suggested "Specified upper bound %s precedes lower
bound %s." in the attached patch.

Although, we can also consider a message along the lines suggested by
David: "Specified upper bound is less than (less than or equal to) lower
bound." or "Specified lower bound is greater than (greater than or equal
to) upper bound", because the "precedes" version looks a bit odd in the
following, for example:

 CREATE TABLE fail_part PARTITION OF range_parted2 FOR VALUES FROM (1) TO (1);
-ERROR:  cannot create range partition with empty range
+ERROR:  invalid range bound specification for partition "fail_part"
+DETAIL:  Specified upper bound (1) precedes lower bound (1).

In any case, for someone to make sense of why that leads to an empty
range, they have to remember the rule that the upper bound is an exclusive
bound.

Thanks,
Amit

  6. Re: dubious error message from partition.c

    Robert Haas <robertmhaas@gmail.com> — 2017-08-09T12:03:54Z

    On Tue, Aug 8, 2017 at 11:34 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> A small suggestion is that it'd be better to write it like "Specified
> upper bound \"%s\" precedes lower bound \"%s\"."  I think "succeeds" has
> more alternate meanings than "precedes", so the wording you have seems
> more confusing than it needs to be.  (Of course, the situation could be
> the opposite in other languages, but translators have the ability to
> reverse the ordering if they need to.)

I think that doesn't quite work, because the failure is caused by LB
<= UB, not LB < UB.  We could fix that by writing "precedes or equals"
but that seems lame.  Maybe:

Lower bound %s does not precede upper bound %s.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  7. Re: dubious error message from partition.c

    Dean Rasheed <dean.a.rasheed@gmail.com> — 2017-08-09T18:56:12Z

    On 9 August 2017 at 13:03, Robert Haas <robertmhaas@gmail.com> wrote:
> On Tue, Aug 8, 2017 at 11:34 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> A small suggestion is that it'd be better to write it like "Specified
>> upper bound \"%s\" precedes lower bound \"%s\"."  I think "succeeds" has
>> more alternate meanings than "precedes", so the wording you have seems
>> more confusing than it needs to be.  (Of course, the situation could be
>> the opposite in other languages, but translators have the ability to
>> reverse the ordering if they need to.)
>
> I think that doesn't quite work, because the failure is caused by LB
> <= UB, not LB < UB.  We could fix that by writing "precedes or equals"
> but that seems lame.  Maybe:
>
> Lower bound %s does not precede upper bound %s.
>

There was an earlier suggestion to use "greater than or equal to". I
think that would work quite well:

ERROR:  invalid range bounds for partition \"%s\"
DETAIL:  lower bound %s is greater than or equal to upper bound %s.

Regards,
Dean

  8. Re: dubious error message from partition.c

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2017-08-09T20:38:21Z

    Dean Rasheed wrote:
> On 9 August 2017 at 13:03, Robert Haas <robertmhaas@gmail.com> wrote:
> > On Tue, Aug 8, 2017 at 11:34 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >> A small suggestion is that it'd be better to write it like "Specified
> >> upper bound \"%s\" precedes lower bound \"%s\"."  I think "succeeds" has
> >> more alternate meanings than "precedes", so the wording you have seems
> >> more confusing than it needs to be.  (Of course, the situation could be
> >> the opposite in other languages, but translators have the ability to
> >> reverse the ordering if they need to.)
> >
> > I think that doesn't quite work, because the failure is caused by LB
> > <= UB, not LB < UB.  We could fix that by writing "precedes or equals"
> > but that seems lame.  Maybe:
> >
> > Lower bound %s does not precede upper bound %s.
> 
> There was an earlier suggestion to use "greater than or equal to". I
> think that would work quite well:
> 
> ERROR:  invalid range bounds for partition \"%s\"
> DETAIL:  lower bound %s is greater than or equal to upper bound %s.

Is it possible to detect the equality case specifically and use a
different errdetail?  Something like "the lower bound %s is equal to the
upper bound" (obviously without including both in the message.)

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  9. Re: dubious error message from partition.c

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-08-09T20:59:21Z

    Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> Dean Rasheed wrote:
>> There was an earlier suggestion to use "greater than or equal to". I
>> think that would work quite well:

> Is it possible to detect the equality case specifically and use a
> different errdetail?  Something like "the lower bound %s is equal to the
> upper bound" (obviously without including both in the message.)

That seems like overkill.  I'm good with "greater than or equal to".

			regards, tom lane

  10. Re: dubious error message from partition.c

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2017-08-10T02:14:00Z

    On 2017/08/10 5:59, Tom Lane wrote:
> Alvaro Herrera <alvherre@2ndquadrant.com> writes:
>> Dean Rasheed wrote:
>>> There was an earlier suggestion to use "greater than or equal to". I
>>> think that would work quite well:
> 
>> Is it possible to detect the equality case specifically and use a
>> different errdetail?  Something like "the lower bound %s is equal to the
>> upper bound" (obviously without including both in the message.)
> 
> That seems like overkill.  I'm good with "greater than or equal to".

Attached updated patch has "greater than or equal to".

Thanks,
Amit

  11. Re: dubious error message from partition.c

    Robert Haas <robertmhaas@gmail.com> — 2017-08-10T17:54:20Z

    On Wed, Aug 9, 2017 at 10:14 PM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:
>> That seems like overkill.  I'm good with "greater than or equal to".
>
> Attached updated patch has "greater than or equal to".

Committed, with one small change which I hope will be considered an
improvement.  Your proposed primary error message was:

invalid range bound specification for partition \"%s\"

I changed it to:

empty range bound specified for partition \"%s\"

Thanks for working on this.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  12. Re: dubious error message from partition.c

    Amit Langote <amitlangote09@gmail.com> — 2017-08-11T03:22:07Z

    (Sorry Robert for the duplicate message, I mistakenly didn't hit Reply All)

On Fri, Aug 11, 2017 at 2:54 Robert Haas <robertmhaas@gmail.com> wrote:

> On Wed, Aug 9, 2017 at 10:14 PM, Amit Langote
> <Langote_Amit_f8@lab.ntt.co.jp> wrote:
> >> That seems like overkill.  I'm good with "greater than or equal to".
> >
> > Attached updated patch has "greater than or equal to".
>
> Committed, with one small change which I hope will be considered an
> improvement.  Your proposed primary error message was:
>
> invalid range bound specification for partition \"%s\"
>
> I changed it to:
>
> empty range bound specified for partition \"%s\"
>
> Thanks for working on this.


Thank for committing.  The new message is certainly an improvement.

Thanks,
Amit