Re: A bug in mapping attributes in ATExecAttachPartition()

On 2017/06/14 5:36, Robert Haas wrote:
> On Tue, Jun 13, 2017 at 10:24 AM, Robert Haas <robertmhaas@gmail.com> wrote:
>> 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.
> 
> OK, I think I see the problem here.  predicate_implied_by() and
> predicate_refuted_by() differ in what they assume about the predicate
> evaluating to NULL, but both of them assume that if the clause
> evaluates to NULL, that's equivalent to false.  So there's actually no
> option to get the behavior we want here, which is to treat both
> operands using CHECK-semantics (null is true) rather than
> WHERE-semantics (null is false).
> 
> Given that, Ashutosh's proposal of passing an additional flag to
> predicate_implied_by() seems like the best option.  Here's a patch
> implementing that.

I tried this patch and it seems to work correctly.

Some comments on the patch itself:

The following was perhaps included for debugging?

+#include "nodes/print.h"

I think the following sentence in a comment near the affected code in
ATExecAttachPartition() needs to be removed.

     *
     * There is a case in which we cannot rely on just the result of the
     * proof.

We no longer need the Bitmapset not_null_attrs.  So, the line declaring it
and the following line can be removed:

                    not_null_attrs = bms_add_member(not_null_attrs, i);

I thought I would make these changes myself and send the v2, but realized
that you might be updating it yourself based on Tom's comments, so didn't.

By the way, I mentioned an existing problem in one of the earlier emails
on this thread about differing attribute numbers in the table being
attached causing predicate_implied_by() to give up due to structural
inequality of Vars.  To clarify: table's check constraints will bear the
table's attribute numbers whereas the partition constraint generated using
get_qual_for_partbound() (the predicate) bears the parent's attribute
numbers.  That results in Var arguments of the expressions passed to
predicate_implied_by() not matching and causing the latter to return
failure prematurely.  Attached find a patch to fix that that applies on
top of your patch (added a test too).

Thanks,
Amit