Re: Report error position in partition bound check

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

Amit Langote <amitlangote09@gmail.com> writes:
> On Thu, Sep 24, 2020 at 7:19 AM Tom Lane <tgl@sss.pgh.pa.us> 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);