Re: indxpath.c's references to IndexOptInfo.ncolumns are all wrong, no?

Peter Geoghegan <pg(at)bowt(dot)ie> writes:
> On Sun, Feb 10, 2019 at 5:18 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Apparently, whoever went through indxpath.c to substitute nkeycolumns
>> for ncolumns was not paying attention. As far as I can tell, the
>> *only* place in there where it's correct to reference ncolumns is in
>> check_index_only, where we determine which columns can be extracted from
>> an index-only scan.

> Ugh. Yeah, it's rather inconsistent.

Looking closer, it seems like most of these are accidentally protected
by the fact that match_clause_to_index stops at nkeycolumns, so that
the indexclauses lists for later columns can never become nonempty;
they're merely wasting cycles by iterating over later columns, rather
than doing anything seriously wrong. It would be possible to get
match_pathkeys_to_index to trigger the assertion in
match_clause_to_ordering_op if GIST supported included columns,
but it doesn't. And I think relation_has_unique_index_for can be
fooled into reporting that an index doesn't prove uniqueness, when
it does. But the only one of these that's really got obviously bad
consequences is the one in expand_indexqual_rowcompare, which triggers
the failure I showed before. It's also the most obviously wrong code:

/*
* The Var side can match any column of the index.
*/
for (i = 0; i < index->nkeycolumns; i++)
{
if (...)
break;
}
if (i >= index->ncolumns)
break; /* no match found */

Even without understanding the bigger picture, any C programmer ought to
find that loop logic pretty fishy. (I'm a bit surprised Coverity hasn't
whined about it.)

Maybe the right compromise is to fix expand_indexqual_rowcompare
now and leave the rest for post-wrap.

regards, tom lane