Re: Report error position in partition bound check

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: Amit Langote <amitlangote09(at)gmail(dot)com>
Cc: Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com>, Ashutosh Bapat <ashutosh(dot)bapat(at)2ndquadrant(dot)com>, Alexandra Wang <alexandra(dot)wanglei(at)gmail(dot)com>, Daniel Gustafsson <daniel(at)yesql(dot)se>, PostgreSQL mailing lists <pgsql-hackers(at)postgresql(dot)org>, "Ashwin Agrawal (Pivotal)" <aagrawal(at)pivotal(dot)io>
Subject: Re: Report error position in partition bound check
Date: 2020-09-24 15:02:43
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

[ 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);
>> 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");
>> 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");
regression=# insert into foo values ('a' COLLATE "POSIX");
regression=# update foo set f1 = 'b' COLLATE "POSIX";

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

Attachment Content-Type Size
remove-useless-collation-check-wip.patch text/x-diff 2.3 KB

In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message Bruce Momjian 2020-09-24 15:43:40 Re: "cert" + clientcert=verify-ca in pg_hba.conf?
Previous Message Heikki Linnakangas 2020-09-24 14:54:11 Re: Refactor pg_rewind code and make it work against a standby