Re: Report error position in partition bound check

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
14 messages Options
Reply | Threaded
Open this post in threaded view
|

Re: Report error position in partition bound check

Alexandra Wang-2
> On 2 July 2020, at 06:39, Daniel Gustafsson <[hidden email]> wrote:

> > On 10 Apr 2020, at 23:50, Alexandra Wang <[hidden email]> wrote:
>
> > On Fri, Apr 10, 2020 at 8:37 AM Ashutosh Bapat <[hidden email] <mailto:[hidden email]>> 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

v3-0001-Improve-check-new-partition-bound-error-position-rep.patch (32K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Report error position in partition bound check

Ashutosh Bapat-3


On Fri, 10 Jul 2020 at 23:31, Alexandra Wang <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: Report error position in partition bound check

Michael Paquier-2
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 (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Report error position in partition bound check

Amit Langote
In reply to this post by Ashutosh Bapat-3
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
<[hidden email]> 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 (34K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Report error position in partition bound check

Ashutosh Bapat-3


On Thu, 17 Sep 2020 at 13:06, Amit Langote <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: Report error position in partition bound check

Amit Langote
Thanks Ashutosh.

On Fri, Sep 18, 2020 at 7:33 PM Ashutosh Bapat
<[hidden email]> 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 (38K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Report error position in partition bound check

Ashutosh Bapat-3


On Wed, 23 Sep 2020 at 14:41, Amit Langote <[hidden email]> wrote:
Thanks Ashutosh.

On Fri, Sep 18, 2020 at 7:33 PM Ashutosh Bapat
<[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: Report error position in partition bound check

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

Great, thanks for your time on this.

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


Reply | Threaded
Open this post in threaded view
|

Re: Report error position in partition bound check

Tom Lane-2
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


Reply | Threaded
Open this post in threaded view
|

Re: Report error position in partition bound check

Amit Langote
On Thu, Sep 24, 2020 at 7:19 AM Tom Lane <[hidden email]> 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 (7K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Report error position in partition bound check

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

Amit Langote <[hidden email]> writes:

> On Thu, Sep 24, 2020 at 7:19 AM Tom Lane <[hidden email]> 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);
Reply | Threaded
Open this post in threaded view
|

Re: Report error position in partition bound check

Amit Langote
On Fri, Sep 25, 2020 at 12:02 AM Tom Lane <[hidden email]> wrote:

> [ cc'ing Peter, since his opinion seems to have got us here in the first place ]
>
> Amit Langote <[hidden email]> writes:
> > On Thu, Sep 24, 2020 at 7:19 AM Tom Lane <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: Report error position in partition bound check

Tom Lane-2
Amit Langote <[hidden email]> writes:

> On Fri, Sep 25, 2020 at 12:02 AM Tom Lane <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: Report error position in partition bound check

Amit Langote
On Tue, Sep 29, 2020 at 2:01 AM Tom Lane <[hidden email]> wrote:

> Amit Langote <[hidden email]> writes:
> > On Fri, Sep 25, 2020 at 12:02 AM Tom Lane <[hidden email]> 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