Re: [Sender Address Forgery]Re: [Sender Address Forgery]Re: [HACKERS] path toward faster partition pruning

From: Amit Langote <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp>
To: David Rowley <david(dot)rowley(at)2ndquadrant(dot)com>
Cc: Kyotaro HORIGUCHI <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp>, Rajkumar Raghuwanshi <rajkumar(dot)raghuwanshi(at)enterprisedb(dot)com>, Robert Haas <robertmhaas(at)gmail(dot)com>, Dilip Kumar <dilipbalaut(at)gmail(dot)com>, Beena Emerson <memissemerson(at)gmail(dot)com>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: [Sender Address Forgery]Re: [Sender Address Forgery]Re: [HACKERS] path toward faster partition pruning
Date: 2018-01-12 02:27:11
Message-ID: 841587dd-cd17-286f-7d26-ea9c1193aca1@lab.ntt.co.jp
Views: Raw Message | Whole Thread | Download mbox
Thread:
Lists: pgsql-hackers

On 2018/01/11 19:23, David Rowley wrote:
> On 11 January 2018 at 22:52, Amit Langote <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp> wrote:
>> On 2018/01/10 10:55, David Rowley wrote:
>>> One more thing I discovered while troubleshooting a bug Beena reported
>>> in the run-time partition pruning patch is that
>>> classify_partition_bounding_keys properly does;
>>>
>>> if (EXPR_MATCHES_PARTKEY(leftop, partattno, partexpr))
>>> constexpr = rightop;
>>> else if (EXPR_MATCHES_PARTKEY(rightop, partattno, partexpr))
>>> constexpr = leftop;
>>> else
>>> /* Clause not meant for this column. */
>>> continue;
>>>
>>> for OpExpr clauses, but does not do the same for leftop for the
>>> ScalarArrayOpExpr test.
>>
>> I'm not sure why we'd need to do that? Does the syntax of clauses that
>> use a ScalarArrayOpExpr() allow them to have the partition key on RHS?
>
> No, but there's no test to ensure the leftop matches the partition key.
>
> There's just:
>
> ScalarArrayOpExpr *saop = (ScalarArrayOpExpr *) clause;
> Oid saop_op = saop->opno;
> Oid saop_opfuncid = saop->opfuncid;
> Oid saop_coll = saop->inputcollid;
> Node *leftop = (Node *) linitial(saop->args),
> *rightop = (Node *) lsecond(saop->args);
> List *elem_exprs,
> *elem_clauses;
> ListCell *lc1;
> bool negated = false;
>
> /*
> * In case of NOT IN (..), we get a '<>', which while not
> * listed as part of any operator family, we are able to
> * handle the same if its negator is indeed a part of the
> * partitioning operator family.
> */
> if (!op_in_opfamily(saop_op, partopfamily))
> {
> Oid negator = get_negator(saop_op);
> int strategy;
> Oid lefttype,
> righttype;
>
>
> if (!OidIsValid(negator))
> continue;
> get_op_opfamily_properties(negator, partopfamily, false,
> &strategy,
> &lefttype, &righttype);
> if (strategy == BTEqualStrategyNumber)
> negated = true;
> }
>
> Since there's nothing to reject the clause that does not match the
> partition key, the IN's left operand might be of any random type, and
> may well not be in partopfamily, so when it comes to looking up
> get_op_opfamily_properties() you'll hit: elog(ERROR, "operator %u is
> not a member of opfamily %u", opno, opfamily);

Ah, I completely missed that. So we need something like the following in
this IsA(clause, ScalarArrayOpExpr) block:

+ /* Clause does not match this partition key. */
+ if (!EXPR_MATCHES_PARTKEY(leftop, partattno, partexpr))
+ continue;
+

> Still looking at the v17 patch here, but I also don't see a test to
> see if the IsBooleanOpfamily(partopfamily) is checking it matches the
> partition key.

You're right. Added checks there as well.

>> Can you point me to the email where Beena reported the problem in question?
>
> https://www.postgresql.org/message-id/CAOG9ApERiop7P=GRkqQKa82AuBKjxN3qVixie3WK4WqQpEjS6g@mail.gmail.com
>
> To save you from having to look at the run-time prune patch, here's
> case that break in v18.
>
> create table xy (a int, b text) partition by range (a,b);
> create table xy1 partition of xy for values from (0,'a') to (10, 'b');
> select * from xy where a = 1 and b in('x','y');
> ERROR: operator 531 is not a member of opfamily 1976

You'll be able to see that the error no longer appears with the attached
updated set of patches, but I'm now seeing that the resulting plan with
patched for this particular query differs from what master (constraint
exclusion) produces. Master produces a plan with no partitions (as one
would think is the correct plan), whereas patched produces a plan
including the xy1 partition. I will think about that a bit and post
something later.

Thanks,
Amit

Attachment Content-Type Size
v19-0001-Some-interface-changes-for-partition_bound_-cmp-.patch text/plain 11.6 KB
v19-0002-Introduce-a-get_partitions_from_clauses.patch text/plain 64.8 KB
v19-0003-Move-some-code-of-set_append_rel_size-to-separat.patch text/plain 8.6 KB
v19-0004-More-refactoring-around-partitioned-table-Append.patch text/plain 12.7 KB
v19-0005-Teach-planner-to-use-get_partitions_from_clauses.patch text/plain 44.6 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Stephen Frost 2018-01-12 02:37:17 Re: [PATCH] using arc4random for strong randomness matters.
Previous Message Thomas Munro 2018-01-12 02:19:26 Re: Transform for pl/perl