On 2017/08/09 13:03, David G. Johnston wrote:
> On Tue, Aug 8, 2017 at 8:34 PM, Tom Lane <t...@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
From 6f2266376c765207e2cc458bb890c671e0bc364a Mon Sep 17 00:00:00 2001
From: amit <amitlangot...@gmail.com>
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 =
-                                                       
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 = "";
-                                               foreach(cell, spec->upperdatums)
-                                               {
-                                                       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, 
")");
+                                               appendStringInfo(buf, "FOR 
VALUES FROM %s TO %s",
+                                                       
get_range_partbound_string(spec->lowerdatums),
+                                                       
get_range_partbound_string(spec->upperdatums));
                                                break;
 
                                        default:
@@ -10943,3 +10905,43 @@ flatten_reloptions(Oid relid)
 
        return result;
 }
+
+/*
+ * get_one_range_partition_bound_string
+ *             A C string representation of one range partition bound
+ */
+char *
+get_range_partbound_string(List *bound_datums)
+{
+       deparse_context context;
+       StringInfo      buf = makeStringInfo();
+       ListCell   *cell;
+       char       *sep;
+
+       memset(&context, 0, sizeof(deparse_context));
+       context.buf = buf;
+
+       appendStringInfoString(buf, "(");
+       sep = "";
+       foreach(cell, bound_datums)
+       {
+               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, ")");
+
+       return buf->data;
+}
diff --git a/src/include/utils/ruleutils.h b/src/include/utils/ruleutils.h
index a2206cb7cd..e9c5193855 100644
--- a/src/include/utils/ruleutils.h
+++ b/src/include/utils/ruleutils.h
@@ -33,5 +33,6 @@ extern List *set_deparse_context_planstate(List *dpcontext,
 extern List *select_rtable_names_for_explain(List *rtable,
                                                                Bitmapset 
*rels_used);
 extern char *generate_collation_name(Oid collid);
+extern char *get_range_partbound_string(List *bound_datums);
 
 #endif                                                 /* RULEUTILS_H */
diff --git a/src/test/regress/expected/create_table.out 
b/src/test/regress/expected/create_table.out
index aa44c11273..fa509859f8 100644
--- a/src/test/regress/expected/create_table.out
+++ b/src/test/regress/expected/create_table.out
@@ -567,10 +567,12 @@ CREATE TABLE range_parted2 (
 ) PARTITION BY RANGE (a);
 -- trying to create range partition with empty range
 CREATE TABLE fail_part PARTITION OF range_parted2 FOR VALUES FROM (1) TO (0);
-ERROR:  cannot create range partition with empty range
+ERROR:  invalid range bound specification for partition "fail_part"
+DETAIL:  Specified upper bound (0) precedes lower bound (1).
 -- note that the range '[1, 1)' has no elements
 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).
 CREATE TABLE part0 PARTITION OF range_parted2 FOR VALUES FROM (minvalue) TO 
(1);
 CREATE TABLE fail_part PARTITION OF range_parted2 FOR VALUES FROM (minvalue) 
TO (2);
 ERROR:  partition "fail_part" would overlap partition "part0"
-- 
2.11.0

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

Reply via email to