sql: UPSERT execution incorrectly assumes partial index PUT and DEL columns will be last columns in mutation input #61414

mgartner · 2021-03-03T17:58:16Z

To Reproduce

# LogicTest: multiregion-9node-3region-3azs

statement ok
CREATE DATABASE multi_region_test_db PRIMARY REGION "ca-central-1" REGIONS "ap-southeast-2", "us-east-1" SURVIVE REGION FAILURE

statement ok
USE multi_region_test_db

statement ok
CREATE TABLE regional_by_row_table (
  pk INT PRIMARY KEY,
  a INT,
  b INT,
  UNIQUE INDEX uniq_idx (a) WHERE a > 0,
  FAMILY (pk, a)
) LOCALITY REGIONAL BY ROW

statement ok
UPSERT INTO regional_by_row_table (crdb_region, pk, a) VALUES ('us-east-1', 1, 1)

Stacktrace

    --- FAIL: TestCCLLogic/multiregion-9node-3region-3azs (0.97s)
        --- FAIL: TestCCLLogic/multiregion-9node-3region-3azs/__test (0.97s)
            logic.go:2567:

                testdata/logic_test/__test:18:
                expected success, but found
                (XX000) internal error: cannot convert int to type bool
                datum.go:392: in GetBool()
                DETAIL: stack trace:
                github.com/cockroachdb/cockroach/pkg/sql/sem/tree/datum.go:392: GetBool()
                github.com/cockroachdb/cockroach/pkg/sql/row/partial_index.go:66: Init()
                github.com/cockroachdb/cockroach/pkg/sql/upsert.go:155: processSourceRow()
                github.com/cockroachdb/cockroach/pkg/sql/upsert.go:100: BatchedNext()
                github.com/cockroachdb/cockroach/pkg/sql/plan_batch.go:159: startExec()
                github.com/cockroachdb/cockroach/pkg/sql/plan.go:498: func2()
                github.com/cockroachdb/cockroach/pkg/sql/walk.go:119: func1()
                github.com/cockroachdb/cockroach/pkg/sql/walk.go:296: visitInternal()
                github.com/cockroachdb/cockroach/pkg/sql/walk.go:86: visit()
                github.com/cockroachdb/cockroach/pkg/sql/walk.go:50: walkPlan()
                github.com/cockroachdb/cockroach/pkg/sql/plan.go:501: startExec()
                github.com/cockroachdb/cockroach/pkg/sql/plan_node_to_row_source.go:124: Start()
                github.com/cockroachdb/cockroach/pkg/sql/colexec/columnarizer.go:153: Init()
                github.com/cockroachdb/cockroach/pkg/sql/colflow/invariants_checker.go:40: Init()
                github.com/cockroachdb/cockroach/pkg/sql/colexecerror/error.go:91: CatchVectorizedRuntimeError()
                github.com/cockroachdb/cockroach/pkg/sql/colexec/materializer.go:241: Start()
                github.com/cockroachdb/cockroach/pkg/sql/execinfra/processorsbase.go:773: Run()
                github.com/cockroachdb/cockroach/pkg/sql/flowinfra/flow.go:382: Run()
                github.com/cockroachdb/cockroach/pkg/sql/distsql_running.go:367: Run()
                github.com/cockroachdb/cockroach/pkg/sql/distsql_running.go:983: PlanAndRun()
                github.com/cockroachdb/cockroach/pkg/sql/conn_executor_exec.go:1036: execWithDistSQLEngine()
                github.com/cockroachdb/cockroach/pkg/sql/conn_executor_exec.go:908: dispatchToExecutionEngine()
                github.com/cockroachdb/cockroach/pkg/sql/conn_executor_exec.go:652: execStmtInOpenState()
                github.com/cockroachdb/cockroach/pkg/sql/conn_executor_exec.go:120: execStmt()
                github.com/cockroachdb/cockroach/pkg/sql/conn_executor.go:1494: func1()
                github.com/cockroachdb/cockroach/pkg/sql/conn_executor.go:1496: execCmd()
                github.com/cockroachdb/cockroach/pkg/sql/conn_executor.go:1421: run()
                github.com/cockroachdb/cockroach/pkg/sql/conn_executor.go:482: ServeConn()
                github.com/cockroachdb/cockroach/pkg/sql/pgwire/conn.go:625: func1()
                runtime/asm_amd64.s:1374: goexit()

                NOTE: internal errors may have more details in logs. Use -show-logs.
            logic.go:3126:
                testdata/logic_test/__test:18: error while processing
            logic.go:3126: pq: internal error: cannot convert int to type bool
    logic.go:3351: -- test log scope end --

The text was updated successfully, but these errors were encountered:

mgartner · 2021-03-03T18:17:53Z

This appears related to #57097. execbuilder might add addition columns to mutation inputs:

cockroach/pkg/sql/opt/exec/execbuilder/mutation.go

Lines 47 to 57 in c9fb63a

    
           if p.WithID != 0 { 
        
           	// The input might have extra columns that are used only by FK or unique 
        
           	// checks; make sure we don't project them away. 
        
           	cols := inputExpr.Relational().OutputCols.Copy() 
        
           	for _, c := range colList { 
        
           		cols.Remove(c) 
        
           	} 
        
           	for c, ok := cols.Next(0); ok; c, ok = cols.Next(c + 1) { 
        
           		colList = append(colList, c) 
        
           	} 
        
           }

This commit adds tests for implicitly partitioned partial unique indexes. It also fixes a bug that caused `UPSERT`s to fail on a regional-by-row table with a partial unique index. The upsert execution code previously assumed that partial index PUT and DEL columns are the last columns in the mutation's input. This may not be the case when FK or unique checks require additional columns. The execution code no longer makes this assumption. There is no regression test added for this case because it was caught by existing tests in `pkg/ccl/logictestccl` once partial unique indexes were added to tables. Fixes cockroachdb#61414 Release justification: This is a low-risk bug fix for multi-region features. Release note: None

This commit fixes a bug in UPSERT execution that was caused by the incorrect assumption that there are never columns following the partial index PUT and DEL columns in the mutation's input. I was unable to reproduce this bug for INSERTs or DELETEs. However, I updated the execution code for both out of caution. The logic for both no longer makes the same assumption that partial index PUT and DEL columns will be the last columns in the input. This commit also adds tests for implicitly partitioned partial unique indexes. Fixes cockroachdb#61414 Release justification: This is a low-risk bug fix necessary for new multi-region features. Release note: None

karlseguin · 2021-03-04T10:24:05Z

I think we just ran into this on a production cluster.

v20.2.5 on ubuntu 20.04

The partial index was newly added. The column indexed was newly added, a timestamptz with default now(). The condition was on an existing enum. Not sure if any of that matters.

Any chance if you can tell me if you think this is the same issue, else I'll open a new one.

Logs:

Error types: (1) *assert.withAssertionFailure (2) *withstack.withStack (3) *errutil.leafError
E210304 09:51:16.798332 56773637 sql/sqltelemetry/report.go:56 ⋮ [REDACTED] encountered internal error:
cannot convert ‹uuid› to type ‹bool›
(1) assertion failure
Wraps: (2) attached stack trace
  -- stack trace:
  | github.com/cockroachdb/cockroach/pkg/sql/sem/tree.GetBool
  |   /go/src/github.com/cockroachdb/cockroach/pkg/sql/sem/tree/datum.go:391
  | github.com/cockroachdb/cockroach/pkg/sql/row.(*PartialIndexUpdateHelper).Init
  |   /go/src/github.com/cockroachdb/cockroach/pkg/sql/row/partial_index.go:70
  | github.com/cockroachdb/cockroach/pkg/sql.(*upsertNode).processSourceRow
  |   /go/src/github.com/cockroachdb/cockroach/pkg/sql/upsert.go:159
  | github.com/cockroachdb/cockroach/pkg/sql.(*upsertNode).BatchedNext
  |   /go/src/github.com/cockroachdb/cockroach/pkg/sql/upsert.go:103
  | github.com/cockroachdb/cockroach/pkg/sql.(*serializeNode).Next
  |   /go/src/github.com/cockroachdb/cockroach/pkg/sql/plan_batch.go:112
  | github.com/cockroachdb/cockroach/pkg/sql.(*planNodeToRowSource).Next
  |   /go/src/github.com/cockroachdb/cockroach/pkg/sql/plan_node_to_row_source.go:178
  | github.com/cockroachdb/cockroach/pkg/sql/execinfra.Run
  |   /go/src/github.com/cockroachdb/cockroach/pkg/sql/execinfra/base.go:170
  | github.com/cockroachdb/cockroach/pkg/sql/execinfra.(*ProcessorBase).Run
  |   /go/src/github.com/cockroachdb/cockroach/pkg/sql/execinfra/processorsbase.go:775
  | github.com/cockroachdb/cockroach/pkg/sql/flowinfra.(*FlowBase).Run
  |   /go/src/github.com/cockroachdb/cockroach/pkg/sql/flowinfra/flow.go:393
  | github.com/cockroachdb/cockroach/pkg/sql.(*DistSQLPlanner).Run
  |   /go/src/github.com/cockroachdb/cockroach/pkg/sql/distsql_running.go:417
  | github.com/cockroachdb/cockroach/pkg/sql.(*DistSQLPlanner).PlanAndRun
  |   /go/src/github.com/cockroachdb/cockroach/pkg/sql/distsql_running.go:997
  | github.com/cockroachdb/cockroach/pkg/sql.(*connExecutor).execWithDistSQLEngine
  |   /go/src/github.com/cockroachdb/cockroach/pkg/sql/conn_executor_exec.go:1001
  | github.com/cockroachdb/cockroach/pkg/sql.(*connExecutor).dispatchToExecutionEngine
  |   /go/src/github.com/cockroachdb/cockroach/pkg/sql/conn_executor_exec.go:872
  | github.com/cockroachdb/cockroach/pkg/sql.(*connExecutor).execStmtInOpenState
  |   /go/src/github.com/cockroachdb/cockroach/pkg/sql/conn_executor_exec.go:639
  | github.com/cockroachdb/cockroach/pkg/sql.(*connExecutor).execStmt
  |   /go/src/github.com/cockroachdb/cockroach/pkg/sql/conn_executor_exec.go:114
  | github.com/cockroachdb/cockroach/pkg/sql.(*connExecutor).execPortal
  |   /go/src/github.com/cockroachdb/cockroach/pkg/sql/conn_executor_exec.go:203
  | github.com/cockroachdb/cockroach/pkg/sql.(*connExecutor).execCmd.func2
  |   /go/src/github.com/cockroachdb/cockroach/pkg/sql/conn_executor.go:1533
  | github.com/cockroachdb/cockroach/pkg/sql.(*connExecutor).execCmd
  |   /go/src/github.com/cockroachdb/cockroach/pkg/sql/conn_executor.go:1535
  | github.com/cockroachdb/cockroach/pkg/sql.(*connExecutor).run
  |   /go/src/github.com/cockroachdb/cockroach/pkg/sql/conn_executor.go:1391
  | github.com/cockroachdb/cockroach/pkg/sql.(*Server).ServeConn
  |   /go/src/github.com/cockroachdb/cockroach/pkg/sql/conn_executor.go:508
  | github.com/cockroachdb/cockroach/pkg/sql/pgwire.(*conn).processCommandsAsync.func1
  |   /go/src/github.com/cockroachdb/cockroach/pkg/sql/pgwire/conn.go:626
  | runtime.goexit
  |   /usr/local/go/src/runtime/asm_amd64.s:1357
Wraps: (3) cannot convert ‹uuid› to type ‹bool›
Error types: (1) *assert.withAssertionFailure (2) *withstack.withStack (3) *errutil.leafError

In the application, this is what we saw:

stack trace:
/go/src/github.com/cockroachdb/cockroach/pkg/sql/sem/tree/datum.go:391: GetBool()
/go/src/github.com/cockroachdb/cockroach/pkg/sql/row/partial_index.go:70: Init()
/go/src/github.com/cockroachdb/cockroach/pkg/sql/upsert.go:159: processSourceRow()
/go/src/github.com/cockroachdb/cockroach/pkg/sql/upsert.go:103: BatchedNext()
/go/src/github.com/cockroachdb/cockroach/pkg/sql/plan_batch.go:112: Next()
/go/src/github.com/cockroachdb/cockroach/pkg/sql/plan_node_to_row_source.go:178: Next()
/go/src/github.com/cockroachdb/cockroach/pkg/sql/execinfra/base.go:170: Run()
/go/src/github.com/cockroachdb/cockroach/pkg/sql/execinfra/processorsbase.go:775: Run()
/go/src/github.com/cockroachdb/cockroach/pkg/sql/flowinfra/flow.go:393: Run()
/go/src/github.com/cockroachdb/cockroach/pkg/sql/distsql_running.go:417: Run()
/go/src/github.com/cockroachdb/cockroach/pkg/sql/distsql_running.go:997: PlanAndRun()
/go/src/github.com/cockroachdb/cockroach/pkg/sql/conn_executor_exec.go:1001: execWithDistSQLEngine()
/go/src/github.com/cockroachdb/cockroach/pkg/sql/conn_executor_exec.go:872: dispatchToExecutionEngine()
/go/src/github.com/cockroachdb/cockroach/pkg/sql/conn_executor_exec.go:639: execStmtInOpenState()
/go/src/github.com/cockroachdb/cockroach/pkg/sql/conn_executor_exec.go:114: execStmt()
/go/src/github.com/cockroachdb/cockroach/pkg/sql/conn_executor_exec.go:203: execPortal()
/go/src/github.com/cockroachdb/cockroach/pkg/sql/conn_executor.go:1533: func2()
/go/src/github.com/cockroachdb/cockroach/pkg/sql/conn_executor.go:1535: execCmd()
/go/src/github.com/cockroachdb/cockroach/pkg/sql/conn_executor.go:1391: run()
/go/src/github.com/cockroachdb/cockroach/pkg/sql/conn_executor.go:508: ServeConn()
/go/src/github.com/cockroachdb/cockroach/pkg/sql/pgwire/conn.go:626: func1()
/usr/local/go/src/runtime/asm_amd64.s:1357: goexit()

file: "datum.go" 

hint: "You have encountered an unexpected error.
Please check the public issue tracker to check whether this problem is
already tracked. If you cannot find it there, please report the error
with details by creating a new issue.
If you would rather not post publicly, please contact us directly
using the support form.

line: "391"

message: "internal error: cannot convert uuid to type bool", pg_code: "XX000", routine: "GetBool", severity: "ERROR"

mgartner · 2021-03-04T16:29:23Z

It looks like it could be the same issue. Are you able to share the schema and statement that produce the error so that I can confirm? If not, is there a simplified reproduction you can share?

karlseguin · 2021-03-04T16:57:23Z

Hope it's ok anonymized (and hope i mapped this properly)

create type tbl_enum1 as enum (
  'tbl_enum1_a', 'tbl_enum1_b', 'tbl_enum1_c'
);

create type tbl_enum2 as enum (
  'tbl_enum2_a', 'tbl_enum2_b', 'tbl_enum2_c', 'tbl_enum2_d'
);

create table tbl (
  id uuid not null primary key,
  col1 text not null,
  col2 text not null,
  col3 tbl_enum2 not null,
  col4 bool not null,
  col5 uuid null,
  col6 bytea null,
  col7 text null,
  col8 timestamptz null,
  col9 smallint not null default(0),
  col10 timestamptz null,
  col11 tbl_enum1 not null,
  col12 uuid null references tbl_2(id),
  col13 timestamptz not null default now(), -- newly added
  index tbl_idx1(col5),
  unique index tbl_idx2(col1, col12, col11),
  index tbl_idx3(col13) where col3 = 'tbl_enum2_c' -- newly added, removing this fixed it
);

With the statement that caused it:

insert into tbl (id, col1, col2, col5, col12, col3, col4, col11)
values ($1, $2, $3, $4, $5, $6, $7, 'tbl_enum1_a')
on conflict (col1, col12, col11) do update set
  col2 = excluded.password,
  col5 = excluded.user_id,
  col3 = excluded.status,
  col4 = excluded.reset
where tbl.col3 = 'tbl_enum2_c'
returning id

