Re: [HACKERS] dubious error message from partition.c

[Amit Langote]

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

On Fri, Aug 11, 2017 at 2:54 Robert Haas  wrote:

> On Wed, Aug 9, 2017 at 10:14 PM, Amit Langote
>  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

Re: [HACKERS] dubious error message from partition.c

[Robert Haas]

On Wed, Aug 9, 2017 at 10:14 PM, Amit Langote
 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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] dubious error message from partition.c

[Amit Langote]

On 2017/08/10 5:59, Tom Lane wrote:
> Alvaro Herrera  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
From 11d40a76dc9e56a85fe480fd5690a53406cea9c3 Mon Sep 17 00:00:00 2001
From: amit 
Date: Wed, 9 Aug 2017 14:36:57 +0900
Subject: [PATCH] Fix error message when apprently empty range bound is
 specified

---
 src/backend/catalog/partition.c| 10 +++-
 src/backend/utils/adt/ruleutils.c  | 84 +++---
 src/include/utils/ruleutils.h  |  1 +
 src/test/regress/expected/create_table.out |  6 ++-
 4 files changed, 56 insertions(+), 45 deletions(-)

diff --git a/src/backend/catalog/partition.c b/src/backend/catalog/partition.c
index dcc7f8af27..65630421d3 100644
--- a/src/backend/catalog/partition.c
+++ b/src/backend/catalog/partition.c
@@ -722,10 +722,16 @@ check_new_partition_bound(char *relname, Relation parent,
 */
if (partition_rbound_cmp(key, lower->datums, 
lower->kind, true,

 upper) >= 0)
+   {
ereport(ERROR,

(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
-errmsg("cannot create 
range partition with empty range"),
-
parser_errposition(pstate, spec->location)));
+errmsg("invalid range 
bound specification for partition \"%s\"",
+   
relname),
+errdetail("Specified 
lower bound %s is greater than or equal to upper bound %s.",
+   
get_range_partbound_string(spec->lowerdatums),
+   
get_range_partbound_string(spec->upperdatums)),
+   
parser_errposition(pstate, spec->location)));
+   }
 
if (partdesc->nparts > 0)
{
diff --git a/src/backend/utils/adt/ruleutils.c 
b/src/backend/utils/adt/ruleutils.c
index d83377d1d8..0faa0204ce 100644
--- a/src/backend/utils/adt/ruleutils.c
+++ b/src/backend/utils/adt/ruleutils.c
@@ -8722,47 +8722,9 @@ get_rule_expr(Node *node, deparse_context *context,
   
list_length(spec->lowerdatums) ==
   
list_length(spec->upperdatums));
 
-   appendStringInfoString(buf, 
"FOR VALUES FROM (");
-   sep = "";
-   foreach(cell, spec->lowerdatums)
-   {
-   PartitionRangeDatum 
*datum =
-   
castNode(PartitionRangeDatum, lfirst(cell));
-
-   
appendStringInfoString(buf, sep);
-   if (datum->kind == 
PARTITION_RANGE_DATUM_MINVALUE)
-   
appendStringInfoString(buf, "MINVALUE");
-   else if (datum->kind == 
PARTITION_RANGE_DATUM_MAXVALUE)
-   
appendStringInfoString(buf, "MAXVALUE");
-   else
-   {
-   Const  *val 
= castNode(Const, datum->value);
-
-   
get_const_expr(val, context, -1);
-   }
-   sep = ", ";
-   }
-   appendStringInfoString(buf, ") 
TO (");
-   sep = "";
-   

Re: [HACKERS] dubious error message from partition.c

[Tom Lane]

Alvaro Herrera  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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] dubious error message from partition.c

[Alvaro Herrera]

Dean Rasheed wrote:
> On 9 August 2017 at 13:03, Robert Haas  wrote:
> > On Tue, Aug 8, 2017 at 11:34 PM, Tom Lane  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 Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] dubious error message from partition.c

[Dean Rasheed]

On 9 August 2017 at 13:03, Robert Haas  wrote:
> On Tue, Aug 8, 2017 at 11:34 PM, Tom Lane  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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] dubious error message from partition.c

[Robert Haas]

On Tue, Aug 8, 2017 at 11:34 PM, Tom Lane  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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] dubious error message from partition.c

[Amit Langote]

On 2017/08/09 13:03, David G. Johnston wrote:
> On Tue, Aug 8, 2017 at 8:34 PM, Tom Lane  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
From 6f2266376c765207e2cc458bb890c671e0bc364a Mon Sep 17 00:00:00 2001
From: amit 
Date: Wed, 9 Aug 2017 14:36:57 +0900
Subject: [PATCH] Fix error message when apprently empty range bound is
 specified

---
 src/backend/catalog/partition.c| 10 +++-
 src/backend/utils/adt/ruleutils.c  | 84 +++---
 src/include/utils/ruleutils.h  |  1 +
 src/test/regress/expected/create_table.out |  6 ++-
 4 files changed, 56 insertions(+), 45 deletions(-)

diff --git a/src/backend/catalog/partition.c b/src/backend/catalog/partition.c
index dcc7f8af27..01ba4a6172 100644
--- a/src/backend/catalog/partition.c
+++ b/src/backend/catalog/partition.c
@@ -722,10 +722,16 @@ check_new_partition_bound(char *relname, Relation parent,
 */
if (partition_rbound_cmp(key, lower->datums, 
lower->kind, true,

 upper) >= 0)
+   {
ereport(ERROR,

(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
-errmsg("cannot create 
range partition with empty range"),
-
parser_errposition(pstate, spec->location)));
+errmsg("invalid range 
bound specification for partition \"%s\"",
+   
relname),
+errdetail("Specified 
upper bound %s precedes lower bound %s.",
+   
get_range_partbound_string(spec->upperdatums),
+   
get_range_partbound_string(spec->lowerdatums)),
+   
parser_errposition(pstate, spec->location)));
+   }
 
if (partdesc->nparts > 0)
{
diff --git a/src/backend/utils/adt/ruleutils.c 
b/src/backend/utils/adt/ruleutils.c
index d83377d1d8..0faa0204ce 100644
--- a/src/backend/utils/adt/ruleutils.c
+++ b/src/backend/utils/adt/ruleutils.c
@@ -8722,47 +8722,9 @@ get_rule_expr(Node *node, deparse_context *context,
   
list_length(spec->lowerdatums) ==
   
list_length(spec->upperdatums));
 
-   appendStringInfoString(buf, 
"FOR VALUES FROM (");
-   sep = "";
-   foreach(cell, spec->lowerdatums)
-   {
-   PartitionRangeDatum 
*datum =
-  

Re: [HACKERS] dubious error message from partition.c

[David G. Johnston]

On Tue, Aug 8, 2017 at 8:34 PM, Tom Lane  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.

Re: [HACKERS] dubious error message from partition.c

[Tom Lane]

Amit Langote  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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] dubious error message from partition.c

[Amit Langote]

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 (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

[HACKERS] dubious error message from partition.c

[Robert Haas]

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers