Обсуждение: pruning disabled for array, enum, record, range type partition keys

Hi.

I noticed that the newly added pruning does not work if the partition key
is of one of the types that have a corresponding pseudo-type.

-- array type list partition key
create table arrpart (a int[]) partition by list (a);
create table arrpart1 partition of arrpart for values in ('{1}');
create table arrpart2 partition of arrpart for values in ('{2, 3}', '{4, 5}');
explain (costs off) select * from arrpart where a = '{1}';
               QUERY PLAN
----------------------------------------
 Append
   ->  Seq Scan on arrpart1
         Filter: (a = '{1}'::integer[])
   ->  Seq Scan on arrpart2
         Filter: (a = '{1}'::integer[])
(5 rows)

For pruning, we normally rely on the type's operator class information in
the system catalogs to be up-to-date, which if it isn't we give up on
pruning.  For example, if pg_amproc entry for a given type and AM type
(btree, hash, etc.) has not been populated, we may fail to prune using a
clause that contains an expression of the said type.  While this is the
theory for the normal cases, we should make an exception for the
pseudo-type types.  For those types, we never have pg_amproc entries with
the "real" type listed.  Instead, the pg_amproc entries contain the
corresponding pseudo-type.  For example, there aren't pg_amproc entries
with int4[] (really, its OID) as amproclefttype and/or amprocrighttype,
instead anyarray is listed there.

Attached find a patch that tries to fix that and adds relevant tests.

Thanks,
Amit

On 2018/04/09 19:14, Amit Langote wrote:
> Hi.
> 
> I noticed that the newly added pruning does not work if the partition key
> is of one of the types that have a corresponding pseudo-type.
> 
> -- array type list partition key
> create table arrpart (a int[]) partition by list (a);
> create table arrpart1 partition of arrpart for values in ('{1}');
> create table arrpart2 partition of arrpart for values in ('{2, 3}', '{4, 5}');
> explain (costs off) select * from arrpart where a = '{1}';
>                QUERY PLAN
> ----------------------------------------
>  Append
>    ->  Seq Scan on arrpart1
>          Filter: (a = '{1}'::integer[])
>    ->  Seq Scan on arrpart2
>          Filter: (a = '{1}'::integer[])
> (5 rows)
> 
> For pruning, we normally rely on the type's operator class information in
> the system catalogs to be up-to-date, which if it isn't we give up on
> pruning.  For example, if pg_amproc entry for a given type and AM type
> (btree, hash, etc.) has not been populated, we may fail to prune using a
> clause that contains an expression of the said type.  While this is the
> theory for the normal cases, we should make an exception for the
> pseudo-type types.  For those types, we never have pg_amproc entries with
> the "real" type listed.  Instead, the pg_amproc entries contain the
> corresponding pseudo-type.  For example, there aren't pg_amproc entries
> with int4[] (really, its OID) as amproclefttype and/or amprocrighttype,
> instead anyarray is listed there.
> 
> Attached find a patch that tries to fix that and adds relevant tests.

Added to the open items list.

https://wiki.postgresql.org/wiki/PostgreSQL_11_Open_Items#Open_Issues

Thanks,
Amit

Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> writes:
> I noticed that the newly added pruning does not work if the partition key
> is of one of the types that have a corresponding pseudo-type.

While I don't recall the details due to acute caffeine shortage,
there are specific coding patterns that are used in the planner
(e.g. in indxpath.c) to ensure that the code works with pseudotype
opclasses as well as simple ones.  The existence of this bug
indicates that those conventions were not followed in the pruning
code.  I wonder whether this patch makes the pruning code look
more like the pre-existing code, or even less like it.

            regards, tom lane

Thanks for the comment.

On 2018/04/09 23:22, Tom Lane wrote:
> Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> writes:
>> I noticed that the newly added pruning does not work if the partition key
>> is of one of the types that have a corresponding pseudo-type.
> 
> While I don't recall the details due to acute caffeine shortage,
> there are specific coding patterns that are used in the planner
> (e.g. in indxpath.c) to ensure that the code works with pseudotype
> opclasses as well as simple ones.  The existence of this bug
> indicates that those conventions were not followed in the pruning
> code.  I wonder whether this patch makes the pruning code look
> more like the pre-existing code, or even less like it.

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.

Once I changed the code to do it that way, no special considerations are
needed to handle pseudo-type type partition key.

Attached please find the updated patch.

Thanks,
Amit

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.

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

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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

Amit Langote wrote:

> On 2018/04/18 7:11, Alvaro Herrera wrote:
> 
> @@ -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.

