On 09/02/2017 06:44 AM, Thomas Munro wrote:
> On Wed, Aug 16, 2017 at 9:23 PM, Konstantin Knizhnik
> <k(dot)knizhnik(at)postgrespro(dot)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