Re: PATCH: index-only scans with partial indexes

On Mon, Dec 7, 2015 at 7:48 AM, Tomas Vondra
<tomas(dot)vondra(at)2ndquadrant(dot)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