Re: [HACKERS] A bug in mapping attributes in ATExecAttachPartition()

On 2017/06/13 23:24, Robert Haas wrote:
> On Mon, Jun 12, 2017 at 4:09 AM, Amit Langote
> <Langote_Amit_f8@lab.ntt.co.jp> wrote:
>> On 2017/06/09 20:49, Ashutosh Bapat wrote:
>>> May be we should pass a flag to predicate_implied_by() to handle NULL
>>> behaviour for CHECK constraints. Partitioning has shown that it needs
>>> to use predicate_implied_by() for comparing constraints and there may
>>> be other cases that can come up in future. Instead of handling it
>>> outside predicate_implied_by() we may want to change it under a flag.
>>
>> IMHO, it may not be a good idea to modify predtest.c to suit the
>> partitioning code's needs.  The workaround of checking that NOT NULL
>> constraints on partitioning columns exist seems to me to be simpler than
>> hacking predtest.c to teach it about the new behavior.
> 
> On the plus side, it might also work correctly.  I mean, the problem
> with what you've done here is that (a) you're completely giving up on
> expressions as partition keys and (b) even if no expressions are used
> for partitioning, you're still giving up unless there are NOT NULL
> constraints on the partitions.  Now, maybe that doesn't sound so bad,
> but what it means is that if you copy-and-paste the partition
> constraint into a CHECK constraint on a new table, you can't skip the
> validation scan when attaching it:
> 
> rhaas=# create table foo (a int, b text) partition by range (a);
> CREATE TABLE
> rhaas=# create table foo1 partition of foo for values from (0) to (10);
> CREATE TABLE
> rhaas=# \d+ foo1
>                                     Table "public.foo1"
>  Column |  Type   | Collation | Nullable | Default | Storage  | Stats
> target | Description
> --------+---------+-----------+----------+---------+----------+--------------+-------------
>  a      | integer |           |          |         | plain    |              |
>  b      | text    |           |          |         | extended |              |
> Partition of: foo FOR VALUES FROM (0) TO (10)
> Partition constraint: ((a IS NOT NULL) AND (a >= 0) AND (a < 10))
> 
> rhaas=# drop table foo1;
> DROP TABLE
> rhaas=# create table foo1 (like foo, check ((a IS NOT NULL) AND (a >=
> 0) AND (a < 10)));
> CREATE TABLE
> rhaas=# alter table foo attach partition foo1 for values from (0) to (10);
> ALTER TABLE
> 
> I think that's going to come as an unpleasant surprise to more than
> one user.  I'm not sure exactly how we need to restructure things here
> so that this works properly, and maybe modifying
> predicate_implied_by() isn't the right thing at all; for instance,
> there's also predicate_refuted_by(), which maybe could be used in some
> way (inject NOT?).  But I don't much like the idea that you copy and
> paste the partitioning constraint into a CHECK constraint and that
> doesn't work.  That's not cool.

I agree with this argument.  I just tried the patch you posted in the
other email and I like how easy it makes the life for users in that they
can just look at the partition constraint of an existing partition (thanks
to 1848b73d45!) and frame the check constraint of the new partition to
attach accordingly.

IOW, +1 from me to the Ashutosh's idea.

Thanks,
Amit