rfc: add rfc for invisible index feature

[wenyihu6]

This commit adds an RFC for the invisible index feature.

Related issue: #72576, #82363

Release justification: low risk to the existing functionality; this commit just
adds rfc.

Release Note: none

[cockroach-teamcity]

This change is 

[wenyihu6]

I noticed that Oliver did a lot of pr review related for invisible column feature, so I reached out to him for some context here.

so NOT VISIBLE was actually added by an open source contributor! #58923
I think we were originally contemplating HIDDEN, but decided with NOT VISIBLE because of some keywording issues: #53428 (comment)

[otan]

hmm, i wonder if we should change is_hidden to not_visible to be consistent (this is probably my bad when i added the feature).

[otan]

As mentioned here it has to become a reserved keyword, which is why we went with NOT VISIBLE. Adding INVISIBLE to reserved keywords is potentially problematic as people who have defined columns like INVISIBLE bool now have to quote it, which makes it backwards incompatible.

[otan]

i kind of wonder whether INVISIBLE / NOT VISIBLE is the right way to "express" this. My understanding is you still have to pay the write penalty of the indexes, it just doesn't get "used" (which also makes me wonder whether this feature is useful in the ADD case - see my motivation comment above). NOT USED sounds a bit better semantically to me. but other databases have made it INVISIBLE so maybe we're ok.

this is not a blocking comment and i can easily be convinced otherwise.

[otan]

