Re: PATCH: index-only scans with partial indexes

On Mon, Dec 7, 2015 at 7:48 AM, Tomas Vondra
<tomas.vondra@2ndquadrant.com> wrote:
> Hello Kyotaro-san,
>
> Sorry for the long delay since your response in this thread :-(
>
> On 10/14/2015 08:06 AM, Kyotaro HORIGUCHI wrote:
>>
>>
>> The table t is referred to twice by different (alias) names (though
>> the diferrence is made by EXPLAIN, it shows that they are different
>> rels in plantree).
>>
>>> So we'll have these indexrinfos:
>>>
>>> aidx: {b=2}
>>> bidx: {a=1}
>>
>>
>> Yes, but each of them belongs to different rels. So,
>>
>>> which makes index only scans unusable.
>>
>>
>> The are usable.
>
>
> Yes, you're of course right. I got confused by the comment at IndexOptInfo
> that says "per-index information" but as you've pointed out it's really
> "per-index per-reloptinfo" which makes it perfectly suitable for keeping the
> info we need.
>
> However, I'm not sure the changes to check_partial_indexes() are correct -
> particularly the first loop.
>
>   /*
>    * 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;
>   }
>
> Now, imagine we have two indexes - partial and regular. In such case the
> loop will terminate after the first index (assuming predOK=true), so the
> regular index won't have indrinfos set. I think we need to remove the
> 'break' at the end.
>
> BTW it took me a while to understand the change at the end:
>
>   /* Search for the first rinfo that is implied by indpred */
>   foreach (lcr, rel->baserestrictinfo)
>   {
>     RestrictInfo *rinfo = (RestrictInfo *) lfirst(lcr);
>
>     if (predicate_implied_by(list_make1(rinfo->clause),
>                  index->indpred))
>       break;
>   }
>
>   /* This index needs rinfos different from baserestrictinfo */
>   if (lcr)
>   {
>     ... filter implied conditions ...
>   }
>
> Is there a reason why not to simply move the second block into the if block
> in the foreach loop like this?
>
>   /* Search for the first rinfo that is implied by indpred */
>   foreach (lcr, rel->baserestrictinfo)
>   {
>     RestrictInfo *rinfo = (RestrictInfo *) lfirst(lcr);
>
>     if (predicate_implied_by(list_make1(rinfo->clause),
>                  index->indpred))
>     {
>       ... filter implied conditions ...
>       break;
>     }
>   }
>
>
> Otherwise the reworked patch seems fine to me, but I'll give it a bit more
> testing over the next few days.
>
> Thanks for the help so far!

Tomas, are you still working on that? This thread is stalling for 3 weeks.
-- 
Michael