Re: PATCH: index-only scans with partial indexes

Hi,

On 02/25/2016 11:56 AM, Kyotaro HORIGUCHI wrote:
> Hello, Tomas. my cerebral cortext gets squeezed by jumping from
> parser to planner.

LOL

>
> At Wed, 24 Feb 2016 01:13:22 +0100, Tomas Vondra <tomas(dot)vondra(at)2ndquadrant(dot)com> wrote in <56CCF5A2(dot)5040702(at)2ndquadrant(dot)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?

I'm not sure what this refers to? You mean the fact that an index will
have one IndexOptInfo instance for each baserel? Yes, that seems fine to me.

>
>> 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?

OK. Do you want me to do the fixes, or will you do that?

regards

--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services