Re: PATCH: index-only scans with partial indexes

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 Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services