From 06581b3dbd1e302dc3108155344de70188b4f856 Mon Sep 17 00:00:00 2001 From: Radu Berinde Date: Sun, 4 Aug 2019 23:03:10 -0400 Subject: [PATCH] opt: add remaining plan nodes as opaque operators Allow all misc plan nodes to be created through the optimizer via an Opaque operator. The code for `Grant/Revoke` needed some reworking because it did its work during planning (!) instead of execution. Release note: None --- pkg/server/updates_test.go | 79 +++++---- pkg/sql/conn_executor_exec.go | 8 + pkg/sql/expand_plan.go | 2 + pkg/sql/grant_revoke.go | 66 ++++---- pkg/sql/logictest/testdata/logic_test/backup | 5 - pkg/sql/logictest/testdata/logic_test/ccl | 9 ++ .../logictest/testdata/logic_test/optimizer | 6 - .../testdata/logic_test/statement_statistics | 8 +- pkg/sql/metric_test.go | 4 +- pkg/sql/opaque.go | 150 ++++++++++++++++-- pkg/sql/opt/exec/execbuilder/relational.go | 8 +- pkg/sql/opt_exec_factory.go | 7 +- pkg/sql/opt_filters.go | 1 + pkg/sql/opt_limits.go | 1 + pkg/sql/opt_needed.go | 1 + pkg/sql/plan.go | 1 + pkg/sql/plan_opt.go | 26 +++ pkg/sql/plan_physical_props.go | 1 + pkg/sql/schema_changer_test.go | 64 ++++++-- pkg/sql/sem/tree/stmt.go | 3 + pkg/sql/walk.go | 5 +- 21 files changed, 341 insertions(+), 114 deletions(-) delete mode 100644 pkg/sql/logictest/testdata/logic_test/backup diff --git a/pkg/server/updates_test.go b/pkg/server/updates_test.go index 7f9531ecb6af..7b756a286f33 100644 --- a/pkg/server/updates_test.go +++ b/pkg/server/updates_test.go @@ -828,41 +828,58 @@ func TestReportUsage(t *testing.T) { // Let's ignore all internal queries for this test. continue } - foundKeys = append(foundKeys, - fmt.Sprintf("[%v,%v,%v] %s", s.Key.Opt, s.Key.DistSQL, s.Key.Failed, s.Key.Query)) + var tags []string + if s.Key.Opt { + tags = append(tags, "opt") + } + + if s.Key.DistSQL { + tags = append(tags, "dist") + } else { + tags = append(tags, "nodist") + } + + if s.Key.Failed { + tags = append(tags, "failed") + } else { + tags = append(tags, "ok") + } + + foundKeys = append(foundKeys, fmt.Sprintf("[%s] %s", strings.Join(tags, ","), s.Key.Query)) } sort.Strings(foundKeys) expectedKeys := []string{ - `[false,false,false] ALTER DATABASE _ CONFIGURE ZONE = _`, - `[false,false,false] ALTER TABLE _ CONFIGURE ZONE = _`, - `[false,false,false] CREATE DATABASE _`, - `[false,false,false] SET CLUSTER SETTING "cluster.organization" = _`, - `[false,false,false] SET CLUSTER SETTING "diagnostics.reporting.send_crash_reports" = _`, - `[false,false,false] SET CLUSTER SETTING "server.time_until_store_dead" = _`, - `[false,false,false] SET application_name = $1`, - `[false,false,false] SET application_name = DEFAULT`, - `[false,false,false] SET application_name = _`, - `[true,false,false] CREATE TABLE _ (_ INT8 NOT NULL DEFAULT unique_rowid())`, - `[true,false,false] CREATE TABLE _ (_ INT8, CONSTRAINT _ CHECK (_ > _))`, - `[true,false,false] INSERT INTO _ SELECT unnest(ARRAY[_, _, __more2__])`, - `[true,false,false] INSERT INTO _ VALUES (_), (__more2__)`, - `[true,false,false] INSERT INTO _ VALUES (length($1::STRING)), (__more1__)`, - `[true,false,false] INSERT INTO _(_, _) VALUES (_, _)`, - `[true,false,false] SELECT (_, _, __more2__) = (SELECT _, _, _, _ FROM _ LIMIT _)`, - `[true,false,false] SELECT _ FROM (VALUES (_)) AS _ (_) WHERE EXISTS (SELECT * FROM (VALUES (_)) AS _ (_) WHERE _._ = _._)`, - "[true,false,false] SELECT _::STRING::INET, _::JSONB - _, ARRAY (SELECT _)[_]", - `[true,false,false] UPDATE _ SET _ = _ + _`, - "[true,false,false] WITH _ AS (SELECT _) SELECT * FROM _", - `[true,false,true] CREATE TABLE _ (_ INT8 PRIMARY KEY, _ INT8, INDEX (_) INTERLEAVE IN PARENT _ (_))`, - `[true,false,true] SELECT _ / $1`, - `[true,false,true] SELECT _ / _`, - `[true,false,true] SELECT crdb_internal.force_assertion_error(_)`, - `[true,false,true] SELECT crdb_internal.force_error(_, $1)`, - `[true,false,true] SELECT crdb_internal.set_vmodule(_)`, - `[true,true,false] SELECT * FROM _ WHERE (_ = _) AND (_ = _)`, - `[true,true,false] SELECT * FROM _ WHERE (_ = length($1::STRING)) OR (_ = $2)`, - `[true,true,false] SELECT _ FROM _ WHERE (_ = _) AND (lower(_) = lower(_))`, + `[opt,nodist,ok] ALTER DATABASE _ CONFIGURE ZONE = _`, + `[opt,nodist,ok] ALTER TABLE _ CONFIGURE ZONE = _`, + `[opt,nodist,ok] CREATE DATABASE _`, + `[opt,nodist,ok] SET CLUSTER SETTING "cluster.organization" = _`, + `[opt,nodist,ok] SET CLUSTER SETTING "diagnostics.reporting.send_crash_reports" = _`, + `[opt,nodist,ok] SET CLUSTER SETTING "server.time_until_store_dead" = _`, + `[opt,nodist,ok] SET application_name = $1`, + `[opt,nodist,ok] SET application_name = DEFAULT`, + `[opt,nodist,ok] SET application_name = _`, + `[opt,nodist,ok] CREATE TABLE _ (_ INT8 NOT NULL DEFAULT unique_rowid())`, + `[opt,nodist,ok] CREATE TABLE _ (_ INT8, CONSTRAINT _ CHECK (_ > _))`, + `[opt,nodist,ok] INSERT INTO _ SELECT unnest(ARRAY[_, _, __more2__])`, + `[opt,nodist,ok] INSERT INTO _ VALUES (_), (__more2__)`, + `[opt,nodist,ok] INSERT INTO _ VALUES (length($1::STRING)), (__more1__)`, + `[opt,nodist,ok] INSERT INTO _(_, _) VALUES (_, _)`, + `[opt,nodist,ok] SELECT (_, _, __more2__) = (SELECT _, _, _, _ FROM _ LIMIT _)`, + `[opt,nodist,ok] SELECT _ FROM (VALUES (_)) AS _ (_) WHERE EXISTS (SELECT * FROM (VALUES (_)) AS _ (_) WHERE _._ = _._)`, + "[opt,nodist,ok] SELECT _::STRING::INET, _::JSONB - _, ARRAY (SELECT _)[_]", + `[opt,nodist,ok] UPDATE _ SET _ = _ + _`, + "[opt,nodist,ok] WITH _ AS (SELECT _) SELECT * FROM _", + `[opt,nodist,failed] CREATE TABLE _ (_ INT8 PRIMARY KEY, _ INT8, INDEX (_) INTERLEAVE IN PARENT _ (_))`, + `[opt,nodist,failed] SELECT _ / $1`, + `[opt,nodist,failed] SELECT _ / _`, + `[opt,nodist,failed] SELECT crdb_internal.force_assertion_error(_)`, + `[opt,nodist,failed] SELECT crdb_internal.force_error(_, $1)`, + `[opt,nodist,failed] SELECT crdb_internal.set_vmodule(_)`, + `[opt,dist,ok] SELECT * FROM _ WHERE (_ = _) AND (_ = _)`, + `[opt,dist,ok] SELECT * FROM _ WHERE (_ = length($1::STRING)) OR (_ = $2)`, + `[opt,dist,ok] SELECT _ FROM _ WHERE (_ = _) AND (lower(_) = lower(_))`, } + sort.Strings(expectedKeys) t.Logf("expected:\n%s\ngot:\n%s", pretty.Sprint(expectedKeys), pretty.Sprint(foundKeys)) for i, found := range foundKeys { if i >= len(expectedKeys) { diff --git a/pkg/sql/conn_executor_exec.go b/pkg/sql/conn_executor_exec.go index c4979759c5c1..c1ca8b3fbd65 100644 --- a/pkg/sql/conn_executor_exec.go +++ b/pkg/sql/conn_executor_exec.go @@ -819,6 +819,14 @@ func canFallbackFromOpt(err error, optMode sessiondata.OptimizerMode, stmt *Stat // We only fallback on "feature not supported" errors. return false } + if err.Error() == "unimplemented: schema change statement cannot follow a statement that has written in the same transaction" { + // This is a special error generated when SetSystemConfigTrigger fails. If + // we fall back to the heuristic planner, the second call to that method + // succeeds. + // TODO(radu): this will go away very soon when we remove fallback + // altogether. + return false + } if optMode == sessiondata.OptimizerAlways { // In Always mode we never fallback, with one exception: SET commands (or diff --git a/pkg/sql/expand_plan.go b/pkg/sql/expand_plan.go index d518567fecec..9fbf8163ce69 100644 --- a/pkg/sql/expand_plan.go +++ b/pkg/sql/expand_plan.go @@ -354,6 +354,7 @@ func doExpandPlan( case *alterTableNode: case *alterSequenceNode: case *alterUserSetPasswordNode: + case *changePrivilegesNode: case *commentOnColumnNode: case *commentOnDatabaseNode: case *commentOnTableNode: @@ -877,6 +878,7 @@ func (p *planner) simplifyOrderings(plan planNode, usefulOrdering sqlbase.Column case *alterTableNode: case *alterSequenceNode: case *alterUserSetPasswordNode: + case *changePrivilegesNode: case *commentOnColumnNode: case *commentOnDatabaseNode: case *commentOnTableNode: diff --git a/pkg/sql/grant_revoke.go b/pkg/sql/grant_revoke.go index f3f4208a9545..1ca4fde2cf6e 100644 --- a/pkg/sql/grant_revoke.go +++ b/pkg/sql/grant_revoke.go @@ -29,9 +29,13 @@ import ( // Notes: postgres requires the object owner. // mysql requires the "grant option" and the same privileges, and sometimes superuser. func (p *planner) Grant(ctx context.Context, n *tree.Grant) (planNode, error) { - return p.changePrivileges(ctx, n.Targets, n.Grantees, func(privDesc *sqlbase.PrivilegeDescriptor, grantee string) { - privDesc.Grant(grantee, n.Privileges) - }) + return &changePrivilegesNode{ + targets: n.Targets, + grantees: n.Grantees, + changePrivilege: func(privDesc *sqlbase.PrivilegeDescriptor, grantee string) { + privDesc.Grant(grantee, n.Privileges) + }, + }, nil } // Revoke removes privileges from users. @@ -44,30 +48,37 @@ func (p *planner) Grant(ctx context.Context, n *tree.Grant) (planNode, error) { // Notes: postgres requires the object owner. // mysql requires the "grant option" and the same privileges, and sometimes superuser. func (p *planner) Revoke(ctx context.Context, n *tree.Revoke) (planNode, error) { - return p.changePrivileges(ctx, n.Targets, n.Grantees, func(privDesc *sqlbase.PrivilegeDescriptor, grantee string) { - privDesc.Revoke(grantee, n.Privileges) - }) + return &changePrivilegesNode{ + targets: n.Targets, + grantees: n.Grantees, + changePrivilege: func(privDesc *sqlbase.PrivilegeDescriptor, grantee string) { + privDesc.Revoke(grantee, n.Privileges) + }, + }, nil +} + +type changePrivilegesNode struct { + targets tree.TargetList + grantees tree.NameList + changePrivilege func(*sqlbase.PrivilegeDescriptor, string) } -func (p *planner) changePrivileges( - ctx context.Context, - targets tree.TargetList, - grantees tree.NameList, - changePrivilege func(*sqlbase.PrivilegeDescriptor, string), -) (planNode, error) { +func (n *changePrivilegesNode) startExec(params runParams) error { + ctx := params.ctx + p := params.p // Check whether grantees exists users, err := p.GetAllUsersAndRoles(ctx) if err != nil { - return nil, err + return err } // We're allowed to grant/revoke privileges to/from the "public" role even though // it does not exist: add it to the list of all users and roles. users[sqlbase.PublicRole] = true // isRole - for _, grantee := range grantees { + for _, grantee := range n.grantees { if _, ok := users[string(grantee)]; !ok { - return nil, errors.Errorf("user or role %s does not exist", &grantee) + return errors.Errorf("user or role %s does not exist", &grantee) } } @@ -75,10 +86,10 @@ func (p *planner) changePrivileges( // DDL statements avoid the cache to avoid leases, and can view non-public descriptors. // TODO(vivek): check if the cache can be used. p.runWithOptions(resolveFlags{skipCache: true}, func() { - descriptors, err = getDescriptorsFromTargetList(ctx, p, targets) + descriptors, err = getDescriptorsFromTargetList(ctx, p, n.targets) }) if err != nil { - return nil, err + return err } // First, update the descriptors. We want to catch all errors before @@ -86,23 +97,23 @@ func (p *planner) changePrivileges( b := p.txn.NewBatch() for _, descriptor := range descriptors { if err := p.CheckPrivilege(ctx, descriptor, privilege.GRANT); err != nil { - return nil, err + return err } privileges := descriptor.GetPrivileges() - for _, grantee := range grantees { - changePrivilege(privileges, string(grantee)) + for _, grantee := range n.grantees { + n.changePrivilege(privileges, string(grantee)) } // Validate privilege descriptors directly as the db/table level Validate // may fix up the descriptor. if err := privileges.Validate(descriptor.GetID()); err != nil { - return nil, err + return err } switch d := descriptor.(type) { case *sqlbase.DatabaseDescriptor: if err := d.Validate(); err != nil { - return nil, err + return err } writeDescToBatch(ctx, p.extendedEvalCtx.Tracing.KVTracingEnabled(), p.execCfg.Settings, b, descriptor.GetID(), descriptor) @@ -111,15 +122,16 @@ func (p *planner) changePrivileges( if !d.Dropped() { if err := p.writeSchemaChangeToBatch( ctx, d, sqlbase.InvalidMutationID, b); err != nil { - return nil, err + return err } } } } // Now update the descriptors transactionally. - if err := p.txn.Run(ctx, b); err != nil { - return nil, err - } - return newZeroNode(nil /* columns */), nil + return p.txn.Run(ctx, b) } + +func (*changePrivilegesNode) Next(runParams) (bool, error) { return false, nil } +func (*changePrivilegesNode) Values() tree.Datums { return tree.Datums{} } +func (*changePrivilegesNode) Close(context.Context) {} diff --git a/pkg/sql/logictest/testdata/logic_test/backup b/pkg/sql/logictest/testdata/logic_test/backup deleted file mode 100644 index 196661a955d3..000000000000 --- a/pkg/sql/logictest/testdata/logic_test/backup +++ /dev/null @@ -1,5 +0,0 @@ -query error pgcode XXC01 a CCL binary is required to use this statement type -BACKUP DATABASE foo TO '/bar' INCREMENTAL FROM '/baz' - -query error pgcode XXC01 a CCL binary is required to use this statement type -RESTORE DATABASE foo FROM '/bar' diff --git a/pkg/sql/logictest/testdata/logic_test/ccl b/pkg/sql/logictest/testdata/logic_test/ccl index 11f17350d97a..7de5054b9562 100644 --- a/pkg/sql/logictest/testdata/logic_test/ccl +++ b/pkg/sql/logictest/testdata/logic_test/ccl @@ -19,3 +19,12 @@ GRANT foo TO testuser statement error pgcode XXC01 a CCL binary is required to use this statement type: \*tree\.RevokeRole REVOKE foo FROM testuser + +statement error pgcode XXC01 a CCL binary is required to use this statement type: \*tree\.CreateChangefeed +CREATE CHANGEFEED FOR foo + +query error pgcode XXC01 a CCL binary is required to use this statement type +BACKUP DATABASE foo TO '/bar' INCREMENTAL FROM '/baz' + +query error pgcode XXC01 a CCL binary is required to use this statement type +RESTORE DATABASE foo FROM '/bar' diff --git a/pkg/sql/logictest/testdata/logic_test/optimizer b/pkg/sql/logictest/testdata/logic_test/optimizer index 6f4d1b079a43..fdc13b8ce941 100644 --- a/pkg/sql/logictest/testdata/logic_test/optimizer +++ b/pkg/sql/logictest/testdata/logic_test/optimizer @@ -78,12 +78,6 @@ SELECT * FROM tview 3 30 4 40 -statement ok -SET OPTIMIZER = ALWAYS - -query error pq: unimplemented: unsupported statement: \*tree\.AlterTable -ALTER TABLE test DROP COLUMN v; - statement ok SET OPTIMIZER = LOCAL diff --git a/pkg/sql/logictest/testdata/logic_test/statement_statistics b/pkg/sql/logictest/testdata/logic_test/statement_statistics index 475dd76b5699..c30a1a828d3d 100644 --- a/pkg/sql/logictest/testdata/logic_test/statement_statistics +++ b/pkg/sql/logictest/testdata/logic_test/statement_statistics @@ -123,10 +123,10 @@ SELECT x FROM test WHERE y IN (_, _, _ + x, _, _) · SELECT x FROM test WHERE y IN (_, _, __more3__) · SELECT x FROM test WHERE y IN (_, _, __more3__) + SELECT x FROM test WHERE y NOT IN (_, _, __more3__) · -SET CLUSTER SETTING "debug.panic_on_failed_assertions" = DEFAULT - -SET CLUSTER SETTING "debug.panic_on_failed_assertions" = _ - -SET application_name = _ - -SET distsql = "on" - +SET CLUSTER SETTING "debug.panic_on_failed_assertions" = DEFAULT · +SET CLUSTER SETTING "debug.panic_on_failed_assertions" = _ · +SET application_name = _ · +SET distsql = "on" · SHOW CLUSTER SETTING "debug.panic_on_failed_assertions" · SHOW application_name · diff --git a/pkg/sql/metric_test.go b/pkg/sql/metric_test.go index d479685e73bc..f2469a022820 100644 --- a/pkg/sql/metric_test.go +++ b/pkg/sql/metric_test.go @@ -67,7 +67,7 @@ func TestQueryCounts(t *testing.T) { var testcases = []queryCounter{ // The counts are deltas for each query. - {query: "SET OPTIMIZER = 'off'", miscCount: 1, miscExecutedCount: 1, fallbackCount: 1}, + {query: "SET OPTIMIZER = 'off'", miscCount: 1, miscExecutedCount: 1, optCount: 1}, {query: "SET DISTSQL = 'off'", miscCount: 1, miscExecutedCount: 1}, {query: "BEGIN; END", txnBeginCount: 1, txnCommitCount: 1}, {query: "SELECT 1", selectCount: 1, selectExecutedCount: 1, txnCommitCount: 1}, @@ -105,7 +105,7 @@ func TestQueryCounts(t *testing.T) { {query: "SELECT 3", selectCount: 1, selectExecutedCount: 1, optCount: 1}, {query: "CREATE TABLE mt.n (num INTEGER PRIMARY KEY)", ddlCount: 1, optCount: 1}, {query: "UPDATE mt.n SET num = num + 1", updateCount: 1, optCount: 1}, - {query: "SET OPTIMIZER = 'off'", miscCount: 1, miscExecutedCount: 1, fallbackCount: 1}, + {query: "SET OPTIMIZER = 'off'", miscCount: 1, miscExecutedCount: 1, optCount: 1}, } accum := initializeQueryCounter(s) diff --git a/pkg/sql/opaque.go b/pkg/sql/opaque.go index 80e08197e370..6d20158bf08f 100644 --- a/pkg/sql/opaque.go +++ b/pkg/sql/opaque.go @@ -16,6 +16,8 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/opt" "github.com/cockroachdb/cockroach/pkg/sql/opt/optbuilder" + "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode" + "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror" "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" "github.com/cockroachdb/cockroach/pkg/sql/sqlbase" "github.com/cockroachdb/errors" @@ -39,24 +41,92 @@ func buildOpaque( var plan planNode var err error switch n := stmt.(type) { + case *tree.AlterIndex: + plan, err = p.AlterIndex(ctx, n) + case *tree.AlterTable: + plan, err = p.AlterTable(ctx, n) + case *tree.AlterSequence: + plan, err = p.AlterSequence(ctx, n) + case *tree.AlterUserSetPassword: + plan, err = p.AlterUserSetPassword(ctx, n) + case *tree.CommentOnColumn: + plan, err = p.CommentOnColumn(ctx, n) + case *tree.CommentOnDatabase: + plan, err = p.CommentOnDatabase(ctx, n) + case *tree.CommentOnTable: + plan, err = p.CommentOnTable(ctx, n) + case *tree.CreateDatabase: + plan, err = p.CreateDatabase(ctx, n) + case *tree.CreateIndex: + plan, err = p.CreateIndex(ctx, n) + case *tree.CreateUser: + plan, err = p.CreateUser(ctx, n) + case *tree.CreateSequence: + plan, err = p.CreateSequence(ctx, n) + case *tree.CreateStats: + plan, err = p.CreateStatistics(ctx, n) + case *tree.Deallocate: + plan, err = p.Deallocate(ctx, n) + case *tree.Discard: + plan, err = p.Discard(ctx, n) + case *tree.DropDatabase: + plan, err = p.DropDatabase(ctx, n) + case *tree.DropIndex: + plan, err = p.DropIndex(ctx, n) + case *tree.DropTable: + plan, err = p.DropTable(ctx, n) + case *tree.DropView: + plan, err = p.DropView(ctx, n) + case *tree.DropSequence: + plan, err = p.DropSequence(ctx, n) + case *tree.DropUser: + plan, err = p.DropUser(ctx, n) + case *tree.Grant: + plan, err = p.Grant(ctx, n) + case *tree.RenameColumn: + plan, err = p.RenameColumn(ctx, n) + case *tree.RenameDatabase: + plan, err = p.RenameDatabase(ctx, n) + case *tree.RenameIndex: + plan, err = p.RenameIndex(ctx, n) + case *tree.RenameTable: + plan, err = p.RenameTable(ctx, n) + case *tree.Revoke: + plan, err = p.Revoke(ctx, n) + case *tree.Scatter: + plan, err = p.Scatter(ctx, n) + case *tree.Scrub: + plan, err = p.Scrub(ctx, n) + case *tree.SetClusterSetting: + plan, err = p.SetClusterSetting(ctx, n) + case *tree.SetZoneConfig: + plan, err = p.SetZoneConfig(ctx, n) + case *tree.SetVar: + plan, err = p.SetVar(ctx, n) + case *tree.SetTransaction: + plan, err = p.SetTransaction(n) + case *tree.SetSessionCharacteristics: + plan, err = p.SetSessionCharacteristics(n) case *tree.ShowClusterSetting: plan, err = p.ShowClusterSetting(ctx, n) - case *tree.ShowHistogram: plan, err = p.ShowHistogram(ctx, n) - case *tree.ShowTableStats: plan, err = p.ShowTableStats(ctx, n) - case *tree.ShowTraceForSession: plan, err = p.ShowTrace(ctx, n) - case *tree.ShowZoneConfig: plan, err = p.ShowZoneConfig(ctx, n) - case *tree.ShowFingerprints: plan, err = p.ShowFingerprints(ctx, n) - + case *tree.Truncate: + plan, err = p.Truncate(ctx, n) + case tree.CCLOnlyStatement: + plan, err = p.maybePlanHook(ctx, stmt) + if plan == nil && err == nil { + return nil, nil, pgerror.Newf(pgcode.CCLRequired, + "a CCL binary is required to use this statement type: %T", stmt) + } default: return nil, nil, errors.AssertionFailedf("unknown opaque statement %T", stmt) } @@ -71,15 +141,63 @@ func buildOpaque( } func init() { - opaqueReadOnlyStmts := []reflect.Type{ - reflect.TypeOf(&tree.ShowClusterSetting{}), - reflect.TypeOf(&tree.ShowHistogram{}), - reflect.TypeOf(&tree.ShowTableStats{}), - reflect.TypeOf(&tree.ShowTraceForSession{}), - reflect.TypeOf(&tree.ShowZoneConfig{}), - reflect.TypeOf(&tree.ShowFingerprints{}), - } - for _, t := range opaqueReadOnlyStmts { - optbuilder.RegisterOpaque(t, optbuilder.OpaqueReadOnly, buildOpaque) + for _, stmt := range []tree.Statement{ + &tree.AlterUserSetPassword{}, + &tree.AlterIndex{}, + &tree.AlterTable{}, + &tree.AlterSequence{}, + &tree.CommentOnColumn{}, + &tree.CommentOnDatabase{}, + &tree.CommentOnTable{}, + &tree.CreateDatabase{}, + &tree.CreateIndex{}, + &tree.CreateUser{}, + &tree.CreateSequence{}, + &tree.CreateStats{}, + &tree.Deallocate{}, + &tree.Discard{}, + &tree.DropDatabase{}, + &tree.DropIndex{}, + &tree.DropTable{}, + &tree.DropView{}, + &tree.DropSequence{}, + &tree.DropUser{}, + &tree.Grant{}, + &tree.RenameColumn{}, + &tree.RenameDatabase{}, + &tree.RenameIndex{}, + &tree.RenameTable{}, + &tree.Revoke{}, + &tree.Scatter{}, + &tree.Scrub{}, + &tree.SetClusterSetting{}, + &tree.SetZoneConfig{}, + &tree.SetVar{}, + &tree.SetTransaction{}, + &tree.SetSessionCharacteristics{}, + &tree.ShowClusterSetting{}, + &tree.ShowHistogram{}, + &tree.ShowTableStats{}, + &tree.ShowTraceForSession{}, + &tree.ShowZoneConfig{}, + &tree.ShowFingerprints{}, + &tree.Truncate{}, + + // CCL statements. + &tree.Backup{}, + &tree.Restore{}, + &tree.CreateChangefeed{}, + &tree.CreateRole{}, + &tree.DropRole{}, + &tree.GrantRole{}, + &tree.RevokeRole{}, + } { + typ := optbuilder.OpaqueReadOnly + if tree.CanModifySchema(stmt) { + typ = optbuilder.OpaqueDDL + } else if tree.CanWriteData(stmt) { + typ = optbuilder.OpaqueMutation + } + optbuilder.RegisterOpaque(reflect.TypeOf(stmt), typ, buildOpaque) } } diff --git a/pkg/sql/opt/exec/execbuilder/relational.go b/pkg/sql/opt/exec/execbuilder/relational.go index 6f8e7245e84a..b70f37b7b568 100644 --- a/pkg/sql/opt/exec/execbuilder/relational.go +++ b/pkg/sql/opt/exec/execbuilder/relational.go @@ -169,8 +169,12 @@ func (b *Builder) buildRelational(e memo.RelExpr) (execPlan, error) { var saveTableName string if b.nameGen != nil { - // This function must be called in a pre-order traversal of the tree. - saveTableName = b.nameGen.GenerateName(e.Op()) + // Don't save tables for operators that don't produce any columns (most + // importantly, for SET which is used to disable saving of tables). + if !e.Relational().OutputCols.Empty() { + // This function must be called in a pre-order traversal of the tree. + saveTableName = b.nameGen.GenerateName(e.Op()) + } } // Handle read-only operators which never write data or modify schema. diff --git a/pkg/sql/opt_exec_factory.go b/pkg/sql/opt_exec_factory.go index 0214bfb3e053..3665a39ab47d 100644 --- a/pkg/sql/opt_exec_factory.go +++ b/pkg/sql/opt_exec_factory.go @@ -888,8 +888,11 @@ func (ef *execFactory) ConstructPlan( root = spool.source } res := &planTop{ - plan: root.(planNode), - auditEvents: ef.planner.curPlan.auditEvents, + plan: root.(planNode), + // TODO(radu): these fields can be modified by planning various opaque + // statements. We should have a cleaner way of plumbing these. + avoidBuffering: ef.planner.curPlan.avoidBuffering, + auditEvents: ef.planner.curPlan.auditEvents, } if len(subqueries) > 0 { res.subqueryPlans = make([]subquery, len(subqueries)) diff --git a/pkg/sql/opt_filters.go b/pkg/sql/opt_filters.go index fa06e2e70ea4..a2935485acca 100644 --- a/pkg/sql/opt_filters.go +++ b/pkg/sql/opt_filters.go @@ -385,6 +385,7 @@ func (p *planner) propagateFilters( case *commentOnColumnNode: case *commentOnDatabaseNode: case *commentOnTableNode: + case *changePrivilegesNode: case *createDatabaseNode: case *createIndexNode: case *CreateUserNode: diff --git a/pkg/sql/opt_limits.go b/pkg/sql/opt_limits.go index 9ce4aa2a90b5..cfaacd31b297 100644 --- a/pkg/sql/opt_limits.go +++ b/pkg/sql/opt_limits.go @@ -217,6 +217,7 @@ func (p *planner) applyLimit(plan planNode, numRows int64, soft bool) { case *renameTableNode: case *scrubNode: case *truncateNode: + case *changePrivilegesNode: case *commentOnColumnNode: case *commentOnDatabaseNode: case *commentOnTableNode: diff --git a/pkg/sql/opt_needed.go b/pkg/sql/opt_needed.go index 30097d267b18..37b60e4d4a96 100644 --- a/pkg/sql/opt_needed.go +++ b/pkg/sql/opt_needed.go @@ -270,6 +270,7 @@ func setNeededColumns(plan planNode, needed []bool) { case *renameTableNode: case *scrubNode: case *truncateNode: + case *changePrivilegesNode: case *commentOnColumnNode: case *commentOnDatabaseNode: case *commentOnTableNode: diff --git a/pkg/sql/plan.go b/pkg/sql/plan.go index 97526f6fc8c8..e37e2b264db2 100644 --- a/pkg/sql/plan.go +++ b/pkg/sql/plan.go @@ -171,6 +171,7 @@ var _ planNode = &alterTableNode{} var _ planNode = &bufferNode{} var _ planNode = &cancelQueriesNode{} var _ planNode = &cancelSessionsNode{} +var _ planNode = &changePrivilegesNode{} var _ planNode = &createDatabaseNode{} var _ planNode = &createIndexNode{} var _ planNode = &createSequenceNode{} diff --git a/pkg/sql/plan_opt.go b/pkg/sql/plan_opt.go index 76a96b9223cb..a452688bcf1a 100644 --- a/pkg/sql/plan_opt.go +++ b/pkg/sql/plan_opt.go @@ -50,6 +50,32 @@ func (p *planner) prepareUsingOptimizer( opc := &p.optPlanningCtx opc.reset() + // These statements do not have result columns and do not support placeholders + // so there is no need to do anything during prepare. + // + // Some of these statements (like BeginTransaction) aren't supported by the + // optbuilder so they would error out. Others (like CreateIndex) have planning + // code that can introduce unnecessary txn retries (because of looking up + // descriptors and such). + switch stmt.AST.(type) { + case *tree.AlterIndex, *tree.AlterTable, *tree.AlterSequence, + *tree.BeginTransaction, + *tree.CommentOnColumn, *tree.CommentOnDatabase, *tree.CommentOnTable, *tree.CommitTransaction, + *tree.CopyFrom, *tree.CreateDatabase, *tree.CreateIndex, *tree.CreateView, + *tree.CreateSequence, + *tree.CreateStats, + *tree.Deallocate, *tree.Discard, *tree.DropDatabase, *tree.DropIndex, + *tree.DropTable, *tree.DropView, *tree.DropSequence, *tree.DropRole, + *tree.Execute, + *tree.Grant, *tree.GrantRole, + *tree.Prepare, + *tree.ReleaseSavepoint, *tree.RenameColumn, *tree.RenameDatabase, + *tree.RenameIndex, *tree.RenameTable, *tree.Revoke, *tree.RevokeRole, + *tree.RollbackToSavepoint, *tree.RollbackTransaction, + *tree.Savepoint, *tree.SetTransaction, *tree.SetTracing, *tree.SetSessionCharacteristics: + return opc.flags, false, nil + } + if opc.useCache { cachedData, ok := p.execCfg.QueryCache.Find(&p.queryCacheSession, stmt.SQL) if ok && cachedData.PrepareMetadata != nil { diff --git a/pkg/sql/plan_physical_props.go b/pkg/sql/plan_physical_props.go index f9158ee5f0f7..be83c5472e79 100644 --- a/pkg/sql/plan_physical_props.go +++ b/pkg/sql/plan_physical_props.go @@ -101,6 +101,7 @@ func planPhysicalProps(plan planNode) physicalProps { case *alterUserSetPasswordNode: case *cancelQueriesNode: case *cancelSessionsNode: + case *changePrivilegesNode: case *commentOnTableNode: case *commentOnColumnNode: case *commentOnDatabaseNode: diff --git a/pkg/sql/schema_changer_test.go b/pkg/sql/schema_changer_test.go index 678dbef03a4f..c147c3044d38 100644 --- a/pkg/sql/schema_changer_test.go +++ b/pkg/sql/schema_changer_test.go @@ -2585,29 +2585,59 @@ INSERT INTO t.kv VALUES ('a', 'b'); expectedErr string }{ // DROP TABLE followed by CREATE TABLE case. - {`drop-create`, `DROP TABLE t.kv`, `CREATE TABLE t.kv (k CHAR PRIMARY KEY, v CHAR)`, - `relation "kv" already exists`}, + { + name: `drop-create`, + firstStmt: `DROP TABLE t.kv`, + secondStmt: `CREATE TABLE t.kv (k CHAR PRIMARY KEY, v CHAR)`, + expectedErr: `relation "kv" already exists`, + }, // schema change followed by another statement works. - {`createindex-insert`, `CREATE INDEX foo ON t.kv (v)`, `INSERT INTO t.kv VALUES ('c', 'd')`, - ``}, + { + name: `createindex-insert`, + firstStmt: `CREATE INDEX foo ON t.kv (v)`, + secondStmt: `INSERT INTO t.kv VALUES ('c', 'd')`, + expectedErr: ``, + }, // CREATE TABLE followed by INSERT works. - {`createtable-insert`, `CREATE TABLE t.origin (k CHAR PRIMARY KEY, v CHAR);`, - `INSERT INTO t.origin VALUES ('c', 'd')`, ``}, + { + name: `createtable-insert`, + firstStmt: `CREATE TABLE t.origin (k CHAR PRIMARY KEY, v CHAR);`, + secondStmt: `INSERT INTO t.origin VALUES ('c', 'd')`, + expectedErr: ``}, // Support multiple schema changes for ORMs: #15269 // Support insert into another table after schema changes: #15297 - {`multiple-schema-change`, - `CREATE TABLE t.orm1 (k CHAR PRIMARY KEY, v CHAR); CREATE TABLE t.orm2 (k CHAR PRIMARY KEY, v CHAR);`, - `CREATE INDEX foo ON t.orm1 (v); CREATE INDEX foo ON t.orm2 (v); INSERT INTO t.origin VALUES ('e', 'f')`, - ``}, + { + name: `multiple-schema-change`, + firstStmt: `CREATE TABLE t.orm1 (k CHAR PRIMARY KEY, v CHAR); CREATE TABLE t.orm2 (k CHAR PRIMARY KEY, v CHAR);`, + secondStmt: `CREATE INDEX foo ON t.orm1 (v); CREATE INDEX foo ON t.orm2 (v); INSERT INTO t.origin VALUES ('e', 'f')`, + expectedErr: ``, + }, // schema change at the end of a transaction that has written. - {`insert-create`, `INSERT INTO t.kv VALUES ('e', 'f')`, `CREATE INDEX foo ON t.kv (v)`, - `schema change statement cannot follow a statement that has written in the same transaction`}, + { + name: `insert-create`, + firstStmt: `INSERT INTO t.kv VALUES ('e', 'f')`, + secondStmt: `CREATE INDEX foo2 ON t.kv (v)`, + expectedErr: `schema change statement cannot follow a statement that has written in the same transaction`, + }, // schema change at the end of a read only transaction. - {`select-create`, `SELECT * FROM t.kv`, `CREATE INDEX bar ON t.kv (v)`, ``}, - {`index-on-add-col`, `ALTER TABLE t.kv ADD i INT`, - `CREATE INDEX foobar ON t.kv (i)`, ``}, - {`check-on-add-col`, `ALTER TABLE t.kv ADD j INT`, - `ALTER TABLE t.kv ADD CONSTRAINT ck_j CHECK (j >= 0)`, ``}, + { + name: `select-create`, + firstStmt: `SELECT * FROM t.kv`, + secondStmt: `CREATE INDEX bar ON t.kv (v)`, + expectedErr: ``, + }, + { + name: `index-on-add-col`, + firstStmt: `ALTER TABLE t.kv ADD i INT`, + secondStmt: `CREATE INDEX foobar ON t.kv (i)`, + expectedErr: ``, + }, + { + name: `check-on-add-col`, + firstStmt: `ALTER TABLE t.kv ADD j INT`, + secondStmt: `ALTER TABLE t.kv ADD CONSTRAINT ck_j CHECK (j >= 0)`, + expectedErr: ``, + }, } for _, testCase := range testCases { diff --git a/pkg/sql/sem/tree/stmt.go b/pkg/sql/sem/tree/stmt.go index e066ec149f4b..50dc0efc30cd 100644 --- a/pkg/sql/sem/tree/stmt.go +++ b/pkg/sql/sem/tree/stmt.go @@ -168,6 +168,7 @@ var _ CCLOnlyStatement = &CreateRole{} var _ CCLOnlyStatement = &DropRole{} var _ CCLOnlyStatement = &GrantRole{} var _ CCLOnlyStatement = &RevokeRole{} +var _ CCLOnlyStatement = &CreateChangefeed{} // StatementType implements the Statement interface. func (*AlterIndex) StatementType() StatementType { return DDL } @@ -282,6 +283,8 @@ func (n *CreateChangefeed) StatementTag() string { return "CREATE CHANGEFEED" } +func (*CreateChangefeed) cclOnlyStatement() {} + // StatementType implements the Statement interface. func (*CreateDatabase) StatementType() StatementType { return DDL } diff --git a/pkg/sql/walk.go b/pkg/sql/walk.go index 883d45345a9e..4fb910123165 100644 --- a/pkg/sql/walk.go +++ b/pkg/sql/walk.go @@ -757,11 +757,12 @@ var planNodeNames = map[reflect.Type]string{ reflect.TypeOf(&alterUserSetPasswordNode{}): "alter user", reflect.TypeOf(&applyJoinNode{}): "apply-join", reflect.TypeOf(&bufferNode{}): "buffer node", + reflect.TypeOf(&cancelQueriesNode{}): "cancel queries", + reflect.TypeOf(&cancelSessionsNode{}): "cancel sessions", + reflect.TypeOf(&changePrivilegesNode{}): "change privileges", reflect.TypeOf(&commentOnColumnNode{}): "comment on column", reflect.TypeOf(&commentOnDatabaseNode{}): "comment on database", reflect.TypeOf(&commentOnTableNode{}): "comment on table", - reflect.TypeOf(&cancelQueriesNode{}): "cancel queries", - reflect.TypeOf(&cancelSessionsNode{}): "cancel sessions", reflect.TypeOf(&controlJobsNode{}): "control jobs", reflect.TypeOf(&createDatabaseNode{}): "create database", reflect.TypeOf(&createIndexNode{}): "create index",