Re: Report error position in partition bound check

[Amit Langote]

On Tue, Sep 29, 2020 at 2:01 AM Tom Lane  wrote:
> Amit Langote  writes:
> > On Fri, Sep 25, 2020 at 12:02 AM Tom Lane  wrote:
> >> Well, I agree with Peter to that extent, but my opinion is that *none*
> >> of these cases ought to be errors.  What we're doing here is performing
> >> an implicit assignment-level coercion of the expression to the type of
> >> the column, and changing the collation is allowed as part of that:
> >>
> >> regression=# create table foo (f1 text collate "C");
> >> CREATE TABLE
> >> regression=# insert into foo values ('a' COLLATE "POSIX");
> >> INSERT 0 1
> >> regression=# update foo set f1 = 'b' COLLATE "POSIX";
> >> UPDATE 1
> >>
> >> So I find it completely inconsistent that the partitioning logic
> >> complains about equivalent cases.
>
> > My perhaps wrong impression was that the bound expression that is
> > specified when creating a partition is not as such being *assigned* to
> > the key column, but now that I think about it some more, that doesn't
> > matter.
>
> >> I think we should just rip the
> >> whole thing out, as per the attached draft.  This causes several
> >> regression test results to change, but AFAICS those are only there
> >> to exercise the error tests that I think we should get rid of.
>
> > Yeah, I can see no other misbehavior resulting from this.
>
> OK, I'll clean up the regression test cases and push that.

Thanks.

> (Although this could be claimed to be a bug, I do not feel
> a need to back-patch the behavioral change.)

Agreed.  The assign_expr_collations() omission was indeed a bug.

--
Amit Langote
EnterpriseDB: http://www.enterprisedb.com

Re: Report error position in partition bound check

[Tom Lane]

Amit Langote  writes:
> On Fri, Sep 25, 2020 at 12:02 AM Tom Lane  wrote:
>> Well, I agree with Peter to that extent, but my opinion is that *none*
>> of these cases ought to be errors.  What we're doing here is performing
>> an implicit assignment-level coercion of the expression to the type of
>> the column, and changing the collation is allowed as part of that:
>> 
>> regression=# create table foo (f1 text collate "C");
>> CREATE TABLE
>> regression=# insert into foo values ('a' COLLATE "POSIX");
>> INSERT 0 1
>> regression=# update foo set f1 = 'b' COLLATE "POSIX";
>> UPDATE 1
>> 
>> So I find it completely inconsistent that the partitioning logic
>> complains about equivalent cases.

> My perhaps wrong impression was that the bound expression that is
> specified when creating a partition is not as such being *assigned* to
> the key column, but now that I think about it some more, that doesn't
> matter.

>> I think we should just rip the
>> whole thing out, as per the attached draft.  This causes several
>> regression test results to change, but AFAICS those are only there
>> to exercise the error tests that I think we should get rid of.

> Yeah, I can see no other misbehavior resulting from this.

OK, I'll clean up the regression test cases and push that.

(Although this could be claimed to be a bug, I do not feel
a need to back-patch the behavioral change.)

regards, tom lane

Re: Report error position in partition bound check

[Amit Langote]

On Fri, Sep 25, 2020 at 12:02 AM Tom Lane  wrote:
> [ cc'ing Peter, since his opinion seems to have got us here in the first 
> place ]
>
> Amit Langote  writes:
> > On Thu, Sep 24, 2020 at 7:19 AM Tom Lane  wrote:
> >> However, while I was looking at it I couldn't help noticing that
> >> transformPartitionBoundValue's handling of collation concerns seems
> >> less than sane.  There are two things bugging me:
> >>
> >> 1. Why does it care about the expression's collation only when there's
> >> a top-level CollateExpr?  For example, that means we get an error for
> >>
> >> regression=# create table p (f1 text collate "C") partition by list(f1);
> >> CREATE TABLE
> >> regression=# create table c1 partition of p for values in ('a' collate 
> >> "POSIX");
> >> ERROR:  collation of partition bound value for column "f1" does not match 
> >> partition key collation "C"
> >>
> >> but not this:
> >>
> >> regression=# create table c2 partition of p for values in ('a' || 'b' 
> >> collate "POSIX");
> >> CREATE TABLE
> >>
> >> Given that we will override the expression's collation with the partition
> >> column's collation anyway, I don't see why we have this check at all,
> >> so my preference is to just rip out the entire stanza beginning with
> >> "if (IsA(value, CollateExpr))".  If we keep it, though, I think it needs
> >> to do something else that is more general.
>
> > I dug up the discussion which resulted in this test being added and
> > found that Peter E had opined that this failure should not occur [1].
>
> Well, I agree with Peter to that extent, but my opinion is that *none*
> of these cases ought to be errors.  What we're doing here is performing
> an implicit assignment-level coercion of the expression to the type of
> the column, and changing the collation is allowed as part of that:
>
> regression=# create table foo (f1 text collate "C");
> CREATE TABLE
> regression=# insert into foo values ('a' COLLATE "POSIX");
> INSERT 0 1
> regression=# update foo set f1 = 'b' COLLATE "POSIX";
> UPDATE 1
>
> So I find it completely inconsistent that the partitioning logic
> complains about equivalent cases.

My perhaps wrong impression was that the bound expression that is
specified when creating a partition is not as such being *assigned* to
the key column, but now that I think about it some more, that doesn't
matter.

>  I think we should just rip the
> whole thing out, as per the attached draft.  This causes several
> regression test results to change, but AFAICS those are only there
> to exercise the error tests that I think we should get rid of.

Yeah, I can see no other misbehavior resulting from this.

--
Amit Langote
EnterpriseDB: http://www.enterprisedb.com

Re: Report error position in partition bound check

[Tom Lane]

[ cc'ing Peter, since his opinion seems to have got us here in the first place ]

Amit Langote  writes:
> On Thu, Sep 24, 2020 at 7:19 AM Tom Lane  wrote:
>> However, while I was looking at it I couldn't help noticing that
>> transformPartitionBoundValue's handling of collation concerns seems
>> less than sane.  There are two things bugging me:
>> 
>> 1. Why does it care about the expression's collation only when there's
>> a top-level CollateExpr?  For example, that means we get an error for
>> 
>> regression=# create table p (f1 text collate "C") partition by list(f1);
>> CREATE TABLE
>> regression=# create table c1 partition of p for values in ('a' collate 
>> "POSIX");
>> ERROR:  collation of partition bound value for column "f1" does not match 
>> partition key collation "C"
>> 
>> but not this:
>> 
>> regression=# create table c2 partition of p for values in ('a' || 'b' 
>> collate "POSIX");
>> CREATE TABLE
>> 
>> Given that we will override the expression's collation with the partition
>> column's collation anyway, I don't see why we have this check at all,
>> so my preference is to just rip out the entire stanza beginning with
>> "if (IsA(value, CollateExpr))".  If we keep it, though, I think it needs
>> to do something else that is more general.

> I dug up the discussion which resulted in this test being added and
> found that Peter E had opined that this failure should not occur [1].

Well, I agree with Peter to that extent, but my opinion is that *none*
of these cases ought to be errors.  What we're doing here is performing
an implicit assignment-level coercion of the expression to the type of
the column, and changing the collation is allowed as part of that:

regression=# create table foo (f1 text collate "C");
CREATE TABLE
regression=# insert into foo values ('a' COLLATE "POSIX");
INSERT 0 1
regression=# update foo set f1 = 'b' COLLATE "POSIX";
UPDATE 1

So I find it completely inconsistent that the partitioning logic
complains about equivalent cases.  I think we should just rip the
whole thing out, as per the attached draft.  This causes several
regression test results to change, but AFAICS those are only there
to exercise the error tests that I think we should get rid of.

regards, tom lane

diff --git a/src/backend/parser/parse_utilcmd.c b/src/backend/parser/parse_utilcmd.c
index 164312d60e..0dc03dd984 100644
--- a/src/backend/parser/parse_utilcmd.c
+++ b/src/backend/parser/parse_utilcmd.c
@@ -4183,50 +4183,6 @@ transformPartitionBoundValue(ParseState *pstate, Node *val,
 	 */
 	Assert(!contain_var_clause(value));
 
-	/*
-	 * Check that the input expression's collation is compatible with one
-	 * specified for the parent's partition key (partcollation).  Don't throw
-	 * an error if it's the default collation which we'll replace with the
-	 * parent's collation anyway.
-	 */
-	if (IsA(value, CollateExpr))
-	{
-		Oid			exprCollOid = exprCollation(value);
-
-		/*
-		 * Check we have a collation iff it is a collatable type.  The only
-		 * expected failures here are (1) COLLATE applied to a noncollatable
-		 * type, or (2) partition bound expression had an unresolved
-		 * collation.  But we might as well code this to be a complete
-		 * consistency check.
-		 */
-		if (type_is_collatable(colType))
-		{
-			if (!OidIsValid(exprCollOid))
-ereport(ERROR,
-		(errcode(ERRCODE_INDETERMINATE_COLLATION),
-		 errmsg("could not determine which collation to use for partition bound expression"),
-		 errhint("Use the COLLATE clause to set the collation explicitly.")));
-		}
-		else
-		{
-			if (OidIsValid(exprCollOid))
-ereport(ERROR,
-		(errcode(ERRCODE_DATATYPE_MISMATCH),
-		 errmsg("collations are not supported by type %s",
-format_type_be(colType;
-		}
-
-		if (OidIsValid(exprCollOid) &&
-			exprCollOid != DEFAULT_COLLATION_OID &&
-			exprCollOid != partCollation)
-			ereport(ERROR,
-	(errcode(ERRCODE_DATATYPE_MISMATCH),
-	 errmsg("collation of partition bound value for column \"%s\" does not match partition key collation \"%s\"",
-			colName, get_collation_name(partCollation)),
-	 parser_errposition(pstate, exprLocation(value;
-	}
-
 	/*
 	 * Coerce to the correct type.  This might cause an explicit coercion step
 	 * to be added on top of the expression, which must be evaluated before
@@ -4253,6 +4209,7 @@ transformPartitionBoundValue(ParseState *pstate, Node *val,
 	 */
 	if (!IsA(value, Const))
 	{
+		assign_expr_collations(pstate, value);
 		value = (Node *) expression_planner((Expr *) value);
 		value = (Node *) evaluate_expr((Expr *) value, colType, colTypmod,
 	   partCollation);

Re: Report error position in partition bound check

[Amit Langote]

On Thu, Sep 24, 2020 at 7:19 AM Tom Lane  wrote:
> I looked this over and pushed it with some minor adjustments.

Thank you.

> However, while I was looking at it I couldn't help noticing that
> transformPartitionBoundValue's handling of collation concerns seems
> less than sane.  There are two things bugging me:
>
> 1. Why does it care about the expression's collation only when there's
> a top-level CollateExpr?  For example, that means we get an error for
>
> regression=# create table p (f1 text collate "C") partition by list(f1);
> CREATE TABLE
> regression=# create table c1 partition of p for values in ('a' collate 
> "POSIX");
> ERROR:  collation of partition bound value for column "f1" does not match 
> partition key collation "C"
>
> but not this:
>
> regression=# create table c2 partition of p for values in ('a' || 'b' collate 
> "POSIX");
> CREATE TABLE
>
> Given that we will override the expression's collation with the partition
> column's collation anyway, I don't see why we have this check at all,
> so my preference is to just rip out the entire stanza beginning with
> "if (IsA(value, CollateExpr))".  If we keep it, though, I think it needs
> to do something else that is more general.
>
> 2. Nothing is doing assign_expr_collations() on the partition expression.
> This can trivially be shown to cause problems:
>
> regression=# create table p (f1 bool) partition by list(f1);
> CREATE TABLE
> regression=# create table cf partition of p for values in ('a' < 'b');
> ERROR:  could not determine which collation to use for string comparison
> HINT:  Use the COLLATE clause to set the collation explicitly.
>
>
> If we want to rip out the collation mismatch error altogether, then
> fixing #2 would just require inserting assign_expr_collations() before
> the expression_planner() call.  The other direction that would make
> sense to me is to perform assign_expr_collations() after
> coerce_to_target_type(), and then to complain if exprCollation()
> isn't default and doesn't match the partition collation.  In any
> case a specific test for a CollateExpr seems quite wrong.

I tried implementing that as attached and one test failed:

create table test_part_coll_posix (a text) partition by range (a
collate "POSIX");
...
create table test_part_coll_cast2 partition of test_part_coll_posix
for values from (name 's') to ('z');
+ERROR:  collation of partition bound value for column "a" does not
match partition key collation "POSIX"
+LINE 1: ...ion of test_part_coll_posix for values from (name 's') to ('...

I dug up the discussion which resulted in this test being added and
found that Peter E had opined that this failure should not occur [1].
Maybe that is why I put that half-baked guard consisting of checking
if the erroneous collation comes from an explicit COLLATE clause.  Now
I think maybe giving an error is alright but we should tell in the
DETAIL message what the expression's collation is, like as follows:

create table test_part_coll_cast2 partition of test_part_coll_posix
for values from (name 's') to ('z');
+ERROR:  collation of partition bound value for column "a" does not
match partition key collation "POSIX"
+LINE 1: ...ion of test_part_coll_posix for values from (name 's') to ('...
+ ^
+DETAIL:  The collation of partition bound value is "C".

--
Amit Langote
EnterpriseDB: http://www.enterprisedb.com

[1] 
https://www.postgresql.org/message-id/04661508-b6f5-177e-6f6b-c4cb8426b9f0%402ndquadrant.com


partition-bound-collation-check.patch
Description: Binary data

Re: Report error position in partition bound check

[Tom Lane]

I looked this over and pushed it with some minor adjustments.

However, while I was looking at it I couldn't help noticing that
transformPartitionBoundValue's handling of collation concerns seems
less than sane.  There are two things bugging me:

1. Why does it care about the expression's collation only when there's
a top-level CollateExpr?  For example, that means we get an error for

regression=# create table p (f1 text collate "C") partition by list(f1);
CREATE TABLE
regression=# create table c1 partition of p for values in ('a' collate "POSIX");
ERROR:  collation of partition bound value for column "f1" does not match 
partition key collation "C"

but not this:

regression=# create table c2 partition of p for values in ('a' || 'b' collate 
"POSIX");
CREATE TABLE

Given that we will override the expression's collation with the partition
column's collation anyway, I don't see why we have this check at all,
so my preference is to just rip out the entire stanza beginning with
"if (IsA(value, CollateExpr))".  If we keep it, though, I think it needs
to do something else that is more general.

2. Nothing is doing assign_expr_collations() on the partition expression.
This can trivially be shown to cause problems:

regression=# create table p (f1 bool) partition by list(f1);
CREATE TABLE
regression=# create table cf partition of p for values in ('a' < 'b');
ERROR:  could not determine which collation to use for string comparison
HINT:  Use the COLLATE clause to set the collation explicitly.


If we want to rip out the collation mismatch error altogether, then
fixing #2 would just require inserting assign_expr_collations() before
the expression_planner() call.  The other direction that would make
sense to me is to perform assign_expr_collations() after
coerce_to_target_type(), and then to complain if exprCollation()
isn't default and doesn't match the partition collation.  In any
case a specific test for a CollateExpr seems quite wrong.

regards, tom lane

Re: Report error position in partition bound check

[Amit Langote]

On Wed, Sep 23, 2020 at 10:22 PM Ashutosh Bapat
 wrote:
> On Wed, 23 Sep 2020 at 14:41, Amit Langote  wrote:
> Setting this CF entry as "RFC". Thanks.

Great, thanks for your time on this.

-- 
Amit Langote
EnterpriseDB: http://www.enterprisedb.com

Re: Report error position in partition bound check

[Ashutosh Bapat]

On Wed, 23 Sep 2020 at 14:41, Amit Langote  wrote:

> Thanks Ashutosh.
>
> On Fri, Sep 18, 2020 at 7:33 PM Ashutosh Bapat
>  wrote:
> > Thanks Amit for addressing comments.
> >
> > @@ -4256,5 +4256,8 @@ transformPartitionBoundValue(ParseState *pstate,
> Node *val,
> >   if (!IsA(value, Const))
> >   elog(ERROR, "could not evaluate partition bound expression");
> >
> > + /* Preserve parser location information. */
> > + ((Const *) value)->location = exprLocation(val);
> > +
> >   return (Const *) value;
> >  }
> >
> > This caught my attention and I was wondering whether transformExpr()
> itself should transfer the location from input expression to the output
> expression. Some minions of transformExprRecurse() seem to be doing that.
> The change here may be an indication that some of them are not doing this.
> In that case may be it's better to find those and fix rather than a
> white-wash fix here. In what case did we find that location was not set by
> transformExpr? Sorry for not catching this earlier.
>
> AFAICS, transformExpr() is fine.  What loses the location value is the
> unconditional evaluate_expr() call which generates a fresh Const node,
> possibly after evaluating a non-Const expression that is passed to it.
> I don't find it very desirable to change evaluate_expr() to accept a
> location value, because other callers of it don't seem to care.
> Instead, in the updated patch, I have made calling evaluate_expr()
> conditional on the expression at hand being a non-Const node and
> assign location by hand on return.  If the expression is already
> Const, we don't need to update the location field as it should already
> be correct.  Though, I did notice that the evaluate_expr() call has an
> additional responsibility which is to pass the partition key specified
> collation to the bound expression, so we should not fail to update an
> already-Const node's collation likewise.
>

Thanks for the detailed explanation. I am not sure whether skipping one
evaluate_expr() call for a constant is better or reassigning the location.
This looks better than the last patch.


> > /* New lower bound is certainly >= bound at offet. */
> > offet/offset? But this comment is implied by the comment just two lines
> above. So I am not sure it's really needed.
>
> Given that cmpval is set all the way in partition_range_bsearch(), I
> thought it would help to clarify why this code can assume it must be
> >= 0.  It is because a valid offset returned by
> partition_range_bsearch() must correspond to a bound that it found to
> be <= the probe bound passed to it.
>

> > /* Fetch the problem bound from lower datums list. */
> > This is fetching problematic key value rather than the whole problematic
> bound. I think the comment would be useful if it explains why cmpval -1 th
> key is problematic but then that's evident from the prologue of
> partition_rbound_cmp() so I am not sure if this comment is really required.
> For example, we aren't adding a comment here
> > + overlap_location = ((PartitionRangeDatum *)
> > + list_nth(spec->upperdatums, -cmpval - 1))->location;
>
> In the attached updated patch, I have tried to make the code and
> comments for different cases consistent.  Please have a look.
>
>

The comments look okay to me. I don't see a way to keep them short and yet
avoid reading the prologue of partition_range_bsearch(). And there is no
point in repeating a portion of that prologue at multiple places. So I am
fine with these set of comments.

Setting this CF entry as "RFC". Thanks.

-- 
Best Wishes,
Ashutosh

Re: Report error position in partition bound check

[Amit Langote]

Thanks Ashutosh.

On Fri, Sep 18, 2020 at 7:33 PM Ashutosh Bapat
 wrote:
> Thanks Amit for addressing comments.
>
> @@ -4256,5 +4256,8 @@ transformPartitionBoundValue(ParseState *pstate, Node 
> *val,
>   if (!IsA(value, Const))
>   elog(ERROR, "could not evaluate partition bound expression");
>
> + /* Preserve parser location information. */
> + ((Const *) value)->location = exprLocation(val);
> +
>   return (Const *) value;
>  }
>
> This caught my attention and I was wondering whether transformExpr() itself 
> should transfer the location from input expression to the output expression. 
> Some minions of transformExprRecurse() seem to be doing that. The change here 
> may be an indication that some of them are not doing this. In that case may 
> be it's better to find those and fix rather than a white-wash fix here. In 
> what case did we find that location was not set by transformExpr? Sorry for 
> not catching this earlier.

AFAICS, transformExpr() is fine.  What loses the location value is the
unconditional evaluate_expr() call which generates a fresh Const node,
possibly after evaluating a non-Const expression that is passed to it.
I don't find it very desirable to change evaluate_expr() to accept a
location value, because other callers of it don't seem to care.
Instead, in the updated patch, I have made calling evaluate_expr()
conditional on the expression at hand being a non-Const node and
assign location by hand on return.  If the expression is already
Const, we don't need to update the location field as it should already
be correct.  Though, I did notice that the evaluate_expr() call has an
additional responsibility which is to pass the partition key specified
collation to the bound expression, so we should not fail to update an
already-Const node's collation likewise.

> /* New lower bound is certainly >= bound at offet. */
> offet/offset? But this comment is implied by the comment just two lines 
> above. So I am not sure it's really needed.

Given that cmpval is set all the way in partition_range_bsearch(), I
thought it would help to clarify why this code can assume it must be
>= 0.  It is because a valid offset returned by
partition_range_bsearch() must correspond to a bound that it found to
be <= the probe bound passed to it.

> /* Fetch the problem bound from lower datums list. */
> This is fetching problematic key value rather than the whole problematic 
> bound. I think the comment would be useful if it explains why cmpval -1 th 
> key is problematic but then that's evident from the prologue of 
> partition_rbound_cmp() so I am not sure if this comment is really required. 
> For example, we aren't adding a comment here
> + overlap_location = ((PartitionRangeDatum *)
> + list_nth(spec->upperdatums, -cmpval - 1))->location;

In the attached updated patch, I have tried to make the code and
comments for different cases consistent.  Please have a look.


--
Amit Langote
EnterpriseDB: http://www.enterprisedb.com


v5-0001-Improve-check-new-partition-bound-error-position-.patch
Description: Binary data

Re: Report error position in partition bound check

[Ashutosh Bapat]

On Thu, 17 Sep 2020 at 13:06, Amit Langote  wrote:

> Hi Ashutosh,
>
> I had forgotten about this thread, but Michael's ping email brought it
> to my attention.
>
> Thanks Amit for addressing comments.

@@ -4256,5 +4256,8 @@ transformPartitionBoundValue(ParseState *pstate, Node
*val,
  if (!IsA(value, Const))
  elog(ERROR, "could not evaluate partition bound expression");

+ /* Preserve parser location information. */
+ ((Const *) value)->location = exprLocation(val);
+
  return (Const *) value;
 }

This caught my attention and I was wondering whether transformExpr() itself
should transfer the location from input expression to the output
expression. Some minions of transformExprRecurse() seem to be doing that.
The change here may be an indication that some of them are not doing this.
In that case may be it's better to find those and fix rather than a
white-wash fix here. In what case did we find that location was not set by
transformExpr? Sorry for not catching this earlier.

/* New lower bound is certainly >= bound at offet. */
offet/offset? But this comment is implied by the comment just two lines
above. So I am not sure it's really needed.

/* Fetch the problem bound from lower datums list. */
This is fetching problematic key value rather than the whole problematic
bound. I think the comment would be useful if it explains why cmpval -1 th
key is problematic but then that's evident from the prologue
of partition_rbound_cmp() so I am not sure if this comment is really
required. For example, we aren't adding a comment here
+ overlap_location = ((PartitionRangeDatum *)
+ list_nth(spec->upperdatums, -cmpval - 1))->location;

-- 
Best Wishes,
Ashutosh

Re: Report error position in partition bound check

[Amit Langote]

Hi Ashutosh,

I had forgotten about this thread, but Michael's ping email brought it
to my attention.

On Fri, Sep 4, 2020 at 11:12 PM Ashutosh Bapat
 wrote:
> Thanks for rebasing patch. It applies cleanly still. Here are some comments

Thanks for the review.

> @@ -3320,7 +3338,9 @@ make_one_partition_rbound(PartitionKey key, int index, 
> List *datums, bool lower)
>   * partition_rbound_cmp
>   *
>   * Return for two range bounds whether the 1st one (specified in datums1,
>
> I think it's better to reword it as. "For two range bounds decide whether ...
>
> - * kind1, and lower1) is <, =, or > the bound specified in *b2.
> + * kind1, and lower1) is <, =, or > the bound specified in *b2. 0 is 
> returned if
> + * equal and the 1-based index of the first mismatching bound if unequal;
> + * multiplied by -1 if the 1st bound is smaller.
>
> This sentence makes sense after the above correction. I liked this change,
> requires very small changes in other parts.

+1 to your suggested rewording, although I wrote: "For two range
bounds this decides whether..."

>  /*
> @@ -3495,7 +3518,7 @@ static int
>  partition_range_bsearch(int partnatts, FmgrInfo *partsupfunc,
> Oid *partcollation,
> PartitionBoundInfo boundinfo,
> -   PartitionRangeBound *probe, bool *is_equal)
> +   PartitionRangeBound *probe, bool *is_equal, int32 
> *cmpval)
>
> Please update the prologue explaining the new argument.

Done.  Actually, I noticed that *is_equal was unused in this
function's only caller.  *cmpval == 0 already gives that, so removed
is_equal parameter.

Attached updated version.

-- 
Amit Langote
EnterpriseDB: http://www.enterprisedb.com


v4-0001-Improve-check-new-partition-bound-error-position-.patch
Description: Binary data

Re: Report error position in partition bound check

[Michael Paquier]

On Fri, Sep 04, 2020 at 07:42:27PM +0530, Ashutosh Bapat wrote:
> After this change, the patch will be ready for a committer.

Alexandra, this patch is waiting on author after this review.  Could
you answer to the points raised by Ashutosh and update this patch
accordingly?
--
Michael


signature.asc
Description: PGP signature

Re: Report error position in partition bound check

[Ashutosh Bapat]

On Fri, 10 Jul 2020 at 23:31, Alexandra Wang 
wrote:

>
>
> Thank you Daniel. Here's the rebased patch. I also squashed the two
> patches into one so it's easier to review.
>
> Thanks for rebasing patch. It applies cleanly still. Here are some comments
@@ -3320,7 +3338,9 @@ make_one_partition_rbound(PartitionKey key, int
index, List *datums, bool lower)
  * partition_rbound_cmp
  *
  * Return for two range bounds whether the 1st one (specified in datums1,

I think it's better to reword it as. "For two range bounds decide whether
...

- * kind1, and lower1) is <, =, or > the bound specified in *b2.
+ * kind1, and lower1) is <, =, or > the bound specified in *b2. 0 is
returned if
+ * equal and the 1-based index of the first mismatching bound if unequal;
+ * multiplied by -1 if the 1st bound is smaller.

This sentence makes sense after the above correction. I liked this change,
requires very small changes in other parts.


 /*
@@ -3495,7 +3518,7 @@ static int
 partition_range_bsearch(int partnatts, FmgrInfo *partsupfunc,
Oid *partcollation,
PartitionBoundInfo boundinfo,
-   PartitionRangeBound *probe, bool *is_equal)
+   PartitionRangeBound *probe, bool *is_equal, int32
*cmpval)

Please update the prologue explaining the new argument.

After this change, the patch will be ready for a committer.
-- 
Best Wishes,
Ashutosh

Re: Report error position in partition bound check

[Alexandra Wang]

> On 2 July 2020, at 06:39, Daniel Gustafsson  wrote:
> > On 10 Apr 2020, at 23:50, Alexandra Wang  wrote:
>
> > On Fri, Apr 10, 2020 at 8:37 AM Ashutosh Bapat 
> > mailto:ashutosh.ba...@2ndquadrant.com>> 
> > wrote:
> > > for a multi-key value the ^
> > > points to the first column and the reader may think that that's the
> > > problematci column. Should it instead point to ( ?
> >
> > I attached a v2 of Amit's 0002 patch to also report the exact column
> > for the partition overlap errors.
>
> This patch fails to apply to HEAD due to conflicts in the create_table 
> expected
> output.  Can you please submit a rebased version?  I'm marking the CF entry
> Waiting on Author in the meantime.

Thank you Daniel. Here's the rebased patch. I also squashed the two
patches into one so it's easier to review.

--
Alex

From 334bb4ea93448073778930201c0b959c5acab924 Mon Sep 17 00:00:00 2001
From: Alexandra Wang 
Date: Mon, 13 Jul 2020 10:28:04 -0700
Subject: [PATCH] Improve check new partition bound error position report

We have been passing a dummy ParseState to ereport(). Without the source
text in the ParseState ereport does not report the error position even
if a error location is supplied. This patch passes a ParseState to
check_new_partition_bound() when it is available.

-- Create parent table
create table foo (a int, b int, c date) partition by range (b,c);

-- Before:
create table foo_part_1 partition of foo for values from (1, date '2007-01-01') to (1, date '2006-01-01');
ERROR:  empty range bound specified for partition "foo_part_1"
DETAIL:  Specified lower bound (1, '2007-01-01') is greater than or equal to upper bound (1, '2006-01-01').

-- After:
create table foo_part_1 partition of foo for values from (1, date '2007-01-01') to (1, date '2006-01-01');
ERROR:  empty range bound specified for partition "foo_part_1"
LINE 1: ...e foo_part_1 partition of foo for values from (1, date '2007...
 ^
DETAIL:  Specified lower bound (1, '2007-01-01') is greater than or equal to upper bound (1, '2006-01-01').

Co-authored-by: Ashwin Agrawal 
Co-authored-by: Amit Langote 
---
 src/backend/commands/tablecmds.c   | 15 --
 src/backend/parser/parse_utilcmd.c |  3 ++
 src/backend/partitioning/partbounds.c  | 63 ++
 src/include/partitioning/partbounds.h  |  4 +-
 src/test/regress/expected/alter_table.out  | 10 
 src/test/regress/expected/create_table.out | 30 +++
 6 files changed, 98 insertions(+), 27 deletions(-)

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index ed553f7384..4cd7709d33 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -541,7 +541,8 @@ static void ComputePartitionAttrs(ParseState *pstate, Relation rel, List *partPa
 static void CreateInheritance(Relation child_rel, Relation parent_rel);
 static void RemoveInheritance(Relation child_rel, Relation parent_rel);
 static ObjectAddress ATExecAttachPartition(List **wqueue, Relation rel,
-		   PartitionCmd *cmd);
+		   PartitionCmd *cmd,
+		   AlterTableUtilityContext * context);
 static void AttachPartitionEnsureIndexes(Relation rel, Relation attachrel);
 static void QueuePartitionConstraintValidation(List **wqueue, Relation scanrel,
 			   List *partConstraint,
@@ -1005,7 +1006,7 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId,
 		 * Check first that the new partition's bound is valid and does not
 		 * overlap with any of existing partitions of the parent.
 		 */
-		check_new_partition_bound(relname, parent, bound);
+		check_new_partition_bound(relname, parent, bound, pstate);
 
 		/*
 		 * If the default partition exists, its partition constraints will
@@ -4646,7 +4647,8 @@ ATExecCmd(List **wqueue, AlteredTableInfo *tab, Relation rel,
 	  cur_pass, context);
 			Assert(cmd != NULL);
 			if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
-ATExecAttachPartition(wqueue, rel, (PartitionCmd *) cmd->def);
+ATExecAttachPartition(wqueue, rel, (PartitionCmd *) cmd->def,
+	  context);
 			else
 ATExecAttachPartitionIdx(wqueue, rel,
 		 ((PartitionCmd *) cmd->def)->name);
@@ -16186,7 +16188,8 @@ QueuePartitionConstraintValidation(List **wqueue, Relation scanrel,
  * Return the address of the newly attached partition.
  */
 static ObjectAddress
-ATExecAttachPartition(List **wqueue, Relation rel, PartitionCmd *cmd)
+ATExecAttachPartition(List **wqueue, Relation rel, PartitionCmd *cmd,
+	  AlterTableUtilityContext * context)
 {
 	Relation	attachrel,
 catalog;
@@ -16201,6 +16204,7 @@ ATExecAttachPartition(List **wqueue, Relation rel, PartitionCmd *cmd)
 	const char *trigger_name;
 	Oid			defaultPartOid;
 	List	   *partBoundConstraint;
+	ParseState *pstate = make_parsestate(NULL);
 
 	/*
 	 * We must lock the default partition if one exists, because attaching a
@@ -16365,8 +16369,9 @@ 

Re: Report error position in partition bound check

[Alexandra Wang]

> On 2 July 2020, at 06:39, Daniel Gustafsson  wrote:
> > On 10 Apr 2020, at 23:50, Alexandra Wang  wrote:
>
> > On Fri, Apr 10, 2020 at 8:37 AM Ashutosh Bapat <
ashutosh.ba...@2ndquadrant.com >
wrote:
> > > for a multi-key value the ^
> > > points to the first column and the reader may think that that's the
> > > problematci column. Should it instead point to ( ?
> >
> > I attached a v2 of Amit's 0002 patch to also report the exact column
> > for the partition overlap errors.
>
> This patch fails to apply to HEAD due to conflicts in the create_table
expected
> output.  Can you please submit a rebased version?  I'm marking the CF
entry
> Waiting on Author in the meantime.

Thank you Daniel. Here's the rebased patch. I also squashed the two
patches into one so it's easier to review.

-- 
*Alexandra Wang*
From 572f9086fc9d6e3e7cf1e58c3a6ee21dd8cd9f1b Mon Sep 17 00:00:00 2001
From: Alexandra Wang 
Date: Fri, 10 Jul 2020 10:28:49 -0700
Subject: [PATCH] Improve check new partition bound error position report

We have been passing a dummy ParseState to ereport(). Without the source
text in the ParseState ereport does not report the error position even
if a error location is supplied. This patch passes a ParseState to
check_new_partition_bound() when it is available.

-- Create parent table
create table foo (a int, b int, c date) partition by range (b,c);

-- Before:
create table foo_part_1 partition of foo for values from (1, date '2007-01-01') to (1, date '2006-01-01');
ERROR:  empty range bound specified for partition "foo_part_1"
DETAIL:  Specified lower bound (1, '2007-01-01') is greater than or equal to upper bound (1, '2006-01-01').

-- After:
create table foo_part_1 partition of foo for values from (1, date '2007-01-01') to (1, date '2006-01-01');
ERROR:  empty range bound specified for partition "foo_part_1"
LINE 1: ...e foo_part_1 partition of foo for values from (1, date '2007...
 ^
DETAIL:  Specified lower bound (1, '2007-01-01') is greater than or equal to upper bound (1, '2006-01-01').

Co-authored-by: Ashwin Agrawal 
Co-authored-by: Amit Langote 
---
 src/backend/commands/tablecmds.c   | 15 --
 src/backend/parser/parse_utilcmd.c |  3 ++
 src/backend/partitioning/partbounds.c  | 63 ++
 src/include/partitioning/partbounds.h  |  4 +-
 src/test/regress/expected/alter_table.out  | 10 
 src/test/regress/expected/create_table.out | 30 +++
 6 files changed, 98 insertions(+), 27 deletions(-)

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 26700da278..8de7473ba7 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -541,7 +541,8 @@ static void ComputePartitionAttrs(ParseState *pstate, Relation rel, List *partPa
 static void CreateInheritance(Relation child_rel, Relation parent_rel);
 static void RemoveInheritance(Relation child_rel, Relation parent_rel);
 static ObjectAddress ATExecAttachPartition(List **wqueue, Relation rel,
-		   PartitionCmd *cmd);
+		   PartitionCmd *cmd,
+		   AlterTableUtilityContext * context);
 static void AttachPartitionEnsureIndexes(Relation rel, Relation attachrel);
 static void QueuePartitionConstraintValidation(List **wqueue, Relation scanrel,
 			   List *partConstraint,
@@ -1005,7 +1006,7 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId,
 		 * Check first that the new partition's bound is valid and does not
 		 * overlap with any of existing partitions of the parent.
 		 */
-		check_new_partition_bound(relname, parent, bound);
+		check_new_partition_bound(relname, parent, bound, pstate);
 
 		/*
 		 * If the default partition exists, its partition constraints will
@@ -4646,7 +4647,8 @@ ATExecCmd(List **wqueue, AlteredTableInfo *tab, Relation rel,
 	  cur_pass, context);
 			Assert(cmd != NULL);
 			if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
-ATExecAttachPartition(wqueue, rel, (PartitionCmd *) cmd->def);
+ATExecAttachPartition(wqueue, rel, (PartitionCmd *) cmd->def,
+	  context);
 			else
 ATExecAttachPartitionIdx(wqueue, rel,
 		 ((PartitionCmd *) cmd->def)->name);
@@ -16186,7 +16188,8 @@ QueuePartitionConstraintValidation(List **wqueue, Relation scanrel,
  * Return the address of the newly attached partition.
  */
 static ObjectAddress
-ATExecAttachPartition(List **wqueue, Relation rel, PartitionCmd *cmd)
+ATExecAttachPartition(List **wqueue, Relation rel, PartitionCmd *cmd,
+	  AlterTableUtilityContext * context)
 {
 	Relation	attachrel,
 catalog;
@@ -16201,6 +16204,7 @@ ATExecAttachPartition(List **wqueue, Relation rel, PartitionCmd *cmd)
 	const char *trigger_name;
 	Oid			defaultPartOid;
 	List	   *partBoundConstraint;
+	ParseState *pstate = make_parsestate(NULL);
 
 	/*
 	 * We must lock the default partition if one exists, because attaching 

Re: Report error position in partition bound check

[Daniel Gustafsson]

> On 10 Apr 2020, at 23:50, Alexandra Wang  wrote:

> On Fri, Apr 10, 2020 at 8:37 AM Ashutosh Bapat 
> mailto:ashutosh.ba...@2ndquadrant.com>> 
> wrote:
> > for a multi-key value the ^
> > points to the first column and the reader may think that that's the
> > problematci column. Should it instead point to ( ?
> 
> I attached a v2 of Amit's 0002 patch to also report the exact column
> for the partition overlap errors.

This patch fails to apply to HEAD due to conflicts in the create_table expected
output.  Can you please submit a rebased version?  I'm marking the CF entry
Waiting on Author in the meantime.

cheers ./daniel

Re: Report error position in partition bound check

[Alexandra Wang]

On Fri, Apr 10, 2020 at 8:37 AM Ashutosh Bapat <
ashutosh.ba...@2ndquadrant.com> wrote:
> for a multi-key value the ^
> points to the first column and the reader may think that that's the
> problematci column. Should it instead point to ( ?

I attached a v2 of Amit's 0002 patch to also report the exact column
for the partition overlap errors.
From f7cf86fcf7c1a26895e68f357717244f776fe5cd Mon Sep 17 00:00:00 2001
From: Alexandra Wang 
Date: Fri, 10 Apr 2020 13:51:53 -0700
Subject: [PATCH v2] Improve check new partition bound error position report

---
 src/backend/parser/parse_utilcmd.c |  3 ++
 src/backend/partitioning/partbounds.c  | 60 +++---
 src/test/regress/expected/alter_table.out  |  2 +-
 src/test/regress/expected/create_table.out | 28 +-
 4 files changed, 59 insertions(+), 34 deletions(-)

diff --git a/src/backend/parser/parse_utilcmd.c b/src/backend/parser/parse_utilcmd.c
index 75c122fe34..72113bb7ff 100644
--- a/src/backend/parser/parse_utilcmd.c
+++ b/src/backend/parser/parse_utilcmd.c
@@ -4170,5 +4170,8 @@ transformPartitionBoundValue(ParseState *pstate, Node *val,
 	if (!IsA(value, Const))
 		elog(ERROR, "could not evaluate partition bound expression");
 
+	/* Preserve parser location information. */
+	((Const *) value)->location = exprLocation(val);
+
 	return (Const *) value;
 }
diff --git a/src/backend/partitioning/partbounds.c b/src/backend/partitioning/partbounds.c
index dd56832efc..0ef0f6ed07 100644
--- a/src/backend/partitioning/partbounds.c
+++ b/src/backend/partitioning/partbounds.c
@@ -223,7 +223,8 @@ static int32 partition_rbound_cmp(int partnatts, FmgrInfo *partsupfunc,
 static int	partition_range_bsearch(int partnatts, FmgrInfo *partsupfunc,
 	Oid *partcollation,
 	PartitionBoundInfo boundinfo,
-	PartitionRangeBound *probe, bool *is_equal);
+	PartitionRangeBound *probe, bool *is_equal,
+	int32 *cmpval);
 static int	get_partition_bound_num_indexes(PartitionBoundInfo b);
 static Expr *make_partition_op_expr(PartitionKey key, int keynum,
 	uint16 strategy, Expr *arg1, Expr *arg2);
@@ -2810,6 +2811,7 @@ check_new_partition_bound(char *relname, Relation parent,
 	PartitionBoundInfo boundinfo = partdesc->boundinfo;
 	int			with = -1;
 	bool		overlap = false;
+	int			overlap_location = 0;
 
 	if (spec->is_default)
 	{
@@ -2904,6 +2906,7 @@ check_new_partition_bound(char *relname, Relation parent,
 		if (boundinfo->indexes[remainder] != -1)
 		{
 			overlap = true;
+			overlap_location = spec->location;
 			with = boundinfo->indexes[remainder];
 			break;
 		}
@@ -2932,6 +2935,7 @@ check_new_partition_bound(char *relname, Relation parent,
 	{
 		Const	   *val = castNode(Const, lfirst(cell));
 
+		overlap_location = val->location;
 		if (!val->constisnull)
 		{
 			int			offset;
@@ -2965,6 +2969,7 @@ check_new_partition_bound(char *relname, Relation parent,
 			{
 PartitionRangeBound *lower,
 		   *upper;
+int			cmpval;
 
 Assert(spec->strategy == PARTITION_STRATEGY_RANGE);
 lower = make_one_partition_rbound(key, -1, spec->lowerdatums, true);
@@ -2974,10 +2979,16 @@ check_new_partition_bound(char *relname, Relation parent,
  * First check if the resulting range would be empty with
  * specified lower and upper bounds
  */
-if (partition_rbound_cmp(key->partnatts, key->partsupfunc,
-		 key->partcollation, lower->datums,
-		 lower->kind, true, upper) >= 0)
+cmpval = partition_rbound_cmp(key->partnatts,
+			  key->partsupfunc,
+			  key->partcollation, lower->datums,
+			  lower->kind, true, upper);
+if (cmpval >= 0)
 {
+	/* Fetch the problem bound from lower datums list. */
+	PartitionRangeDatum *datum = list_nth(spec->lowerdatums,
+		  cmpval - 1);
+
 	ereport(ERROR,
 			(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
 			 errmsg("empty range bound specified for partition \"%s\"",
@@ -2985,7 +2996,7 @@ check_new_partition_bound(char *relname, Relation parent,
 			 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)));
+			 parser_errposition(pstate, datum->location)));
 }
 
 if (partdesc->nparts > 0)
@@ -3017,7 +3028,7 @@ check_new_partition_bound(char *relname, Relation parent,
 	 key->partsupfunc,
 	 key->partcollation,
 	 boundinfo, lower,
-	 );
+	 , );
 
 	if (boundinfo->indexes[offset + 1] < 0)
 	{
@@ -3029,7 +3040,6 @@ check_new_partition_bound(char *relname, Relation parent,
 		 */
 		if (offset + 1 < boundinfo->ndatums)
 		{
-			int32		cmpval;
 			Datum	   *datums;
 			PartitionRangeDatumKind *kind;
 			bool		is_lower;
@@ 

Re: Report error position in partition bound check

[Alexandra Wang]

On Fri, 10 Apr 2020 at 14:31, Amit Langote  wrote:
> I agree with that.  Tried that in the attached 0002, although trying
> to get the cursor to point to exactly the offending column seems a bit
> tough for partition overlap errors.  The patch does allow to single
> out which one of the lower and upper bounds is causing the overlap
> with an existing partition, which is better than now and seems helpful
> enough.
>
> Also, updated Alexandra's patch to incorporate Ashutosh's comment such
> that we get the same output with ATTACH PARTITION commands too.

Thank you Amit for updating the patches, the cursor looks much helpful now.
I
created the commitfest entry https://commitfest.postgresql.org/28/2533/

Re: Report error position in partition bound check

[Ashutosh Bapat]

On Fri, 10 Apr 2020 at 14:31, Amit Langote  wrote:

> On Thu, Apr 9, 2020 at 11:04 PM Tom Lane  wrote:
> > While I'm quite on board with providing useful error cursors,
> > the example cases in this patch don't seem all that useful:
> >
> >  -- trying to create range partition with empty range
> >  CREATE TABLE fail_part PARTITION OF range_parted2 FOR VALUES FROM (1)
> TO (0);
> >  ERROR:  empty range bound specified for partition "fail_part"
> > +LINE 1: ...E fail_part PARTITION OF range_parted2 FOR VALUES FROM (1)
> T...
> > + ^
> >  DETAIL:  Specified lower bound (1) is greater than or equal to upper
> bound (0).
> >
> > As best I can tell from these examples, the cursor will always
> > point at the FROM keyword, making it pretty unhelpful.  It seems
> > like in addition to getting the query string passed down, you
> > need to do some work on the code that's actually reporting the
> > error position.  I'd expect at a minimum that the pointer allows
> > identifying which column of a multi-column partition key is
> > giving trouble.  The phrasing of this particular message, for
> > example, suggests that it ought to point at the "1" expression.
>
> I agree with that.  Tried that in the attached 0002, although trying
> to get the cursor to point to exactly the offending column seems a bit
> tough for partition overlap errors.  The patch does allow to single
> out which one of the lower and upper bounds is causing the overlap
> with an existing partition, which is better than now and seems helpful
> enough.
>
> Also, updated Alexandra's patch to incorporate Ashutosh's comment such
> that we get the same output with ATTACH PARTITION commands too.
>

I looked at this briefly. It looks good, but I will review more in the next
CF. Do we have entry there yet? To nit-pick: for a multi-key value the ^
points to the first column and the reader may think that that's the
problematci column. Should it instead point to ( ?

-- 
Best Wishes,
Ashutosh

Re: Report error position in partition bound check

[Amit Langote]

On Thu, Apr 9, 2020 at 11:04 PM Tom Lane  wrote:
> While I'm quite on board with providing useful error cursors,
> the example cases in this patch don't seem all that useful:
>
>  -- trying to create range partition with empty range
>  CREATE TABLE fail_part PARTITION OF range_parted2 FOR VALUES FROM (1) TO (0);
>  ERROR:  empty range bound specified for partition "fail_part"
> +LINE 1: ...E fail_part PARTITION OF range_parted2 FOR VALUES FROM (1) T...
> + ^
>  DETAIL:  Specified lower bound (1) is greater than or equal to upper bound 
> (0).
>
> As best I can tell from these examples, the cursor will always
> point at the FROM keyword, making it pretty unhelpful.  It seems
> like in addition to getting the query string passed down, you
> need to do some work on the code that's actually reporting the
> error position.  I'd expect at a minimum that the pointer allows
> identifying which column of a multi-column partition key is
> giving trouble.  The phrasing of this particular message, for
> example, suggests that it ought to point at the "1" expression.

I agree with that.  Tried that in the attached 0002, although trying
to get the cursor to point to exactly the offending column seems a bit
tough for partition overlap errors.  The patch does allow to single
out which one of the lower and upper bounds is causing the overlap
with an existing partition, which is better than now and seems helpful
enough.

Also, updated Alexandra's patch to incorporate Ashutosh's comment such
that we get the same output with ATTACH PARTITION commands too.

-- 

Amit Langote
EnterpriseDB: http://www.enterprisedb.com


v3-0001-Report-error-position-in-partition-bound-check.patch
Description: Binary data


v1-0002-Improve-check_new_partition_bound-error-position-.patch
Description: Binary data

Re: Report error position in partition bound check

[Amit Langote]

On Thu, Apr 9, 2020 at 10:51 PM Ashutosh Bapat
 wrote:
>
> Hi Alexandra,
> As Michael said it will be considered for the next commitfest. But
> from a quick glance, a suggestion.
> Instead of passing NULL parsestate from ATExecAttachPartition, pass
> make_parsestate(NULL). parse_errorposition() takes care of NULL parse
> state input, but it might be safer this way. Better if we could cook
> up a parse state with the query text available in
> AlterTableUtilityContext available in ATExecCmd().

+1.  Maybe pass the *context* down to ATExecAttachPartition() from
ATExecCmd() rather than a ParseState.

-- 

Amit Langote
EnterpriseDB: http://www.enterprisedb.com

Re: Report error position in partition bound check

[Tom Lane]

While I'm quite on board with providing useful error cursors,
the example cases in this patch don't seem all that useful:

 -- trying to create range partition with empty range
 CREATE TABLE fail_part PARTITION OF range_parted2 FOR VALUES FROM (1) TO (0);
 ERROR:  empty range bound specified for partition "fail_part"
+LINE 1: ...E fail_part PARTITION OF range_parted2 FOR VALUES FROM (1) T...
+ ^
 DETAIL:  Specified lower bound (1) is greater than or equal to upper bound (0).

As best I can tell from these examples, the cursor will always
point at the FROM keyword, making it pretty unhelpful.  It seems
like in addition to getting the query string passed down, you
need to do some work on the code that's actually reporting the
error position.  I'd expect at a minimum that the pointer allows
identifying which column of a multi-column partition key is
giving trouble.  The phrasing of this particular message, for
example, suggests that it ought to point at the "1" expression.

regards, tom lane

Re: Report error position in partition bound check

[Ashutosh Bapat]

Hi Alexandra,
As Michael said it will be considered for the next commitfest. But
from a quick glance, a suggestion.
Instead of passing NULL parsestate from ATExecAttachPartition, pass
make_parsestate(NULL). parse_errorposition() takes care of NULL parse
state input, but it might be safer this way. Better if we could cook
up a parse state with the query text available in
AlterTableUtilityContext available in ATExecCmd().

On Thu, Apr 9, 2020 at 6:36 AM Alexandra Wang  wrote:
>
> Forgot to run make installcheck. Here's the new version of the patch that 
> updated the test answer file.
>


-- 
Best Wishes,
Ashutosh Bapat

Re: Report error position in partition bound check

[Michael Paquier]

On Wed, Apr 08, 2020 at 08:17:55PM -0700, Alexandra Wang wrote:
> On Wed, Apr 8, 2020 at 6:11 PM Michael Paquier  wrote:
> > Please note that you forgot to update the regression test output.
> 
> Yep thanks! Please see my previous email for the updated patch.

Thanks, I saw the update.  It looks like my email was a couple of
minutes too late :)

Could you add this patch to the next commit fest [1]?

[1]: https://commitfest.postgresql.org/28/
--
Michael


signature.asc
Description: PGP signature

Re: Report error position in partition bound check

[Alexandra Wang]

On Wed, Apr 8, 2020 at 6:11 PM Michael Paquier  wrote:
> Please note that you forgot to update the regression test output.

Yep thanks! Please see my previous email for the updated patch.

Re: Report error position in partition bound check

[Michael Paquier]

On Wed, Apr 08, 2020 at 05:15:57PM -0700, Alexandra Wang wrote:
> I have attached a patch to pass in a ParseState to
> check_new_partition_bound() to enable the reporting of the error
> position. Below is what the error message looks like before and after
> applying the patch.
> 
> Another option is to not pass the parser_errposition() argument at all
> to ereport() in this function, since the query is relatively short and
> the error message is already descriptive enough.

It depends on the complexity of the relation definition, so adding a
position looks like a good idea to me.  Anyway, even if this looks
like an oversight to me, we are post feature freeze for 13 and that's
an improvement, so this looks like material for PG14 to me.  Are there
more opinions on the matter?

Please note that you forgot to update the regression test output.
--
Michael


signature.asc
Description: PGP signature

Re: Report error position in partition bound check

[Alexandra Wang]

Forgot to run make installcheck. Here's the new version of the patch that
updated the test answer file.
From 9071918648412383e41976a01106257bd6a2539e Mon Sep 17 00:00:00 2001
From: Alexandra Wang 
Date: Wed, 8 Apr 2020 16:07:28 -0700
Subject: [PATCH v2] Report error position in partition bound check

We have been passing a dummy ParseState to ereport(). Without the source
text in the ParseState ereport does not report the error position even
if a error location is supplied. This patch passes a ParseState to
check_new_partition_bound() when it is available.

-- Create parent table
create table foo (a int, b date) partition by range (b);

-- Before:
create table foo_part_1 partition of foo for values from (date '2007-01-01') to (date '2006-01-01');
ERROR:  empty range bound specified for partition "foo_part_1"
DETAIL:  Specified lower bound ('2007-01-01') is greater than or equal to upper bound ('2006-01-01').

-- After:
create table foo_part_1 partition of foo for values from (date '2007-01-01') to (date '2006-01-01');
ERROR:  empty range bound specified for partition "foo_part_1"
LINE 1: ...eate table foo_part_1 partition of foo for values from (date...
 ^
DETAIL:  Specified lower bound ('2007-01-01') is greater than or equal to upper bound ('2006-01-01').

Co-authored-by: Ashwin Agrawal
---
 src/backend/commands/tablecmds.c   |  4 +--
 src/backend/partitioning/partbounds.c  |  3 +--
 src/include/partitioning/partbounds.h  |  4 ++-
 src/test/regress/expected/create_table.out | 30 ++
 4 files changed, 36 insertions(+), 5 deletions(-)

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 6162fb018c..46df40bee8 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -1004,7 +1004,7 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId,
 		 * Check first that the new partition's bound is valid and does not
 		 * overlap with any of existing partitions of the parent.
 		 */
-		check_new_partition_bound(relname, parent, bound);
+		check_new_partition_bound(relname, parent, bound, pstate);
 
 		/*
 		 * If the default partition exists, its partition constraints will
@@ -16268,7 +16268,7 @@ ATExecAttachPartition(List **wqueue, Relation rel, PartitionCmd *cmd)
 	 * error.
 	 */
 	check_new_partition_bound(RelationGetRelationName(attachrel), rel,
-			  cmd->bound);
+			  cmd->bound, NULL);
 
 	/* OK to create inheritance.  Rest of the checks performed there */
 	CreateInheritance(attachrel, rel);
diff --git a/src/backend/partitioning/partbounds.c b/src/backend/partitioning/partbounds.c
index 7607501fe7..dd56832efc 100644
--- a/src/backend/partitioning/partbounds.c
+++ b/src/backend/partitioning/partbounds.c
@@ -2803,12 +2803,11 @@ partitions_are_ordered(PartitionBoundInfo boundinfo, int nparts)
  */
 void
 check_new_partition_bound(char *relname, Relation parent,
-		  PartitionBoundSpec *spec)
+		  PartitionBoundSpec *spec, ParseState *pstate)
 {
 	PartitionKey key = RelationGetPartitionKey(parent);
 	PartitionDesc partdesc = RelationGetPartitionDesc(parent);
 	PartitionBoundInfo boundinfo = partdesc->boundinfo;
-	ParseState *pstate = make_parsestate(NULL);
 	int			with = -1;
 	bool		overlap = false;
 
diff --git a/src/include/partitioning/partbounds.h b/src/include/partitioning/partbounds.h
index dfc720720b..c82f77d02f 100644
--- a/src/include/partitioning/partbounds.h
+++ b/src/include/partitioning/partbounds.h
@@ -14,6 +14,7 @@
 #include "fmgr.h"
 #include "nodes/parsenodes.h"
 #include "nodes/pg_list.h"
+#include "parser/parse_node.h"
 #include "partitioning/partdefs.h"
 #include "utils/relcache.h"
 struct RelOptInfo;/* avoid including pathnodes.h here */
@@ -98,7 +99,8 @@ extern PartitionBoundInfo partition_bounds_merge(int partnatts,
  List **inner_parts);
 extern bool partitions_are_ordered(PartitionBoundInfo boundinfo, int nparts);
 extern void check_new_partition_bound(char *relname, Relation parent,
-	  PartitionBoundSpec *spec);
+	  PartitionBoundSpec *spec,
+	  ParseState *pstate);
 extern void check_default_partition_contents(Relation parent,
 			 Relation defaultRel,
 			 PartitionBoundSpec *new_spec);
diff --git a/src/test/regress/expected/create_table.out b/src/test/regress/expected/create_table.out
index 1c72f23bc9..b8de012536 100644
--- a/src/test/regress/expected/create_table.out
+++ b/src/test/regress/expected/create_table.out
@@ -677,6 +677,8 @@ LINE 1: ...BLE fail_part PARTITION OF list_parted FOR VALUES WITH (MODU...
 CREATE TABLE part_default PARTITION OF list_parted DEFAULT;
 CREATE TABLE fail_default_part PARTITION OF list_parted DEFAULT;
 ERROR:  partition "fail_default_part" conflicts with existing default partition "part_default"
+LINE 1: ...TE TABLE fail_default_part PARTITION OF list_parted DEFAULT;
+