Hmm, not sure whether this fully solves this portion of the motivation. You still need to do a backfill to introduce the index, and then pay the price on every write. I do buy that it does let you experiment with seeing if READ latencies improve with the index though (just wonder if it's worth the write cost!). Am I missing something?

[wenyihu6]

I believe you are right. We are just marking the index as invisible for queries like SELECT. The indexes are maintained up to date with every writes.

[mgartner]

Any time the set of indexes changes, query plans can change too. It's easy to imagine cases where dropping an index negatively affects query plans. It's harder to imagine cases where adding an index leads to worse query plans, but it's possible. This paragraph can be more clear that this RFC is solving for the impact of adding an index on query plans, not the impact on index backfills and maintenance during mutations.

[mgartner]

We'll need to consider how foreign keys constraints interact with unique invisible indexes.

You need a unique constraint or index on the reference column to create a foreign key:

defaultdb> CREATE TABLE p (id INT PRIMARY KEY, a INT);
CREATE TABLE

defaultdb> CREATE TABLE c (id INT PRIMARY KEY, p_a INT REFERENCES p(a));
ERROR: there is no unique constraint matching given keys for referenced table p
SQLSTATE: 23503

defaultdb> CREATE UNIQUE INDEX p_a_key ON p(a);
CREATE INDEX

defaultdb> CREATE TABLE c (id INT PRIMARY KEY, p_a INT REFERENCES p(a));
CREATE TABLE

This is a requirement to make foreign key checks efficient. For example, when inserting into c we must verify that the new value for p_a exists in column a of table p. Without an index, searching for that value in p would be expensive. You can see in the query plan of an INSERT that p_a_key is used in the anti-join (lookup p@p_a_key) within f-k-check.

defaultdb> EXPLAIN (OPT, VERBOSE) INSERT INTO c VALUES (1, 10);
                                        info
------------------------------------------------------------------------------------
  insert c
   ├── columns: <none>
   ├── insert-mapping:
   │    ├── column1:5 => c.id:1
   │    └── column2:6 => c.p_a:2
   ├── input binding: &1
   ├── cardinality: [0 - 0]
   ├── volatile, mutations
   ├── stats: [rows=0]
   ├── cost: 6.1179112
   ├── distribution: us-east1
   ├── values
   │    ├── columns: column1:5 column2:6
   │    ├── cardinality: [1 - 1]
   │    ├── stats: [rows=1, distinct(6)=1, null(6)=0, avgsize(6)=4]
   │    ├── cost: 0.02
   │    ├── key: ()
   │    ├── fd: ()-->(5,6)
   │    ├── distribution: us-east1
   │    ├── prune: (5,6)
   │    └── (1, 10)
   └── f-k-checks
        └── f-k-checks-item: c(p_a) -> p(a)
             └── anti-join (lookup p@p_a_key)
                  ├── columns: p_a:7
                  ├── key columns: [7] = [9]
                  ├── lookup columns are key
                  ├── cardinality: [0 - 1]
                  ├── stats: [rows=1e-10]
                  ├── cost: 6.0879112
                  ├── key: ()
                  ├── fd: ()-->(7)
                  ├── cte-uses
                  │    └── &1: count=1 used-columns=(6)
                  ├── with-scan &1
                  │    ├── columns: p_a:7
                  │    ├── mapping:
                  │    │    └──  column2:6 => p_a:7
                  │    ├── cardinality: [1 - 1]
                  │    ├── stats: [rows=1, distinct(7)=1, null(7)=0, avgsize(7)=4]
                  │    ├── cost: 0.01
                  │    ├── key: ()
                  │    ├── fd: ()-->(7)
                  │    └── cte-uses
                  │         └── &1: count=1 used-columns=(6)
                  └── filters (true)

So what happens if we run ALTER INDEX p_a_key NOT VISIBLE? Does the insert continue to read from the invisible p_a_key so that the fk check is efficient? Does the insert stop reading from p_a_key so that the fk check is inefficient? Or does the ALTER INDEX statement error because a FK depends on that index (and can we even tell if a FK depends on a particular index - multiple overlapping unique indexes that support an FK could exist)?

[wenyihu6]

I was thinking unique constraint is imposed regardless of whether the index is invisible, and insert can still read from the unique index. The index is ignored just for queries like SELECT.

[mgartner]

I agree the unique constraint must be imposed because invisible indexes are maintained just like visible indexes. But should they be read for FK checks? Probably, because the alternative is full table scan which would cause abysmal INSERT performance. But we should call out this case, and other like it, because it'll be tricky to get right in the implementation and it'll require thoughtful tests.

And I'm skeptical it's as simple as ignoring only for SELECT queries. Secondary indexes are read from in UPDATE and DELETE statements too. For example:

defaultdb> CREATE TABLE t (k INT PRIMARY KEY, a INT, INDEX (a));
CREATE TABLE


defaultdb> EXPLAIN (OPT, VERBOSE) UPDATE t SET a = 2 WHERE a = 1;
                                   info
---------------------------------------------------------------------------
  update t
   ├── columns: <none>
   ├── fetch columns: k:5 a:6
   ├── update-mapping:
   │    └── a_new:9 => a:2
   ├── cardinality: [0 - 0]
   ├── volatile, mutations
   ├── stats: [rows=0]
   ├── cost: 25.0500001
   ├── distribution: us-east1
   └── project
        ├── columns: a_new:9 k:5 a:6
        ├── stats: [rows=10]
        ├── cost: 25.0400001
        ├── key: (5)
        ├── fd: ()-->(6,9)
        ├── distribution: us-east1
        ├── prune: (5,6,9)
        ├── scan t@t_a_idx
        │    ├── columns: k:5 a:6
        │    ├── constraint: /6/5: [/1 - /1]
        │    ├── stats: [rows=10, distinct(6)=1, null(6)=0, avgsize(6)=4]
        │    ├── cost: 24.8200001
        │    ├── key: (5)
        │    ├── fd: ()-->(6)
        │    └── distribution: us-east1
        └── projections
             └── 2 [as=a_new:9]
(28 rows)

And what about cases where there is a SELECT subquery in an INSERT, UPDATE, DELETE, or UPSERT statement?

[michae2]

[Copying my Reviewable comment from below] because these FK indexes can't be dropped without the CASCADE option, I think we should disallow making them invisible. (Too hard to add a CASCADE option to ALTER INDEX SET NOT VISIBLE.)

But in general, since our goal is to demonstrate the consequences of dropping an index, I don't think we should be scared of causing full table scans. After all, dropping an index likely leads to full table scans as well.

[michae2]

Thanks for opening this!

It might help to add a short subsection to Technical design explaining what will happen when an index is invisible, and calling out the restrictions (e.g. no invisible primary indexes).

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @devadvocado, @knz, @mgartner, @otan, @postamar, @rhu713, @vy-ton, and @wenyihu6)

---

docs/RFCS/20220628_invisible_index.md line 176 at r1 (raw file):

Previously, otan (Oliver Tan) wrote…

hmm, i wonder if we should change is_hidden to not_visible to be consistent (this is probably my bad when i added the feature).

Can we change it now that SHOW COLUMNS has been released in the wild? Seems like customers could have written queries using the name, e.g. SELECT is_hidden FROM [SHOW COLUMNS FROM p].

---

docs/RFCS/20220628_invisible_index.md line 187 at r1 (raw file):

Previously, otan (Oliver Tan) wrote…

i kind of wonder whether INVISIBLE / NOT VISIBLE is the right way to "express" this. My understanding is you still have to pay the write penalty of the indexes, it just doesn't get "used" (which also makes me wonder whether this feature is useful in the ADD case - see my motivation comment above). NOT USED sounds a bit better semantically to me. but other databases have made it INVISIBLE so maybe we're ok.

this is not a blocking comment and i can easily be convinced otherwise.

Oracle distinguishes between "unusable" indexes (not maintained and not used by reads) and "invisible" indexes (maintained but not used by reads). I think for better or for worse these have become terms of art, and we should follow them, at least somewhat.

