From a4f34e779d039357f8ecd1bbdfc1c39f48954bd5 Mon Sep 17 00:00:00 2001 From: Faizan Qazi Date: Fri, 21 Jul 2023 10:28:24 -0400 Subject: [PATCH] sql: add internal function to detect keys in system tables Previously, we had no easy way of knowing if a key belonged to a system table. This was inadequate because we can improve our redacted debug zips by including contention information involving these tables. This patch will add a new builtin crdb_internal.is_system_table_key which can be used for conditional redaction in SQL. Release note: None --- pkg/sql/faketreeeval/evalctx.go | 5 +++++ pkg/sql/privileged_accessor.go | 10 ++++++++++ pkg/sql/sem/builtins/builtins.go | 32 ++++++++++++++++++++++++++++++ pkg/sql/sem/builtins/fixed_oids.go | 1 + pkg/sql/sem/eval/deps.go | 3 +++ 5 files changed, 51 insertions(+) diff --git a/pkg/sql/faketreeeval/evalctx.go b/pkg/sql/faketreeeval/evalctx.go index 6a3e1dd25216..7274368b78f7 100644 --- a/pkg/sql/faketreeeval/evalctx.go +++ b/pkg/sql/faketreeeval/evalctx.go @@ -535,6 +535,11 @@ func (ep *DummyPrivilegedAccessor) LookupZoneConfigByNamespaceID( return "", false, errors.WithStack(errEvalPrivileged) } +// IsSystemTable is part of the tree.PrivilegedAccessor interface. +func (ep *DummyPrivilegedAccessor) IsSystemTable(ctx context.Context, id int64) (bool, error) { + return false, errors.WithStack(errEvalPrivileged) +} + // DummySessionAccessor implements the eval.SessionAccessor interface by returning errors. type DummySessionAccessor struct{} diff --git a/pkg/sql/privileged_accessor.go b/pkg/sql/privileged_accessor.go index 8f8dc9648942..109c95adaf91 100644 --- a/pkg/sql/privileged_accessor.go +++ b/pkg/sql/privileged_accessor.go @@ -19,6 +19,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb" "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode" "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror" + "github.com/cockroachdb/cockroach/pkg/sql/sem/catid" "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" "github.com/cockroachdb/cockroach/pkg/sql/sessiondata" "github.com/cockroachdb/errors" @@ -76,6 +77,15 @@ func (p *planner) LookupZoneConfigByNamespaceID( return tree.DBytes(zc.GetRawBytesInStorage()), true, nil } +// IsSystemTable implements tree.PrivilegedAccessor. +func (p *planner) IsSystemTable(ctx context.Context, id int64) (bool, error) { + tbl, err := p.Descriptors().ByID(p.Txn()).Get().Table(ctx, catid.DescID(id)) + if err != nil { + return false, err + } + return catalog.IsSystemDescriptor(tbl), nil +} + // checkDescriptorPermissions returns nil if the executing user has permissions // to check the permissions of a descriptor given its ID, or the id given // is not a descriptor of a table or database. diff --git a/pkg/sql/sem/builtins/builtins.go b/pkg/sql/sem/builtins/builtins.go index 667d0f125016..bff0204d8db1 100644 --- a/pkg/sql/sem/builtins/builtins.go +++ b/pkg/sql/sem/builtins/builtins.go @@ -5728,6 +5728,38 @@ SELECT Volatility: volatility.Immutable, }, ), + // Return if a key belongs to a system table, which should make it to print + // within redacted output. + "crdb_internal.is_system_table_key": makeBuiltin( + tree.FunctionProperties{ + Category: builtinconstants.CategorySystemInfo, + Undocumented: true, + }, + tree.Overload{ + Types: tree.ParamTypes{ + {Name: "raw_key", Typ: types.Bytes}, + }, + ReturnType: tree.FixedReturnType(types.Bool), + Fn: func(ctx context.Context, evalCtx *eval.Context, args tree.Datums) (tree.Datum, error) { + _, tableID, err := evalCtx.Codec.DecodeTablePrefix(roachpb.Key(tree.MustBeDBytes(args[0]))) + if err != nil { + // If a key isn't prefixed with a table ID ignore. + //nolint:returnerrcheck + return tree.DBoolFalse, nil + } + isSystemTable, err := evalCtx.PrivilegedAccessor.IsSystemTable(ctx, int64(tableID)) + if err != nil { + // If we can't find the descriptor or its not the right type then its + // not a system table. + //nolint:returnerrcheck + return tree.DBoolFalse, nil + } + return tree.MakeDBool(tree.DBool(isSystemTable)), nil + }, + Info: "This function is used only by CockroachDB's developers for testing purposes.", + Volatility: volatility.Stable, + }, + ), // Return a pretty string for a given span, skipping the specified number of // fields. diff --git a/pkg/sql/sem/builtins/fixed_oids.go b/pkg/sql/sem/builtins/fixed_oids.go index 26486974f306..47d98a8c2a7d 100644 --- a/pkg/sql/sem/builtins/fixed_oids.go +++ b/pkg/sql/sem/builtins/fixed_oids.go @@ -2439,6 +2439,7 @@ var builtinOidsArray = []string{ 2466: `crdb_internal.setup_span_configs_stream(tenant_name: string) -> bytes`, 2467: `crdb_internal.request_statement_bundle(stmtFingerprint: string, planGist: string, samplingProbability: float, minExecutionLatency: interval, expiresAfter: interval) -> bool`, 2468: `crdb_internal.request_statement_bundle(stmtFingerprint: string, planGist: string, antiPlanGist: bool, samplingProbability: float, minExecutionLatency: interval, expiresAfter: interval) -> bool`, + 2469: `crdb_internal.is_system_table_key(raw_key: bytes) -> bool`, } var builtinOidsBySignature map[string]oid.Oid diff --git a/pkg/sql/sem/eval/deps.go b/pkg/sql/sem/eval/deps.go index 21afbc763537..e5ab63c2bd91 100644 --- a/pkg/sql/sem/eval/deps.go +++ b/pkg/sql/sem/eval/deps.go @@ -534,6 +534,9 @@ type PrivilegedAccessor interface { // Returns the config byte array, a bool representing whether the namespace exists, // and an error if there is one. LookupZoneConfigByNamespaceID(ctx context.Context, id int64) (tree.DBytes, bool, error) + + // IsSystemTable returns if a given descriptor ID is a system table.s + IsSystemTable(ctx context.Context, id int64) (bool, error) } // RegionOperator gives access to the current region, validation for all