Re: [HACKERS] Secondary index access optimizations

On 09/02/2017 06:44 AM, Thomas Munro wrote:
> On Wed, Aug 16, 2017 at 9:23 PM, Konstantin Knizhnik
> <k.knizhnik@postgrespro.ru> wrote:
>> postgres=# explain select * from bt where k between 1 and 20000 and v = 100;
>>                                QUERY PLAN
>> ----------------------------------------------------------------------
>>   Append  (cost=0.29..15.63 rows=2 width=8)
>>     ->  Index Scan using dti1 on dt1  (cost=0.29..8.30 rows=1 width=8)
>>           Index Cond: (v = 100)
>>     ->  Index Scan using dti2 on dt2  (cost=0.29..7.33 rows=1 width=8)
>>           Index Cond: (v = 100)
>>           Filter: (k <= 20000)
>> (6 rows)
> +1
>
> This seems like a good feature to me: filtering stuff that is
> obviously true is a waste of CPU cycles and may even require people to
> add redundant stuff to indexes.  I was pondering something related to
> this over in the partition-wise join thread (join quals that are
> implied by partition constraints and should be discarded).
>
> It'd be interesting to get Amit Langote's feedback, so I CC'd him.
> I'd be surprised if he and others haven't got a plan or a patch for
> this down the back of the sofa.
>
> I might be missing some higher level architectural problems with the
> patch, but for what it's worth here's some feedback after a first read
> through:
>
> --- a/src/backend/optimizer/util/plancat.c
> +++ b/src/backend/optimizer/util/plancat.c
> @@ -1441,6 +1441,20 @@ relation_excluded_by_constraints(PlannerInfo *root,
>       if (predicate_refuted_by(safe_constraints, rel->baserestrictinfo, false))
>           return true;
>
> +    /*
> +     * Remove from restrictions list items implied by table constraints
> +     */
> +    safe_restrictions = NULL;
> +    foreach(lc, rel->baserestrictinfo)
> +    {
> +        RestrictInfo *rinfo = (RestrictInfo *) lfirst(lc);
>
> I think the new way to write this is "RestrictInfo *rinfo =
> lfirst_node(RestrictInfo, lc)".
>
> +        if (!predicate_implied_by(list_make1(rinfo->clause),
> safe_constraints, false)) {
> +            safe_restrictions = lappend(safe_restrictions, rinfo);
> +        }
> +    }
> +    rel->baserestrictinfo = safe_restrictions;
>
> It feels wrong to modify rel->baserestrictinfo in
> relation_excluded_by_constraints().  I think there should probably be
> a function with a name that more clearly indicates that it mutates its
> input, and you should call that separately after
> relation_excluded_by_constraints().  Something like
> remove_restrictions_implied_by_constraints()?
>
>> It is because operator_predicate_proof is not able to understand that k <
>> 20001 and k <= 20000 are equivalent for integer type.
>>
>> [...]
>>
>>   /*
>>    * operator_predicate_proof
>>       if (clause_const->constisnull)
>>           return false;
>>
>> +    if (!refute_it
>> +        && ((pred_op == Int4LessOrEqualOperator && clause_op ==
>> Int4LessOperator)
>> +            || (pred_op == Int8LessOrEqualOperator && clause_op ==
>> Int8LessOperator)
>> +            || (pred_op == Int2LessOrEqualOperator && clause_op ==
>> Int2LessOperator))
>> +        && pred_const->constbyval && clause_const->constbyval
>> +        && pred_const->constvalue + 1 == clause_const->constvalue)
>> +    {
>> +        return true;
>> +    }
>> +
> I'm less sure about this part.  It seems like a slippery slope.
>
> A couple of regression test failures:
>
>       inherit                  ... FAILED
>       rowsecurity              ... FAILED
> ========================
>   2 of 179 tests failed.
> ========================
>
> I didn't try to understand the rowsecurity one, but at first glance
> all the differences reported in the inherit test are in fact cases
> where your patch is doing the right thing and removing redundant
> filters from scans.  Nice!
>
Thank you for review.
I attached new version of the patch with remove_restrictions_implied_by_constraints() function.
Concerning failed tests - this is actually result of this optimization: extra filter conditions are removed from query
plans.
Sorry, I have not included updated version of expected test output files to the patch.
Now I did it.

-- 
Konstantin Knizhnik
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers