Re: Secondary index access optimizations

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!

--
Thomas Munro
http://www.enterprisedb.com