Hello, let me make some comments.
At Tue, 30 Jan 2018 09:57:44 +0900, Amit Langote
<langote_amit...@lab.ntt.co.jp> wrote in
<4a7dda08-b883-6e5e-b0bf-f5ce95584...@lab.ntt.co.jp>
> Hi Jesper.
>
> On 2018/01/29 22:13, Jesper Pedersen wrote:
> > Hi Amit,
> >
> > On 01/26/2018 04:17 AM, Amit Langote wrote:
> >> I made a few of those myself in the updated patches.
> >>
> >> Thanks a lot again for your work on this.
> >>
> >
> > This needs a rebase.
>
> AFAICS, v22 cleanly applies to HEAD (c12693d8f3 [1]), compiles, and make
> check passes.
Yes, it cleanly applies to HEAD and seems working.
0001 seems fine.
I have some random comments on 0002.
-extract_partition_key_clauses implicitly assumes that the
commutator of inequality operator is the same to the original
operator. (commutation for such operators is an identity
function.)
I believe it is always true for a sane operator definition, but
perhaps wouldn't it be better using commutator instead of
opclause->opno as the source of negator if any? (Attached diff 1)
-get_partitions_from_or_clause_args abandons arg_partset with all
bit set when partition constraint doesn't refute whole the
partition. Finally get_partitions_from_clauses does the same
thing but it's waste of cycles and looks weird. It seems to have
intended to return immediately there.
> /* Couldn't eliminate any of the partitions. */
> partdesc = RelationGetPartitionDesc(context->relation);
> - arg_partset = bms_add_range(NULL, 0, partdesc->nparts - 1);
> + return bms_add_range(NULL, 0, partdesc->nparts - 1);
> }
>
> subcontext.rt_index = context->rt_index;
> subcontext.relation = context->relation;
> subcontext.clauseinfo = &partclauseinfo;
!> arg_partset = get_partitions_from_clauses(&subcontext);
-get_partitions_from_or_clause_args converts IN (null) into
nulltest and the nulltest doesn't exclude a child that the
partition key column can be null.
drop table if exists p;
create table p (a int, b int) partition by list (a);
create table c1 partition of p for values in (1, 5, 7);
create table c2 partition of p for values in (4, 6, null);
insert into p values (1, 0), (null, 0);
explain select tableoid::regclass, * from p where a in (1, null);
> QUERY PLAN
> -----------------------------------------------------------------
> Result (cost=0.00..76.72 rows=22 width=12)
> -> Append (cost=0.00..76.50 rows=22 width=12)
> -> Seq Scan on c1 (cost=0.00..38.25 rows=11 width=12)
> Filter: (a = ANY ('{1,NULL}'::integer[]))
> -> Seq Scan on c2 (cost=0.00..38.25 rows=11 width=12)
> Filter: (a = ANY ('{1,NULL}'::integer[]))
Although the clause "a in (null)" doesn't match the (null, 0)
row so it donesn't harm finally, I don't think this is a right
behavior. null in an SAOP rightop should be just ignored on
partition pruning. Or is there any purpose of this behavior?
- In extract_bounding_datums, clauseinfo is set twice to the same
value.
regards,
--
Kyotaro Horiguchi
NTT Open Source Software Center
diff --git a/src/backend/catalog/partition.c b/src/backend/catalog/partition.c
index ab17524..a2488ab 100644
--- a/src/backend/catalog/partition.c
+++ b/src/backend/catalog/partition.c
@@ -2111,7 +2111,6 @@ extract_partition_key_clauses(PartitionKey partkey, List *clauses,
PartClause *pc;
Oid partopfamily = partkey->partopfamily[i];
Oid partcoll = partkey->partcollation[i];
- Oid commutator = InvalidOid;
AttrNumber partattno = partkey->partattrs[i];
Expr *partexpr = NULL;
@@ -2144,6 +2143,7 @@ extract_partition_key_clauses(PartitionKey partkey, List *clauses,
if (IsA(clause, OpExpr))
{
OpExpr *opclause = (OpExpr *) clause;
+ Oid comparator = opclause->opno;
Expr *leftop,
*rightop,
*valueexpr;
@@ -2161,13 +2161,14 @@ extract_partition_key_clauses(PartitionKey partkey, List *clauses,
valueexpr = rightop;
else if (EXPR_MATCHES_PARTKEY(rightop, partattno, partexpr))
{
- valueexpr = leftop;
-
- commutator = get_commutator(opclause->opno);
+ Oid commutator = get_commutator(opclause->opno);
/* nothing we can do unless we can swap the operands */
if (!OidIsValid(commutator))
continue;
+
+ valueexpr = leftop;
+ comparator = commutator;
}
else
/* Clause does not match this partition key. */
@@ -2212,7 +2213,7 @@ extract_partition_key_clauses(PartitionKey partkey, List *clauses,
* equality operator *and* this is a list partitioned
* table, we can use it prune partitions.
*/
- negator = get_negator(opclause->opno);
+ negator = get_negator(comparator);
if (OidIsValid(negator) &&
op_in_opfamily(negator, partopfamily))
{
@@ -2236,7 +2237,7 @@ extract_partition_key_clauses(PartitionKey partkey, List *clauses,
}
pc = (PartClause *) palloc0(sizeof(PartClause));
- pc->opno = OidIsValid(commutator) ? commutator : opclause->opno;
+ pc->opno = comparator;
pc->inputcollid = opclause->inputcollid;
pc->value = valueexpr;
