sql: UPSERT fast-path broken

[petermattis]

Consider a simple table:

CREATE TABLE IF NOT EXISTS test.kv (
  k BIGINT NOT NULL PRIMARY KEY,
  v BYTES NOT NULL
)

The query UPSERT INTO test.kv (k, v) VALUES (1, 'hello'::bytes) should hit the UPSERT fast-path in which we blindly put the values. The fast-path is controlled by the conditional at https://github.com/cockroachdb/cockroach/blob/master/pkg/sql/tablewriter.go#L230:

    allColsIdentityExpr := len(tu.ri.insertCols) == len(tu.tableDesc.Columns) &&
        tu.evaler != nil && tu.evaler.isIdentityEvaler()
    if len(tu.tableDesc.Indexes) == 0 && allColsIdentityExpr {
        // Do fast-path
    }

But we're not taking the fast-path because allColsIdentityExpr is false. And that variable is false because tu.evaler.isIdentityEvaler() is false. Following this down we hit the code at https://github.com/cockroachdb/cockroach/blob/master/pkg/sql/upsert.go#L135-L149:

    helper.allExprsIdentity = true
    for i, expr := range evalExprs {
        // analyzeExpr above has normalized all direct column names to ColumnItems.
        c, ok := expr.(*parser.ColumnItem)
        if !ok {
            helper.allExprsIdentity = false
            break
        }
        if len(c.Selector) > 0 ||
            !c.TableName.TableName.Equal(upsertExcludedTable.TableName) ||
            c.ColumnName.Normalize() != parser.ReNormalizeName(updateCols[i].Name) {
            helper.allExprsIdentity = false
            break
        }
    }

This code always sets helper.allExprsIdentity to true because evalExprs contains *parser.IndexedVar exprs, not *parser.ColumnItem. Consulting with @danhhz, this likely regressed at some point.

In addition to fixing the regression, it would be good to add a test. Perhaps something based on EXPLAIN(PLAN). At the moment EXPLAIN(PLAN) for the above UPSERT query doesn't show enough details of what is going on. It definitely doesn't show the scan that is being performed. That too is a deficiency that should be fixed.

Cc @knz

[danhhz]

@knz any thoughts on how to test this?

[knz]

to check that the fast path was taken?

If there is a difference in the K/V operations that are run, then I'd suggest checking the output of EXPLAIN(TRACE), which andrei recently fixed.

Otherwise, add a boolean in the data structure, and make a Go test that builds and runs a plan for a couple queries and checks the boolean.

[danhhz]

@knz any chance I could get you to pick this one up?

It seems EXPLAIN(TRACE) doesn't work with INSERT/UPSERT and EXPLAIN(PLAN) would need to be fixed for upsert as well. I haven't really worked with either of those.

I also tried writing the fix (we're getting IndexedVar instead of ColumnItem in https://github.com/cockroachdb/cockroach/blob/master/pkg/sql/upsert.go#L138-L148) and realized anything I knew had paged out (and lots has changed since I last looked at it).