Re: A bug in mapping attributes in ATExecAttachPartition()

PFA patch set addressing comments by Tom and Amit.

0001 is same as Robert's patch.

On Wed, Jun 14, 2017 at 7:20 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Tue, Jun 13, 2017 at 5:28 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Robert Haas <robertmhaas@gmail.com> writes:
>>> 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've not reviewed the logic changes in predtest.c in detail, but
>> I think this is a reasonable direction to go in.  Two suggestions:
>>
>> 1. predicate_refuted_by() should grow the extra argument at the
>> same time.  There's no good reason to be asymmetric.
>
> OK.

0002 has these changes.

>
>> 2. It might be clearer, and would certainly be shorter, to call the
>> extra argument something like "null_is_true".
>
> I think it's pretty darn important to make it clear that the argument
> only applies to the clauses supplied as axioms, and not to the
> predicate to be proven; if you want to control how the *predicate* is
> handled with respect to nulls, change your selection as among
> predicate_implied_by() and predicate_refuted_by().  For that reason, I
> disesteem null_is_true, but I'm open to other suggestions.

The extern functions viz. predicate_refuted_by() and
predicate_implied_by() both accept restrictinfo_list and so the new
argument gets name restrictinfo_is_check, which is fine. But the
static minions have the corresponding argument named clause but the
new argument isn't named clause_is_check. I think it would be better
to be consistent everywhere and use either clause or restrictinfo.

0004 patch does that, it renames restrictinfo_list as clause_list and
the boolean argument as clause_is_check.

0003 addresses comments by Amit Langote.

In your original patch, if restrictinfo_is_check is true, we will call
operator_predicate_proof(), which does not handle anything other than
an operator expression. So, it's going to return false from that
function. Looking at the way argisrow is handled in that function, it
looks like we don't want to pass NullTest expression to
operator_predicate_proof(). 0005 handles the boolean flag in the same
way as argisrow is handled.

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company