---

docs/RFCS/20220628_invisible_index.md line 106 at r2 (raw file):

Previously, wenyihu6 (Wenyi Hu) wrote…

I was thinking unique constraint is imposed regardless of whether the index is invisible, and insert can still read from the unique index. The index is ignored just for queries like SELECT.

I guess we disallow dropping these indexes (unless the CASCADE option is used, which then also drops the FK reference in the other table):

[email protected]:26257/defaultdb> DROP INDEX p_a_key;
ERROR: "p_a_key" is referenced by foreign key from table "c"

[email protected]:26257/defaultdb> DROP INDEX p_a_key CASCADE;
DROP INDEX

[email protected]:26257/defaultdb> SHOW CREATE TABLE p;
  table_name |              create_statement
-------------+---------------------------------------------
  p          | CREATE TABLE public.p (
             |     id INT8 NOT NULL,
             |     a INT8 NULL,
             |     CONSTRAINT p_pkey PRIMARY KEY (id ASC)
             | )
(1 row)

[email protected]:26257/defaultdb> SHOW CREATE TABLE c;
  table_name |              create_statement
-------------+---------------------------------------------
  c          | CREATE TABLE public.c (
             |     id INT8 NOT NULL,
             |     p_a INT8 NULL,
             |     CONSTRAINT c_pkey PRIMARY KEY (id ASC)
             | )
(1 row)

I don't think we would want to implement a CASCADE option for invisible indexes, so I propose disallowing making these FK indexes invisible as well.

---

docs/RFCS/20220628_invisible_index.md line 18 at r3 (raw file):

its initialization. As for now, primary indexes cannot be invisible. But unique 
indexes can still be invisible. Specifically, the unique constraint still prevents 
insertion of duplicates into a column regardless of whether the inedx is invisible.

nit: "index" instead of "inedx"

---

docs/RFCS/20220628_invisible_index.md line 49 at r3 (raw file):

This following section will discuss different SQL syntax choices. PostgreSQL
does not support invisible indexes yet. We will be using MySQL and Oracle SQL as

nit: It would be good to link to MySQL docs and Oracle docs here.

---

docs/RFCS/20220628_invisible_index.md line 66 at r3 (raw file):

        [PARTITION BY <partition params>]
        [WITH <storage_parameter_list>] [WHERE <where_conds...>]
        *** [INVISIBLE | NOT VISIBLE | VISIBLE | HIDDEN] *** 

It would be nice to be compatible with MySQL and Oracle, but I guess we don't really promise that in general. Being internally consistent with our own ALTER COLUMN SET NOT VISIBLE / SET VISIBLE seems more important.

[yuzefovich]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @mgartner, @michae2, @otan, and @wenyihu6)

---

docs/RFCS/20220628_invisible_index.md line 29 at r6 (raw file):

With invisible indexes, you can introduce the index as invisible first. In a new
session, you could give a workout and observe the impact of the new index by
turning `optimizer_use_invisible_indexes` on or with index hinting. If this

nit: this seems to have been renamed to optimizer_use_not_visible_indexes.

[wenyihu6]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @mgartner, @michae2, @otan, and @wenyihu6)

docs/RFCS/20220628_invisible_index.md line 29 at r6 (raw file):

With invisible indexes, you can introduce the index as invisible first. In a new
session, you could give a workout and observe the impact of the new index by
turning `optimizer_use_invisible_indexes` on or with index hinting. If this

nit: this seems to have been renamed to optimizer_use_not_visible_indexes.

You are right! Thank you!

[yuzefovich]

Nice work! Some nits from me that I noticed when reading through the RFC.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @mgartner, @michae2, @otan, @wenyihu6, and @yuzefovich)

---

docs/RFCS/20220628_invisible_index.md line 84 at r6 (raw file):

- Force index or index hinting with invisible index is allowed and will override the invisible index feature.
  - If the index is dropped instead, this will throw an error.
- Invisible indexes will be treated as while policing unique or foreign key constraints. In other words, we will temporarily disable the invisible index feature during any constraint check.

nit: probably s/treated as while/treated as visible while/.

---

docs/RFCS/20220628_invisible_index.md line 141 at r6 (raw file):

