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

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