sql: fix cached gists panic'ing after schema changes #76801
Conversation
|
This change is |
cucaroach
force-pushed
the
gist-fix-76800
branch
from
81d876a to
635142d
Compare
| ---- | ||
| • scan | ||
| table: t2@t2_pkey | ||
| spans: FULL SCAN |
There was a problem hiding this comment.
How will the columns be displayed if they can't be decoded? Maybe this query should be a lookup join to test that case: SELECT * FROM t2 AS t2a INNER LOOKUP JOIN t2 AS t2b ON t2a.a = t2b.a and add an INDEX (a).
| Name: string(col.ColName()), | ||
| Typ: col.DatumType(), | ||
| }) | ||
| // Be defensive about bitset values because they may come from cached gists. |
There was a problem hiding this comment.
nit: Might be helpful to explain that this guards against deleted columns.
| @@ -655,8 +655,7 @@ func (opc *optPlanningCtx) runExecBuilder( | |||
| return nil | |||
| } | |||
|
|
|||
| // DecodeGist Avoid an import cycle by keeping the cat out of the tree, RFC: is | |||
| // there a better way? | |||
| // DecodeGist Avoid an import cycle by keeping the cat out of the tree. | |||
There was a problem hiding this comment.
Avoids a call to the fire department too!
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @cucaroach, @mgartner, and @RaduBerinde)
pkg/sql/opt/exec/explain/result_columns.go, line 204 at r1 (raw file):
Previously, mgartner (Marcus Gartner) wrote…
nit: Might be helpful to explain that this guards against deleted columns.
👍
pkg/sql/opt/exec/execbuilder/testdata/explain_gist, line 86 at r1 (raw file):
Previously, mgartner (Marcus Gartner) wrote…
How will the columns be displayed if they can't be decoded? Maybe this query should be a lookup join to test that case:
SELECT * FROM t2 AS t2a INNER LOOKUP JOIN t2 AS t2b ON t2a.a = t2b.aand add anINDEX (a).
So if the column index is out of bounds you'll get a "_" in the plan. But its also possible they aren't out of bounds and now refer to a new column (even "rowid" or "crdb_mvcc_timestamp"). Not perfect but should be rare.
Previously we didn't guard against out of bounds column ids in gists,
if a gist is created on a table and then that table drops columns we
would hit a runtime index out of bounds panic. By design gist decoding
is best effort and should silently ignore these columns when this
happens. The columns are used for equality conditions, ie:
└── • lookup join
│ table: broker@broker_pkey
│ equality: (tr_s_symb) = (b_id)
So they are a nice to have. Doing anything more sophisticated would
dramatically increase the gist size so isn't worth it.
Fixes: cockroachdb#76800
Release note: None
cucaroach
force-pushed
the
gist-fix-76800
branch
from
635142d to
edc105f
Compare
There was a problem hiding this comment.
Reviewed 2 of 3 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status:
|
bors r=mgartner |
|
Build failed (retrying...): |
|
Build failed (retrying...): |
|
Build failed: |
|
bors r=mgartner |
|
Build succeeded: |
Previously we didn't guard against out of bounds column ids in gists,
if a gist is created on a table and then that table drops columns we
would hit a runtime index out of bounds panic. By design gist decoding
is best effort and should silently ignore these columns when this
happens. The columns are used for equality conditions, ie:
So they are a nice to have. Doing anything more sophisticated would
dramatically increase the gist size so isn't worth it.
Fixes: #76800
Release note: None