Re: no partition pruning when partitioning using array type

On 2017/12/09 3:46, Robert Haas wrote:
> On Fri, Dec 8, 2017 at 5:40 AM, Amit Langote
> <Langote_Amit_f8@lab.ntt.co.jp> wrote:
>> I noticed that if you partition using a array type column, partition
>> pruning using constraint exclusion fails to work due to a minor problem.
>>
>> Example:
>>
>> create table p (a int[]) partition by list (a);
>> create table p1 partition of p for values in ('{1}');
>> create table p1 partition of p for values in ('{2, 3}', '{4, 5}');
>>
>> explain select a from p where a = '{1}';
>>                         QUERY PLAN
>> |---------------------------------------------------------
>>  Append  (cost=0.00..54.00 rows=14 width=32)
>>    ->  Seq Scan on p1  (cost=0.00..27.00 rows=7 width=32)
>>          Filter: (a = '{1}'::integer[])
>>    ->  Seq Scan on p2  (cost=0.00..27.00 rows=7 width=32)
>>          Filter: (a = '{1}'::integer[])
>>
>> explain select a from p where a = '{2, 3}';
>>                         QUERY PLAN
>> |---------------------------------------------------------
>>  Append  (cost=0.00..54.00 rows=14 width=32)
>>    ->  Seq Scan on p1  (cost=0.00..27.00 rows=7 width=32)
>>          Filter: (a = '{2,3}'::integer[])
>>    ->  Seq Scan on p2  (cost=0.00..27.00 rows=7 width=32)
>>          Filter: (a = '{2,3}'::integer[])
>> (5 rows)
>>
>> In the case of array type partition key, make_partition_op_expr() will
>> have to put a RelabelType node on top of the partition key Var, after
>> having selected an = operator from the array_ops family.  The RelabelType
>> causes operator_predicate_proof() to fail to consider predicate leftop and
>> clause leftop as equal, because only one of them ends up having the
>> RelabelType attached to it.
>>
>> As a simple measure, the attached patch teaches operator_predicate_proof()
>> to strip RelabelType nodes from all the nodes it compares using equal().
>> I also added a relevant test in partition_prune.sql.
> 
> I guess the question is whether that's guaranteed to be safe.  I spent
> a little bit of time thinking about it and I don't see a problem.  The
> function is careful to check that the opclasses and collations of the
> OpExprs are compatible, and it is the behavior of the operator that is
> in question here, not the column type, so your change seems OK to me.
> But I hope somebody else will also study this, because this stuff is
> fairly subtle and I would not like to be the one who breaks it.

Thanks for taking a look at it.

I will try to say a little more on why it seems safe.  RelabelType node
exists (if any) on top of a given expression node only to denote that the
operator for which the node is an input will interpret its result as of
the type RelableType.resulttype, instead of the node's original type.  No
conversion of values actually occurs before making any decisions that this
function is in charge of making, because the mismatching types in question
are known to be binary coercible.  Or more to the point, the operator that
will be used in the proof will give correct answers for the values without
having to do any conversion of values.  IOW, it's okay if we simply drop
the RelabelType, because it doesn't alter in any way the result of the
proof that operator_predicate_proof() performs.

That said, I've to come think in this particular case that the
partitioning code that generates the predicate expression should be a bit
smarter about the various types it manipulates such that RelabelType won't
be added in the first place.  In contrast, make_op(), that generates an
OpExpr from the parser representation of a = '{1}' appearing in the
query's WHERE clause, won't add the RelabelType because the underlying
type machinery that it invokes is able to conclude that that's
unnecessary.  The original patch may still be worth considering as a
solution though.

I hope someone else chimes in as well. :)

Thanks,
Amit