Re: A bug in mapping attributes in ATExecAttachPartition()

On Mon, Jun 12, 2017 at 4:09 AM, Amit Langote
<Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)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.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company