From 3fcafa8213237f20da583abf79fd7fcfdcbdece3 Mon Sep 17 00:00:00 2001 From: Raphael 'kena' Poss Date: Fri, 11 Sep 2020 12:48:18 +0200 Subject: [PATCH] sql: make multitenancy errors report to telemetry This change ensures that multitenancy errors get collected in telemetry. By also making the issue number non-optional, this patch also catches a few more cases that were missing open issues. Release note: None --- pkg/sql/opt_exec_factory.go | 8 ++--- pkg/sql/scatter.go | 2 +- pkg/sql/sem/builtins/generator_builtins.go | 3 +- pkg/sql/show_zone_config.go | 2 +- pkg/util/errorutil/tenant.go | 30 +++++++------------ .../errorutil/tenant_deprecated_wrapper.go | 5 ---- 6 files changed, 19 insertions(+), 31 deletions(-) diff --git a/pkg/sql/opt_exec_factory.go b/pkg/sql/opt_exec_factory.go index 9c53ff66a19d..775a7f27806a 100644 --- a/pkg/sql/opt_exec_factory.go +++ b/pkg/sql/opt_exec_factory.go @@ -1692,7 +1692,7 @@ func (ef *execFactory) ConstructAlterTableSplit( index cat.Index, input exec.Node, expiration tree.TypedExpr, ) (exec.Node, error) { if !ef.planner.ExecCfg().Codec.ForSystemTenant() { - return nil, errorutil.UnsupportedWithMultiTenancy() + return nil, errorutil.UnsupportedWithMultiTenancy(54254) } expirationTime, err := parseExpirationTime(ef.planner.EvalContext(), expiration) @@ -1713,7 +1713,7 @@ func (ef *execFactory) ConstructAlterTableUnsplit( index cat.Index, input exec.Node, ) (exec.Node, error) { if !ef.planner.ExecCfg().Codec.ForSystemTenant() { - return nil, errorutil.UnsupportedWithMultiTenancy() + return nil, errorutil.UnsupportedWithMultiTenancy(54254) } return &unsplitNode{ @@ -1726,7 +1726,7 @@ func (ef *execFactory) ConstructAlterTableUnsplit( // ConstructAlterTableUnsplitAll is part of the exec.Factory interface. func (ef *execFactory) ConstructAlterTableUnsplitAll(index cat.Index) (exec.Node, error) { if !ef.planner.ExecCfg().Codec.ForSystemTenant() { - return nil, errorutil.UnsupportedWithMultiTenancy() + return nil, errorutil.UnsupportedWithMultiTenancy(54254) } return &unsplitAllNode{ @@ -1740,7 +1740,7 @@ func (ef *execFactory) ConstructAlterTableRelocate( index cat.Index, input exec.Node, relocateLease bool, ) (exec.Node, error) { if !ef.planner.ExecCfg().Codec.ForSystemTenant() { - return nil, errorutil.UnsupportedWithMultiTenancy() + return nil, errorutil.UnsupportedWithMultiTenancy(54250) } return &relocateNode{ diff --git a/pkg/sql/scatter.go b/pkg/sql/scatter.go index fd7dafd999a0..b6205f31f194 100644 --- a/pkg/sql/scatter.go +++ b/pkg/sql/scatter.go @@ -34,7 +34,7 @@ type scatterNode struct { // Privileges: INSERT on table. func (p *planner) Scatter(ctx context.Context, n *tree.Scatter) (planNode, error) { if !p.ExecCfg().Codec.ForSystemTenant() { - return nil, errorutil.UnsupportedWithMultiTenancy() + return nil, errorutil.UnsupportedWithMultiTenancy(54255) } tableDesc, index, err := p.getTableAndIndex(ctx, &n.TableOrIndex, privilege.INSERT) diff --git a/pkg/sql/sem/builtins/generator_builtins.go b/pkg/sql/sem/builtins/generator_builtins.go index dacdb0a3d336..47d8a9aa4bc9 100644 --- a/pkg/sql/sem/builtins/generator_builtins.go +++ b/pkg/sql/sem/builtins/generator_builtins.go @@ -1129,7 +1129,8 @@ func makeCheckConsistencyGenerator( ctx *tree.EvalContext, args tree.Datums, ) (tree.ValueGenerator, error) { if !ctx.Codec.ForSystemTenant() { - return nil, errorutil.UnsupportedWithMultiTenancy() + return nil, errorutil.UnsupportedWithMultiTenancy( + errorutil.FeatureNotAvailableToNonSystemTenantsIssue) } keyFrom := roachpb.Key(*args[1].(*tree.DBytes)) diff --git a/pkg/sql/show_zone_config.go b/pkg/sql/show_zone_config.go index 778ad3086a1d..cc21da8aba4c 100644 --- a/pkg/sql/show_zone_config.go +++ b/pkg/sql/show_zone_config.go @@ -64,7 +64,7 @@ const ( func (p *planner) ShowZoneConfig(ctx context.Context, n *tree.ShowZoneConfig) (planNode, error) { if !p.ExecCfg().Codec.ForSystemTenant() { - return nil, errorutil.UnsupportedWithMultiTenancy() + return nil, errorutil.UnsupportedWithMultiTenancy(multitenancyZoneCfgIssueNo) } return &delayedNode{ diff --git a/pkg/util/errorutil/tenant.go b/pkg/util/errorutil/tenant.go index 27aac2cff6ed..1eb022966edd 100644 --- a/pkg/util/errorutil/tenant.go +++ b/pkg/util/errorutil/tenant.go @@ -10,28 +10,20 @@ package errorutil -import ( - "fmt" - "strings" - - "github.com/cockroachdb/errors" -) +import "github.com/cockroachdb/cockroach/pkg/util/errorutil/unimplemented" // UnsupportedWithMultiTenancy returns an error suitable for returning when an // operation could not be carried out due to the SQL server running in // multi-tenancy mode. In that mode, Gossip and other components of the KV layer // are not available. -func UnsupportedWithMultiTenancy(issues ...int) error { - var buf strings.Builder - buf.WriteString("operation is unsupported in multi-tenancy mode") - if len(issues) > 0 { - buf.WriteString("; see:\n") - const prefix = "\nhttps://github.com/cockroachdb/cockroach/issues/" - for _, n := range issues { - fmt.Fprint(&buf, prefix, n) - } - } - // TODO(knz): This should be done in a different way, - // see: https://github.com/cockroachdb/cockroach/issues/48164 - return errors.Newf("%s", buf.String()) +func UnsupportedWithMultiTenancy(issue int) error { + return unimplemented.NewWithIssue(issue, "operation is unsupported in multi-tenancy mode") } + +// FeatureNotAvailableToNonSystemTenantsIssue is to be used with the +// Optional and related error interfaces when a feature is simply not +// available to non-system tenants (i.e. we're not planning to change +// this). +// For all other multitenancy errors where there is a plan to +// improve the situation, a specific issue should be created instead. +const FeatureNotAvailableToNonSystemTenantsIssue = 54252 diff --git a/pkg/util/errorutil/tenant_deprecated_wrapper.go b/pkg/util/errorutil/tenant_deprecated_wrapper.go index 424d08828bc4..5d98755aca74 100644 --- a/pkg/util/errorutil/tenant_deprecated_wrapper.go +++ b/pkg/util/errorutil/tenant_deprecated_wrapper.go @@ -80,8 +80,3 @@ func (w TenantSQLDeprecatedWrapper) OptionalErr(issue int) (interface{}, error) } return v, nil } - -// FeatureNotAvailableToNonSystemTenantsIssue is to be used with the Optinal -// and related error interfaces when a feature is simply not available -// to non-system tenants (i.e. we're not planning to change this). -const FeatureNotAvailableToNonSystemTenantsIssue = 54252