Re: PATCH: index-only scans with partial indexes - Mailing list pgsql-hackers

From Kyotaro HORIGUCHI
Subject Re: PATCH: index-only scans with partial indexes
Date
Msg-id 20160225.195659.208751048.horiguchi.kyotaro@lab.ntt.co.jp
Whole thread Raw
In response to Re: PATCH: index-only scans with partial indexes  (Tomas Vondra <tomas.vondra@2ndquadrant.com>)
Responses Re: PATCH: index-only scans with partial indexes  (Tomas Vondra <tomas.vondra@2ndquadrant.com>)
List pgsql-hackers
Hello, Tomas. my cerebral cortext gets squeezed by jumping from
parser to planner.

At Wed, 24 Feb 2016 01:13:22 +0100, Tomas Vondra <tomas.vondra@2ndquadrant.com> wrote in
<56CCF5A2.5040702@2ndquadrant.com>
> Hi,
> 
> On 12/06/2015 11:48 PM, Tomas Vondra wrote:
> >    /*
> >     * Frequently, there will be no partial indexes, so first check to
> >     * make sure there's something useful to do here.
> >     */
> >    have_partial = false;
> >    foreach(lc, rel->indexlist)
> >    {
> >      IndexOptInfo *index = (IndexOptInfo *) lfirst(lc);
> >
> >      /*
> >       * index rinfos are the same to baseristrict infos for non-partial
> >       * indexes
> >       */
> >      index->indrinfos = rel->baserestrictinfo;
> >
> >      if (index->indpred == NIL)
> >        continue;      /* ignore non-partial indexes */
> >
> >      if (index->predOK)
> >        continue;      /* don't repeat work if already proven OK */
> >
> >      have_partial = true;
> >      break;
> >    }
> 
> Attached is a v6 of the patch, which is actually the version submitted
> by Kyotaro-san on 2015/10/8 rebased to current master and with two
> additional changes.

This relies on the fact I believe that no indexrelinfos are
shared among any two baserels. Are you OK with that?

> Firstly, I've removed the "break" from the initial foreach loop in
> check_partial_indexes(). As explained in the previous message, I
> believe this was a bug in the patch.

I agreed. The break is surely a bug. (the regression didn't find
it though..)

> Secondly, I've tried to improve the comments to explain a bit better
> what the code is doing.

In check_partial_indexes, the following commnet.

>   /*
>    * Update restrictinfo for this index by excluding clauses that
>    * are implied by the index predicates. We first quickly check if
>    * there are any implied clauses, and if we found at least one
>    * we do the actual work. This way we don't need the construct
>    * a copy of the list unless actually needed.
>    */

Is "need the construct" a mistake of "need to construct" ?


match_restriction_clauses_to_index is called only from
create_index_paths, and now the former doesn't use its first
parameter rel. We can safely remove the useless parameter.

-  match_restriction_clauses_to_index(RelOptInfo *rel, IndexOptInfo *index,
-                                     IndexClauseSet *clauseset)
+  match_restriction_clauses_to_index(IndexOptInfo *index,
+                                     IndexClauseSet *clauseset)

I find no other problem and have no objection to this. May I mark
this "Ready for committer" after fixing them?

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center





pgsql-hackers by date:

Previous
From: Kyotaro HORIGUCHI
Date:
Subject: Re: Support for N synchronous standby servers - take 2
Next
From: Tomas Vondra
Date:
Subject: Re: PATCH: index-only scans with partial indexes