Обсуждение: indxpath.c's references to IndexOptInfo.ncolumns are all wrong, no?

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.  A lot of the other places are obviously wrong, eg in
relation_has_unique_index_for:

        for (c = 0; c < ind->ncolumns; c++)
        ...
                if (!list_member_oid(rinfo->mergeopfamilies, ind->opfamily[c]))

Even if it were plausible that an INCLUDE column is something to consider
when deciding whether the index enforces uniqueness, this code accesses
beyond the documented end of the opfamily[] array :-(

The fact that the regression tests haven't caught this doesn't give
me a warm feeling about how thoroughly the included-columns logic has
been tested.  It's really easy to make it fall over, for instance

regression=# explain select * from tenk1 where (thousand, tenthous) < (10,100);
                                     QUERY PLAN

--------------------------------------------------------------------------------
-----
 Bitmap Heap Scan on tenk1  (cost=5.11..233.86 rows=107 width=244)
   Recheck Cond: (ROW(thousand, tenthous) < ROW(10, 100))
   ->  Bitmap Index Scan on tenk1_thous_tenthous  (cost=0.00..5.09 rows=107 widt
h=0)
         Index Cond: (ROW(thousand, tenthous) < ROW(10, 100))
(4 rows)

regression=# drop index tenk1_thous_tenthous;
DROP INDEX
regression=# create index on tenk1(thousand) include (tenthous);
CREATE INDEX
regression=# explain select * from tenk1 where (thousand, tenthous) < (10,100);
ERROR:  operator 97 is not a member of opfamily 2139062142

I've got mixed feelings about whether to try to fix this before
tomorrow's wraps.  The attached patch seems correct and passes
check-world, but there's sure not a lot of margin for error now.
Thoughts?

            regards, tom lane

diff --git a/src/backend/optimizer/path/indxpath.c b/src/backend/optimizer/path/indxpath.c
index 51d2da5..a55b9e0 100644
--- a/src/backend/optimizer/path/indxpath.c
+++ b/src/backend/optimizer/path/indxpath.c
@@ -258,7 +258,7 @@ create_index_paths(PlannerInfo *root, RelOptInfo *rel)
         IndexOptInfo *index = (IndexOptInfo *) lfirst(lc);

         /* Protect limited-size array in IndexClauseSets */
-        Assert(index->ncolumns <= INDEX_MAX_KEYS);
+        Assert(index->nkeycolumns <= INDEX_MAX_KEYS);

         /*
          * Ignore partial indexes that do not match the query.
@@ -473,7 +473,7 @@ consider_index_join_clauses(PlannerInfo *root, RelOptInfo *rel,
      * relation itself is also included in the relids set.  considered_relids
      * lists all relids sets we've already tried.
      */
-    for (indexcol = 0; indexcol < index->ncolumns; indexcol++)
+    for (indexcol = 0; indexcol < index->nkeycolumns; indexcol++)
     {
         /* Consider each applicable simple join clause */
         considered_clauses += list_length(jclauseset->indexclauses[indexcol]);
@@ -628,7 +628,7 @@ get_join_index_paths(PlannerInfo *root, RelOptInfo *rel,
     /* Identify indexclauses usable with this relids set */
     MemSet(&clauseset, 0, sizeof(clauseset));

-    for (indexcol = 0; indexcol < index->ncolumns; indexcol++)
+    for (indexcol = 0; indexcol < index->nkeycolumns; indexcol++)
     {
         ListCell   *lc;

@@ -925,7 +925,7 @@ build_index_paths(PlannerInfo *root, RelOptInfo *rel,
     index_clauses = NIL;
     found_lower_saop_clause = false;
     outer_relids = bms_copy(rel->lateral_relids);
-    for (indexcol = 0; indexcol < index->ncolumns; indexcol++)
+    for (indexcol = 0; indexcol < index->nkeycolumns; indexcol++)
     {
         ListCell   *lc;

@@ -2680,7 +2680,7 @@ match_pathkeys_to_index(IndexOptInfo *index, List *pathkeys,
              * amcanorderbyop.  We might need different logic in future for
              * other implementations.
              */
-            for (indexcol = 0; indexcol < index->ncolumns; indexcol++)
+            for (indexcol = 0; indexcol < index->nkeycolumns; indexcol++)
             {
                 Expr       *expr;

@@ -3111,7 +3111,7 @@ relation_has_unique_index_for(PlannerInfo *root, RelOptInfo *rel,
          * Try to find each index column in the lists of conditions.  This is
          * O(N^2) or worse, but we expect all the lists to be short.
          */
-        for (c = 0; c < ind->ncolumns; c++)
+        for (c = 0; c < ind->nkeycolumns; c++)
         {
             bool        matched = false;
             ListCell   *lc;
@@ -3187,7 +3187,7 @@ relation_has_unique_index_for(PlannerInfo *root, RelOptInfo *rel,
         }

         /* Matched all columns of this index? */
-        if (c == ind->ncolumns)
+        if (c == ind->nkeycolumns)
             return true;
     }

@@ -3991,7 +3991,7 @@ expand_indexqual_rowcompare(RestrictInfo *rinfo,

                 break;
         }
-        if (i >= index->ncolumns)
+        if (i >= index->nkeycolumns)
             break;                /* no match found */

         /* Add column number to returned list */

On Sun, Feb 10, 2019 at 5:18 PM Tom Lane <tgl@sss.pgh.pa.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.

> I've got mixed feelings about whether to try to fix this before
> tomorrow's wraps.  The attached patch seems correct and passes
> check-world, but there's sure not a lot of margin for error now.
> Thoughts?

I think that it should be fixed in the next point release if at all
possible. The bug is a simple omission. I have a hard time imagining
how your patch could possibly destabilize things, since nkeycolumns is
already used in numerous other places in indxpath.c.

-- 
Peter Geoghegan

Peter Geoghegan <pg@bowt.ie> writes:
> On Sun, Feb 10, 2019 at 5:18 PM Tom Lane <tgl@sss.pgh.pa.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