On Fri, Sep 25, 2020 at 12:02 AM Tom Lane <t...@sss.pgh.pa.us> wrote:
> [ cc'ing Peter, since his opinion seems to have got us here in the first
> place ]
>
> Amit Langote <amitlangot...@gmail.com> writes:
> > On Thu, Sep 24, 2020 at 7:19 AM Tom Lane <t...@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.
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
