sql: fix create table failure with regional by row #80590
Conversation
|
This change is |
fqazi
force-pushed
the
missingTypeIssue
branch
2 times, most recently
from
2e250d0
to
df2db04
Compare
Fixes: cockroachdb#71600 Previously, if a transaction retry error happened during a CREATE TABLE, we could had AST changes persist which could lead to incorrect behaviour. For example tables with a locality of regional by row would have an implicit column added with an OID from the database descriptor. Unfortunately, after a transaction retry it is possible that the OID ID could have changed. To address this, and prevent future issues in CREATE TABLE, we are going to copy the column definition slice. Release note (bug fix): Creating a table with a locality of regional by row could intermittently fail with a missing type error.
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @fqazi and @Xiang-Gu)
pkg/sql/create_table.go line 263 at r1 (raw file):
defer func(originalDefs tree.TableDefs) { n.n.Defs = originalDefs }(n.n.Defs) n.n.Defs = defsCopy
If I understand correctly, we want to save a copy defsCopy to be the initial n.n.Defs. We then go ahead and modify n.n.Defs but we can rest assured that we have deferred a function that reset n.n.Defs back to the initial defsCopy that we saved and is intact.
I didn't understand the defer function where we essentially did n.n.Defs = n.n.Defs
There was a problem hiding this comment.
Reviewable status:
pkg/sql/create_table.go line 263 at r1 (raw file):
Previously, Xiang-Gu (Xiang Gu) wrote…
If I understand correctly, we want to save a copy
defsCopyto be the initialn.n.Defs. We then go ahead and modifyn.n.Defsbut we can rest assured that we have deferred a function that resetn.n.Defsback to the initialdefsCopythat we saved and is intact.I didn't understand the defer function where we essentially did
n.n.Defs = n.n.Defs
The key here is that this function will be invoked when the function scope ends. With the original slice that we specified as a parameter to the call (think of that being closure capturing it in a slice). The slice afterwards could have had any value, but will be reverted back to the originally observed value
There was a problem hiding this comment.
Reviewable status:
pkg/sql/create_table.go line 263 at r1 (raw file):
Previously, fqazi (Faizan Qazi) wrote…
The key here is that this function will be invoked when the function scope ends. With the original slice that we specified as a parameter to the call (think of that being closure capturing it in a slice). The slice afterwards could have had any value, but will be reverted back to the originally observed value
yeah, the key point about defer is that, the function input value is evaluated at definition time. so the defer function takes the original defs value as input. this is true even we over write it with the defsCopy. this should work.
|
@Xiang-Gu @chengxiong-ruan TFTR! |
|
bors r+ |
|
Build succeeded: |
Fixes: #71600
Previously, if a transaction retry error happened
during a CREATE TABLE, we could had AST changes
persist which could lead to incorrect behaviour.
For example tables with a locality of regional
by row would have an implicit column added with an
OID from the database descriptor. Unfortunately, after
a transaction retry it is possible that the OID
ID could have changed. To address this, and prevent
future issues in CREATE TABLE, we are going to clone
the AST before any modification.
Release note (bug fix): Creating a table with a locality
of regional by row could intermittently fail with a missing
type error.