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