From: Tom Lane <tgl@sss.pgh.pa.us>
To: pgsql-hackers@lists.postgresql.org
Subject: indxpath.c's references to IndexOptInfo.ncolumns are all wrong, no?
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0"
Content-ID: <25364.1549847701.0@sss.pgh.pa.us>
Date: Sun, 10 Feb 2019 20:18:48 -0500
Message-ID: <25526.1549847928@sss.pgh.pa.us>
Precedence: bulk

------- =_aaaaaaaaaa0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <25364.1549847701.1@sss.pgh.pa.us>
Content-Transfer-Encoding: quoted-printable

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 =3D 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=3D# explain select * from tenk1 where (thousand, tenthous) < (1=
0,100);
                                     QUERY PLAN                           =
      =

     =

--------------------------------------------------------------------------=
------
-----
 Bitmap Heap Scan on tenk1  (cost=3D5.11..233.86 rows=3D107 width=3D244)
   Recheck Cond: (ROW(thousand, tenthous) < ROW(10, 100))
   ->  Bitmap Index Scan on tenk1_thous_tenthous  (cost=3D0.00..5.09 rows=3D=
107 widt
h=3D0)
         Index Cond: (ROW(thousand, tenthous) < ROW(10, 100))
(4 rows)

regression=3D# drop index tenk1_thous_tenthous;
DROP INDEX
regression=3D# create index on tenk1(thousand) include (tenthous);
CREATE INDEX
regression=3D# explain select * from tenk1 where (thousand, tenthous) < (1=
0,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


------- =_aaaaaaaaaa0
Content-Type: text/x-diff; name="ignore-included-columns-in-indxpath-c.patch";
	charset="us-ascii"
Content-ID: <25364.1549847701.2@sss.pgh.pa.us>
Content-Description: ignore-included-columns-in-indxpath-c.patch
Content-Transfer-Encoding: quoted-printable

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 =3D (IndexOptInfo *) lfirst(lc);
 =

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

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

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

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

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

@@ -3111,7 +3111,7 @@ relation_has_unique_index_for(PlannerInfo *root, Rel=
OptInfo *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 =3D 0; c < ind->ncolumns; c++)
+		for (c =3D 0; c < ind->nkeycolumns; c++)
 		{
 			bool		matched =3D false;
 			ListCell   *lc;
@@ -3187,7 +3187,7 @@ relation_has_unique_index_for(PlannerInfo *root, Rel=
OptInfo *rel,
 		}
 =

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

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

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

 		/* Add column number to returned list */

------- =_aaaaaaaaaa0--