[errors](https://dev.mysql.com/doc/refman/8.0/en/invisible-indexes.html).
Instead, MySQL supports index hinting with the session variable
[optimizer_use_not_visible_indexes](https://dev.mysql.com/doc/refman/8.0/en/switchable-optimizations.html#optflag_use-invisible-indexes)

nit: missing period.

---

docs/RFCS/20220628_invisible_index.md line 228 at r6 (raw file):

feature and focus on how the optimizer will ignore invisible indexes in general.

During exploration, the optimizer will explore every possible query plans using

nit: s/plans/plan/.

---

docs/RFCS/20220628_invisible_index.md line 233 at r6 (raw file):

`pkg/sql/opt/xform/scan_index_iter.go`. This is where we can hide the index away
from the optimizer. While enumerating every index, the optimizer can check if
the index is invisible and ignore if it is. optimizer is effectively ignoring

nit: s/optimizer/The optimizer/.

---

docs/RFCS/20220628_invisible_index.md line 239 at r6 (raw file):

Second, let’s think about what happens when force index is used with invisible
index. Force index will override the invisible index feature. We will just need
to check if the flag for force index is set before ignore invisible indexes

nit: s/before ignore/before ignoring/

---

docs/RFCS/20220628_invisible_index.md line 427 at r6 (raw file):

   Related: https://github.com/cockroachdb/cockroach/issues/82363

3. e can consider introducing another session variable or another type of

nit: s/e can/We can/

[wenyihu6]

Nice work! Some nits from me that I noticed when reading through the RFC.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @mgartner, @michae2, @otan, @wenyihu6, and @yuzefovich)

docs/RFCS/20220628_invisible_index.md line 84 at r6 (raw file):

- Force index or index hinting with invisible index is allowed and will override the invisible index feature.
  - If the index is dropped instead, this will throw an error.
- Invisible indexes will be treated as while policing unique or foreign key constraints. In other words, we will temporarily disable the invisible index feature during any constraint check.

nit: probably s/treated as while/treated as visible while/.

docs/RFCS/20220628_invisible_index.md line 141 at r6 (raw file):

[errors](https://dev.mysql.com/doc/refman/8.0/en/invisible-indexes.html).
Instead, MySQL supports index hinting with the session variable
[optimizer_use_not_visible_indexes](https://dev.mysql.com/doc/refman/8.0/en/switchable-optimizations.html#optflag_use-invisible-indexes)

nit: missing period.

docs/RFCS/20220628_invisible_index.md line 228 at r6 (raw file):

feature and focus on how the optimizer will ignore invisible indexes in general.

During exploration, the optimizer will explore every possible query plans using

nit: s/plans/plan/.

docs/RFCS/20220628_invisible_index.md line 233 at r6 (raw file):

`pkg/sql/opt/xform/scan_index_iter.go`. This is where we can hide the index away
from the optimizer. While enumerating every index, the optimizer can check if
the index is invisible and ignore if it is. optimizer is effectively ignoring

nit: s/optimizer/The optimizer/.

docs/RFCS/20220628_invisible_index.md line 239 at r6 (raw file):

Second, let’s think about what happens when force index is used with invisible
index. Force index will override the invisible index feature. We will just need
to check if the flag for force index is set before ignore invisible indexes

nit: s/before ignore/before ignoring/

docs/RFCS/20220628_invisible_index.md line 427 at r6 (raw file):

   Related: https://github.com/cockroachdb/cockroach/issues/82363

3. e can consider introducing another session variable or another type of

nit: s/e can/We can/

Thanks for looking at this pr as part of the QA!

[yuzefovich]

Should the invisible indexes still influence the decision which columns we collect statistics on? I believe so, and I think we can get this behavior already but thought it'd be worth to confirm with folks. (For context, we collect stats on all columns in the secondary indexes which is determined in createStatsDefaultColumns in sql/create_stats.go.)

[wenyihu6]

Should the invisible indexes still influence the decision which columns we collect statistics on? I believe so, and I think we can get this behavior already but thought it'd be worth to confirm with folks. (For context, we collect stats on all columns in the secondary indexes which is determined in createStatsDefaultColumns in sql/create_stats.go.)

I think so. Stats are collected on invisible indexes as well. And this aligns with what MySQL and Oracle do. This is not officially documented on their websites; the link for Oracle was found on a blog post. I played around with MySQL(see screenshot below); stats from invisible indexes are still collected to determine cardinality estimates.

[wenyihu6]

TFTRs!

bors r+

[craig]

Build failed (retrying...):

• Bazel Essential CI (Cockroach)

[craig]

Build failed (retrying...):

• Bazel Essential CI (Cockroach)

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)

[irfansharif]

nice work!

Reviewable status: complete! 1 of 0 LGTMs obtained

[itsbilal]

[adityamaru]

We'll miss you!

Reviewable status: complete! 3 of 0 LGTMs obtained

[renatolabs]

Reviewable status: complete! 4 of 0 LGTMs obtained

[arulajmani]

Reviewable status: complete! 5 of 0 LGTMs obtained

[fqazi]

, everyone is gonna miss you!

In retrospect, we should take coolness into account when naming a feature, we should have called them invisible or translucent indexes 🙃