Makes sense.  Still, I was expecting that pruning of hash partitioning
would also work for pseudotypes, yet it doesn't.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Thu, Apr 19, 2018 at 12:01 AM, Alvaro Herrera
<alvherre@alvh.no-ip.org> wrote:
> Amit Langote wrote:
>
>> On 2018/04/18 7:11, Alvaro Herrera wrote:
>>
>> @@ -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.
>
> Makes sense.  Still, I was expecting that pruning of hash partitioning
> would also work for pseudotypes, yet it doesn't.

It does?

+-- array type hash partition key
+create table pph_arrpart (a int[]) partition by hash (a);
+create table pph_arrpart1 partition of pph_arrpart for values with
(modulus 2, remainder 0);
+create table pph_arrpart2 partition of pph_arrpart for values with
(modulus 2, remainder 1);
+insert into pph_arrpart values ('{1}'), ('{1, 2}'), ('{4, 5}');
+select tableoid::regclass, * from pph_arrpart order by 1;
+   tableoid   |   a
+--------------+-------
+ pph_arrpart1 | {1,2}
+ pph_arrpart1 | {4,5}
+ pph_arrpart2 | {1}
+(3 rows)
+
+explain (costs off) select * from pph_arrpart where a = '{1}';
+               QUERY PLAN
+----------------------------------------
+ Append
+   ->  Seq Scan on pph_arrpart2
+         Filter: (a = '{1}'::integer[])
+(3 rows)
+
+explain (costs off) select * from pph_arrpart where a = '{1, 2}';
+                QUERY PLAN
+------------------------------------------
+ Append
+   ->  Seq Scan on pph_arrpart1
+         Filter: (a = '{1,2}'::integer[])
+(3 rows)

Thanks,
Amit

Amit Langote wrote:
> On Thu, Apr 19, 2018 at 12:01 AM, Alvaro Herrera
> <alvherre@alvh.no-ip.org> wrote:

> > Makes sense.  Still, I was expecting that pruning of hash partitioning
> > would also work for pseudotypes, yet it doesn't.
> 
> It does?

Aha, so it does.

While staring at this new code, I was confused as to why we didn't use
the commutator if the code above had determined one.  I was unable to
cause a test to fail, so I put that thought aside.

Some time later, after restructuring the code in a way that seemed to
make more sense to me (and saving one get_op_opfamily_properties call
for the case of the not-equals operator), I realized that with the new
code we can store the opstrategy in the PartClause instead of leaving it
as Invalid and look it up again later, so I did that.  And lo and
behold, the tests that used commutators started failing!  So I fixed
that one in the obvious way, and the tests work fully again.

Please give this version another look.  I also rewrote a couple of
comments.

I now wonder if there's anything else that equivclass.c or indxpath.c
can teach us on this topic.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> I now wonder if there's anything else that equivclass.c or indxpath.c
> can teach us on this topic.

I've been meaning to review this but have been a bit distracted.
Will try to look tomorrow.

            regards, tom lane

Hi.

On 2018/04/19 6:45, Alvaro Herrera wrote:
> Amit Langote wrote:
>> On Thu, Apr 19, 2018 at 12:01 AM, Alvaro Herrera
>> <alvherre@alvh.no-ip.org> wrote:
> 
>>> Makes sense.  Still, I was expecting that pruning of hash partitioning
>>> would also work for pseudotypes, yet it doesn't.
>>
>> It does?
> 
> Aha, so it does.
> 
> While staring at this new code, I was confused as to why we didn't use
> the commutator if the code above had determined one.  I was unable to
> cause a test to fail, so I put that thought aside.

Oops, you're right.  Shouldn't have ignored the commutator.

> Some time later, after restructuring the code in a way that seemed to
> make more sense to me (and saving one get_op_opfamily_properties call
> for the case of the not-equals operator), I realized that with the new
> code we can store the opstrategy in the PartClause instead of leaving it
> as Invalid and look it up again later, so I did that.  And lo and
> behold, the tests that used commutators started failing!  So I fixed
> that one in the obvious way, and the tests work fully again.
> 
> Please give this version another look.  I also rewrote a couple of
> comments.

Thanks, your rewritten version looks much better.

> I now wonder if there's anything else that equivclass.c or indxpath.c
> can teach us on this topic.

I have referenced indxpath.c number of times when writing this code (for
example, match_clause_to_indexcol), but never equivclass.c.

Thanks,
Amit

Amit Langote wrote:

> On 2018/04/19 6:45, Alvaro Herrera wrote:

> > Please give this version another look.  I also rewrote a couple of
> > comments.
> 
> Thanks, your rewritten version looks much better.

Thanks!  Pushed now.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services