This commit fixes a bug in UPSERT execution that was caused by the incorrect assumption that there are never columns following the partial index PUT and DEL columns in the mutation's input. I was unable to reproduce this bug for INSERTs or DELETEs. However, I updated the execution code for both out of caution. The logic for both no longer makes the same assumption that partial index PUT and DEL columns will be the last columns in the input. Fixes cockroachdb#61414 Release justification: This is a low-risk fix for a bug causing UPSERT statements to err when executed on tables with partial indexes. Release note (bug fix): A bug which caused UPSERT and INSERT..ON CONFLICT..DO UPDATE statements to fail on tables with both partial indexes and foreign key references has been fixed. This bug has been present since version 20.2.0.

mgartner · 2021-03-04T17:56:42Z

@karlseguin Thanks for catching this and sharing! I've confirmed that you're running into the same bug.

#61416 will fix this issue and I'll backport the fix so that it will be included in an upcoming 20.2.x release, hopefully the next one, 20.2.6.

This commit fixes a bug in UPSERT execution that was caused by the incorrect assumption that there are never columns following the partial index PUT and DEL columns in the mutation's input. I was unable to reproduce this bug for INSERTs or DELETEs. However, I updated the execution code for both out of caution. The logic for both no longer makes the same assumption that partial index PUT and DEL columns will be the last columns in the input. Fixes cockroachdb#61414 Release justification: This is a low-risk fix for a bug causing UPSERT statements to err when executed on tables with partial indexes. Release note (bug fix): A bug which caused UPSERT and INSERT..ON CONFLICT..DO UPDATE statements to fail on tables with both partial indexes and foreign key references has been fixed. This bug has been present since version 20.2.0.

61416: sql: fix UPSERT with columns after partial index PUT and DEL columns r=mgartner a=mgartner #### sql: fix UPSERT with columns after partial index PUT and DEL columns This commit fixes a bug in UPSERT execution that was caused by the incorrect assumption that there are never columns following the partial index PUT and DEL columns in the mutation's input. I was unable to reproduce this bug for INSERTs or DELETEs. However, I updated the execution code for both out of caution. The logic for both no longer makes the same assumption that partial index PUT and DEL columns will be the last columns in the input. Fixes #61414 Release justification: This is a low-risk fix for a bug causing UPSERT statements to err when executed on tables with partial indexes. Release note (bug fix): A bug which caused UPSERT and INSERT..ON CONFLICT..DO UPDATE statements to fail on tables with both partial indexes and foreign key references has been fixed. This bug has been present since version 20.2.0. #### ccl: add tests for implicitly partitioned partial unqiue indexes Release justification: This is a test-only change. Release note: None Co-authored-by: Marcus Gartner <[email protected]>

mgartner added C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. A-multiregion Related to multi-region labels Mar 3, 2021

mgartner self-assigned this Mar 3, 2021

mgartner mentioned this issue Mar 3, 2021

sql: fix UPSERT with columns after partial index PUT and DEL columns #61416

Merged

mgartner changed the title ~~sql: error on UPSERT to regional-by-row table with partial unique index~~ sql: UPSERT execution incorrectly assumes partial index PUT and DEL columns will be last columns in mutation input Mar 4, 2021

mgartner mentioned this issue Mar 4, 2021

release-20.2: sql: fix UPSERT with columns after partial index PUT and DEL columns #61489

Merged

craig bot closed this as completed in 442dc43 Mar 4, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sql: UPSERT execution incorrectly assumes partial index PUT and DEL columns will be last columns in mutation input #61414

sql: UPSERT execution incorrectly assumes partial index PUT and DEL columns will be last columns in mutation input #61414

mgartner commented Mar 3, 2021

mgartner commented Mar 3, 2021

karlseguin commented Mar 4, 2021 •

edited

Loading

mgartner commented Mar 4, 2021

karlseguin commented Mar 4, 2021

mgartner commented Mar 4, 2021

sql: UPSERT execution incorrectly assumes partial index PUT and DEL columns will be last columns in mutation input #61414

sql: UPSERT execution incorrectly assumes partial index PUT and DEL columns will be last columns in mutation input #61414

Comments

mgartner commented Mar 3, 2021

mgartner commented Mar 3, 2021

karlseguin commented Mar 4, 2021 • edited Loading

mgartner commented Mar 4, 2021

karlseguin commented Mar 4, 2021

mgartner commented Mar 4, 2021

karlseguin commented Mar 4, 2021 •

edited

Loading