sql: v2.0.5: panic while executing 1 statements: UPSERT panics in AddRow #29436
Assignees
Labels
A-sql-execution
Relating to SQL execution.
C-bug
Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior.
S-2-temp-unavailability
Temp crashes or other availability problems. Can be worked around or resolved by restarting.
Comments
tbg
changed the title
tbg
added
S-2-temp-unavailability
knz
added
the
C-bug
|
This is caused by the explicit panic in this statement: I think the issue here is that As you can see, the col type info is created directly from I'll try to make a repro, but @vivekmenezes does this sound right? |
|
That sounds right. Glad you spotted this!
…On Sun, 2 Sep, 2018, 10:10 AM Jordan Lewis, ***@***.***> wrote:
This is caused by the explicit panic in this statement:
// AddRow attempts to insert a new row in the RowContainer. The row slice is not
// used directly: the Datum values inside the Datums are copied to internal storage.
// Returns an error if the allocation was denied by the MemoryMonitor.
func (c *RowContainer) AddRow(ctx context.Context, row tree.Datums) (tree.Datums, error) {
if len(row) != c.numCols {
panic(fmt.Sprintf("invalid row length %d, expected %d", len(row), c.numCols))
}
if c.numCols == 0 {
c.numRows++
return nil, nil
}
I think the issue here is that upsert returning doesn't take into account
mutation columns. Here's where the row container is initialized:
tu.rowsUpserted = sqlbase.NewRowContainer(
evalCtx.Mon.MakeBoundAccount(),
sqlbase.ColTypeInfoFromColDescs(tableDesc.Columns),
tu.insertRows.Len(),
)
As you can see, the col type info is created directly from
tabledesc.Columns. That field includes all of the public columns of the
table descriptor, but not the ones that are in the write and delete only
state. Since upsert returning needs to write to columns that are in that
state, when it tries to save the columns to return into the row container,
there's a mismatch in the number of columns it wrote and the number of
columns the row container was created with, and 💥.
I'll try to make a repro, but @vivekmenezes
<https://github.com/vivekmenezes> does this sound right?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#29436 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ALOpBDXpErBmswQ4k4trjpSpGX6LcyBoks5uW-bngaJpZM4WVrCG>
.
|
|
Hey I think this actually explains the issue in #29374 and others as well. The code that checks for |
|
Hm, no, my theory is wrong because users can't insert into a fresh column until its done backfilling completely. |
|
But I did confirm that |
|
Hmm, you don't even need The way I provoked this was to create a table, add a non-nullable column with a default, kill the server, restart it so it has to wait for its schema change lease to time out, then play around in the sql shell. The panic here was just on |
|
Ah, |
jordanlewis
changed the title
This was referenced Sep 3, 2018
craig bot
pushed a commit
that referenced
this issue
Sep 4, 2018
29487: sql: fix UPSERT RETURNING in presence of schema changes r=jordanlewis a=jordanlewis Fixes #29436. Release note (bug fix): fix crash caused by certain kinds of UPSERT RETURNING statements on tables with active schema changes. Co-authored-by: Jordan Lewis <jordanthelewis@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
https://sentry.io/cockroach-labs/cockroachdb/issues/669605580/
The text was updated successfully, but these errors were encountered: