Re: pruning disabled for array, enum, record, range type partitionkeys - Mailing list pgsql-hackers

Thanks for the review.

On 2018/04/18 7:11, Alvaro Herrera wrote:
> Amit Langote wrote:
> 
>> Ah, I think I got it after staring at the (btree) index code for a bit.
>>
>> What pruning code got wrong is that it's comparing the expression type
>> (type of the constant arg that will be compared with partition bound
>> datums when pruning) with the partopcintype to determine if we should look
>> up the cross-type comparison/hashing procedure, whereas what the latter
>> should be compare with is the clause operator's oprighttype.  ISTM, if
>> op_in_opfamily() passed for the operator, that's the correct thing to do.
> 
> I wonder why you left out the hash partitioning case?  I don't really
> know that this is correct, but here's a delta patch as demonstration.

@@ -1546,8 +1546,8 @@ match_clause_to_partition_key(RelOptInfo *rel,
   case PARTITION_STRATEGY_HASH:
      cmpfn = get_opfamily_proc(part_scheme->partopfamily[partkeyidx],
-                               op_righttype, op_righttype,
-                               HASHEXTENDED_PROC);
+                               part_scheme->partopcintype[partkeyidx],
+                               op_righttype, HASHEXTENDED_PROC);

This change is not quite right, because it disables pruning.  The above
returns InvalidOid as there are no hash AM procedures (in pg_amproc) whose
lefttype and righttype don't match.

select count(*)
from pg_amproc
where amprocfamily in
   (select opf.oid
    from pg_opfamily opf join pg_am am on (opf.opfmethod = am.oid)
    where amname = 'hash')
and amproclefttype <> amprocrighttype;

 count
-------
     0
(1 row)

So, the original coding passes op_righttype for both lefttype and righttype.

Consider the following example:

create table h(a int) partition by hash (a);
create table h1 partition of foo for values with (modulus 2, remainder 0);
create table h2 partition of foo for values with (modulus 2, remainder 1);

insert into h values (1::bigint);
select tableoid::regclass, * from h;
 tableoid | a
----------+---
 h1     | 1
(1 row)

-- without the delta patch
explain select * from h where a = 1::bigint;
                         QUERY PLAN
------------------------------------------------------------
 Append  (cost=0.00..41.94 rows=13 width=4)
   ->  Seq Scan on h1  (cost=0.00..41.88 rows=13 width=4)
         Filter: (a = '1'::bigint)
(3 rows)

-- with
explain select * from h where a = 1::bigint;
                         QUERY PLAN
------------------------------------------------------------
 Append  (cost=0.00..83.88 rows=26 width=4)
   ->  Seq Scan on h1  (cost=0.00..41.88 rows=13 width=4)
         Filter: (a = '1'::bigint)
   ->  Seq Scan on h2  (cost=0.00..41.88 rows=13 width=4)
         Filter: (a = '1'::bigint)
(5 rows)

> (v3 is your patch, I think the only change is I renamed the tables used
> in the test)

Thanks, that seems like a good idea.

Here's v4 with parts of your delta patch merged.  I've also updated
comments in match_clause_to_partition_key() to describe the rationale of
support function look up in a bit more detail.

Regards,
Amit