cockroach icon indicating copy to clipboard operation
cockroach copied to clipboard

sql: add not visible index to optimizer

Open wenyihu6 opened this issue 2 years ago • 5 comments

This commit adds the logic of the invisible index feature to the optimizer. After this commit has been merged, the invisible index feature should be fully functional with CREATE INDEX and CREATE TABLE.

Assists: https://github.com/cockroachdb/cockroach/issues/72576

See also: https://github.com/cockroachdb/cockroach/pull/85239

Release note (sql change): creating a not visible index using CREATE TABLE …(INDEX … NOT VISIBLE) or CREATE INDEX … NOT VISIBLE is now supported.

wenyihu6 avatar Aug 09 '22 01:08 wenyihu6

This change is Reviewable

cockroach-teamcity avatar Aug 09 '22 01:08 cockroach-teamcity

I split the other PR https://github.com/cockroachdb/cockroach/pull/85354. This PR contains changes in the optimizer, and the other PR only has notice related code.

wenyihu6 avatar Aug 09 '22 01:08 wenyihu6

pkg/sql/opt/exec/execbuilder/testdata/not_visible_index line 192 at r1 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

nit: add a test just like this with the index forced - you already have a similar tests for partial indexes below, but I think it'd be nice to have one for non-partial indexes here.

I think there is a case like this below (the first case under force index) https://github.com/cockroachdb/cockroach/blob/4a2af4e2aa538cca07e92b9f768141030afd7b0b/pkg/sql/opt/exec/execbuilder/testdata/not_visible_index#L347

wenyihu6 avatar Aug 11 '22 02:08 wenyihu6

pkg/sql/opt/exec/execbuilder/testdata/not_visible_index line 391 at r1 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

I'm not sure I understand the motivation for testing based on the sources. These functions should end up calling buildScan in the optbuilder phase. A more interesting test might be testing lookup join, since they also iterate over seconary indexes. For example: https://github.com/cockroachdb/cockroach/blob/14a0ef30a8680f2eeeef3a3f59c816dfaf9cf836/pkg/sql/opt/exec/execbuilder/testdata/lookup_join#L44-L60

Generation of zigzag joins and merge joins also iterate over secondary indexes, so those could be interesting tests too.

My initial intention was that we have covered most of places where buildScan was called now. buildDataSource was the only function where some cases were not covered like (buildJoin, buildFromWithLateral). I've removed these cases and added more cases to cover join (merge join, lookup join, cross join, inverted join) and one case for lateral subquery

wenyihu6 avatar Aug 11 '22 02:08 wenyihu6

pkg/sql/opt/exec/execbuilder/testdata/not_visible_index line 793 at r1 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

There's a delete cascade fast path that we should test here. Notice the slight difference in the two query plans below. The first's fk-cascade does not scan buffer 1:

defaultdb> create table p (p int primary key, i int);
CREATE TABLE

defaultdb> create table c (c int primary key, p int references
p(p) on delete cascade);
CREATE TABLE

defaultdb> explain delete from p where p = 1;
           info
--------------------------
  distribution: local
  vectorized: true

  • root
  │
  ├── • delete range
  │     from: p
  │     spans: [/1 - /1]
  │
  └── • fk-cascade
        fk: c_p_fkey
(11 rows)

defaultdb> explain delete from p where p = 1 and i = 2;
                   info
------------------------------------------
  distribution: local
  vectorized: true

  • root
  │
  ├── • delete
  │   │ from: p
  │   │
  │   └── • buffer
  │       │ label: buffer 1
  │       │
  │       └── • filter
  │           │ filter: i = 2
  │           │
  │           └── • scan
  │                 missing stats
  │                 table: p@p_pkey
  │                 spans: [/1 - /1]
  │
  └── • fk-cascade
        fk: c_p_fkey
        input: buffer 1

I think you'll need to run this delete before adding the child_update table to get the delete fast path to trigger.

I think this case is triggering fast path because the filter on p got transferred to the cascade constraint. I added another case to cover when fast path is not triggered.

wenyihu6 avatar Aug 11 '22 02:08 wenyihu6

TFTRs!

bors r+

wenyihu6 avatar Aug 12 '22 16:08 wenyihu6

retrying ...

bors r+

wenyihu6 avatar Aug 12 '22 23:08 wenyihu6

Build failed (retrying...):

craig[bot] avatar Aug 13 '22 01:08 craig[bot]

Build succeeded:

craig[bot] avatar Aug 13 '22 04:08 craig[bot]