Re: Report error position in partition bound check

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