sql: add DisableNotVisibleIndex to ScanFlags#85239
sql: add DisableNotVisibleIndex to ScanFlags#85239craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
|
This change is |
15c7aba to
dd1b5ae
Compare
|
The first commit comes from #84912. |
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: cockroachdb#72576 See also: cockroachdb#85239 Release note (sql change): creating a not visible index using `CREATE TABLE … (INDEX … NOT VISIBLE)` or `CREATE INDEX … NOT VISIBLE` is now supported.
266afe2 to
ba68a0b
Compare
9ea2b12 to
2904adc
Compare
mgartner
left a comment
There was a problem hiding this comment.
Reviewed 8 of 32 files at r7, 3 of 3 files at r10, all commit messages.
Reviewable status:complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @mgartner, @rhu713, and @wenyihu6)
pkg/sql/opt/memo/expr.go line 700 at r10 (raw file):
// IsNotVisibleIndex returns true if the index being scanned is a not visible // index. func (s *ScanPrivate) IsNotVisibleIndex(md *opt.Metadata) bool {
nit: IsNotVisibleIndexScan
pkg/sql/opt/memo/expr_format.go line 465 at r5 (raw file):
Previously, michae2 (Michael Erickson) wrote…
Thank you!
👍
pkg/sql/opt/optbuilder/testdata/fk-on-update-cascade line 1575 at r6 (raw file):
│ │ ├── scan t.public.child_diff_type │ │ │ ├── columns: t.public.child_diff_type.c:12(int!null) t.public.child_diff_type.p:13(int2) │ │ │ ├── flags: disabled not visible index feature
Isn't this a case where we expect the flag to be set?
pkg/sql/opt/optbuilder/testdata/fk-on-update-cascade line 1626 at r6 (raw file):
├── scan t.public.parent_diff_type │ ├── columns: t.public.parent_diff_type.p:20(int!null) │ ├── flags: disabled not visible index feature
Shouldn't the flag be set here too?
michae2
left a comment
There was a problem hiding this comment.
Reviewed all commit messages.
Reviewable status:
pkg/sql/opt/optbuilder/testdata/fk-on-update-cascade line 1575 at r6 (raw file):
Previously, mgartner (Marcus Gartner) wrote…
Isn't this a case where we expect the flag to be set?
Wow, good catch!
After adding that same case to pkg/sql/opt/optbuilder/testdata/fk-on-delete-cascade it looks like the flag is also missing for the scan of the child in the cascaded delete:
+
+# Test cascade to a child with a reference column of a different type.
+exec-ddl
+CREATE TABLE parent_diff_type (p INT PRIMARY KEY)
+----
+
+exec-ddl
+CREATE TABLE child_diff_type (
+ c INT PRIMARY KEY,
+ p INT2 REFERENCES parent_diff_type(p) ON DELETE CASCADE
+)
+----
+
+build-cascades format=show-all
+DELETE FROM parent_diff_type WHERE p = 0
+----
+root
+ ├── delete t.public.parent_diff_type
+ │ ├── columns: <none>
+ │ ├── fetch columns: t.public.parent_diff_type.p:4(int)
+ │ ├── cascades
+ │ │ └── child_diff_type_p_fkey
+ │ ├── cardinality: [0 - 0]
+ │ ├── volatile, mutations
+ │ ├── stats: [rows=0]
+ │ └── select
+ │ ├── columns: t.public.parent_diff_type.p:4(int!null) t.public.parent_diff_type.crdb_internal_mvcc_timestamp:5(decimal) t.public.parent_diff_type.tableoid:6(oid)
+ │ ├── cardinality: [0 - 1]
+ │ ├── stats: [rows=1, distinct(4)=1, null(4)=0, avgsize(4)=4]
+ │ ├── key: ()
+ │ ├── fd: ()-->(4-6)
+ │ ├── prune: (5,6)
+ │ ├── scan t.public.parent_diff_type
+ │ │ ├── columns: t.public.parent_diff_type.p:4(int!null) t.public.parent_diff_type.crdb_internal_mvcc_timestamp:5(decimal) t.public.parent_diff_type.tableoid:6(oid)
+ │ │ ├── stats: [rows=1000, distinct(4)=1000, null(4)=0, avgsize(4)=4]
+ │ │ ├── key: (4)
+ │ │ ├── fd: (4)-->(5,6)
+ │ │ └── prune: (4-6)
+ │ └── filters
+ │ └── eq [type=bool, outer=(4), constraints=(/4: [/0 - /0]; tight), fd=()-->(4)]
+ │ ├── variable: t.public.parent_diff_type.p:4 [type=int]
+ │ └── const: 0 [type=int]
+ └── cascade
+ └── delete t.public.child_diff_type
+ ├── columns: <none>
+ ├── fetch columns: t.public.child_diff_type.c:11(int) t.public.child_diff_type.p:12(int2)
+ ├── cardinality: [0 - 0]
+ ├── volatile, mutations
+ ├── stats: [rows=0]
+ └── select
+ ├── columns: t.public.child_diff_type.c:11(int!null) t.public.child_diff_type.p:12(int2!null)
+ ├── stats: [rows=10, distinct(12)=1, null(12)=0, avgsize(12)=4]
+ ├── key: (11)
+ ├── fd: ()-->(12)
+ ├── prune: (11)
+ ├── scan t.public.child_diff_type
+ │ ├── columns: t.public.child_diff_type.c:11(int!null) t.public.child_diff_type.p:12(int2)
+ │ ├── stats: [rows=1000, distinct(11)=1000, null(11)=0, avgsize(11)=4, distinct(12)=100, null(12)=10, avgsize(12)=4]
+ │ ├── key: (11)
+ │ ├── fd: (11)-->(12)
+ │ └── prune: (11,12)
+ └── filters
+ ├── eq [type=bool, outer=(12), constraints=(/12: [/0 - /0]; tight), fd=()-->(12)]
+ │ ├── variable: t.public.child_diff_type.p:12 [type=int2]
+ │ └── const: 0 [type=int]
+ └── is-not [type=bool, outer=(12), constraints=(/12: (/NULL - ]; tight)]
+ ├── variable: t.public.child_diff_type.p:12 [type=int2]
+ └── null [type=int2]
Yeah. I think its because of how we are setting the flag for |
|
Previously, wenyihu6 (Wenyi Hu) wrote…
Michael and I talked about this for a bit and decided that staying with our existing flags is probably better. Initially, I was thinking about introducing show flags and writing the const |
This commit adds a flag to ScanFlags to indicate when the invisible index feature should be enabled in buildScan. The invisible index feature should be enabled by default but temporarily disabled during any unique or foreign key constraint check. More details on how the decision was made and which flag to pass in `docs/RFCS/20220628_invisible_index.md`. Note that no logic has been added to the optimizer yet. This commit just passes the correct flag to buildScan and shows the effect of the flag in the output of test data. Assists: cockroachdb#72576 See also: cockroachdb#85354 Release note: none
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: cockroachdb#72576 See also: cockroachdb#85239 Release note (sql change): creating a not visible index using `CREATE TABLE … (INDEX … NOT VISIBLE)` or `CREATE INDEX … NOT VISIBLE` is now supported.
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: cockroachdb#72576 See also: cockroachdb#85239 Release note (sql change): creating a not visible index using `CREATE TABLE … (INDEX … NOT VISIBLE)` or `CREATE INDEX … NOT VISIBLE` is now supported.
michae2
left a comment
There was a problem hiding this comment.
Reviewed 17 of 18 files at r11, all commit messages.
Reviewable status:
pkg/sql/opt/memo/expr.go line 379 at r11 (raw file):
// table to generate equivalent memo groups. If DisableNotVisibleIndex is // true, optimizer will also generate equivalent memo group using the // invisible index. Otherwise, optimizer will ignore the invisible indexes.
This is a great comment! 💯
michae2
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sql/opt/memo/expr_format.go line 75 at r11 (raw file):
// a flag that show things, ShowAll = 0000..000 would actually turn this flag // off. Thus, in order for ExprFmtShowAll and ExprFmtHideAll to be correct, we // can only add flags to hide things but not flags to show things.
Thank you for taking the time to document this, as well. Really thoughtful.
|
TFTRs! bors r+ |
|
Build succeeded: |
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: cockroachdb#72576 See also: cockroachdb#85239 Release note (sql change): creating a not visible index using `CREATE TABLE … (INDEX … NOT VISIBLE)` or `CREATE INDEX … NOT VISIBLE` is now supported.
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: cockroachdb#72576 See also: cockroachdb#85239 Release note (sql change): creating a not visible index using `CREATE TABLE … (INDEX … NOT VISIBLE)` or `CREATE INDEX … NOT VISIBLE` is now supported.
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: cockroachdb#72576 See also: cockroachdb#85239 Release note (sql change): creating a not visible index using `CREATE TABLE …(INDEX … NOT VISIBLE)` or `CREATE INDEX … NOT VISIBLE` is now supported.
This commit adds a flag to ScanFlags to indicate when the invisible index
feature should be enabled in buildScan. The invisible index feature should be
enabled by default but temporarily disabled during any unique or foreign key
constraint check. More details on how the decision was made and which flag to
pass in
docs/RFCS/20220628_invisible_index.md. Note that no logic has beenadded to the optimizer yet. This commit just passes the correct flag to
buildScan and shows the effect of the flag in the output of test data.
Assists: #72576
See also: #85354
Release note: none