From 2b4f1762e457a23223806ee8ae117c541a443e00 Mon Sep 17 00:00:00 2001 From: Aditya Maru Date: Thu, 11 Aug 2022 11:58:47 -0400 Subject: [PATCH 1/8] externalconn: add gs support to External Connections This change registers Google Storage `gs` as a supported External Connection. Release note (sql change): Users can now `CREATE EXTERNAL CONNECTION` to represent an underlying google storage resource. --- .../testdata/create_drop_external_connection | 33 +++ pkg/ccl/cloudccl/gcp/BUILD.bazel | 3 +- ...nection_test.go => gcp_connection_test.go} | 268 +++++++++++++++++- .../externalconn/connectionpb/connection.go | 2 +- .../connectionpb/connection.proto | 1 + pkg/cloud/gcp/BUILD.bazel | 1 + pkg/cloud/gcp/gcs_connection.go | 47 +++ pkg/cloud/gcp/gcs_storage.go | 18 +- 8 files changed, 362 insertions(+), 11 deletions(-) rename pkg/ccl/cloudccl/gcp/{gcp_kms_connection_test.go => gcp_connection_test.go} (53%) create mode 100644 pkg/cloud/gcp/gcs_connection.go diff --git a/pkg/ccl/cloudccl/externalconn/testdata/create_drop_external_connection b/pkg/ccl/cloudccl/externalconn/testdata/create_drop_external_connection index 8c581d3778c0..c3ee1fa200f1 100644 --- a/pkg/ccl/cloudccl/externalconn/testdata/create_drop_external_connection +++ b/pkg/ccl/cloudccl/externalconn/testdata/create_drop_external_connection @@ -269,3 +269,36 @@ DROP EXTERNAL CONNECTION "foo-userfile"; ---- subtest end + +subtest basic-gs + +disable-check-external-storage +---- + +exec-sql +CREATE EXTERNAL CONNECTION "foo-gs" AS 'gs://bucket/path?AUTH=implicit&ASSUME_ROLE=soccer,cricket,football' +---- + +# Reject invalid gs external connections. +exec-sql +CREATE EXTERNAL CONNECTION "invalid-param-gs" AS 'gs://bucket/path?INVALIDPARAM=baz' +---- +pq: failed to construct External Connection details: failed to create gs external connection: unknown GS query parameters: INVALIDPARAM + +exec-sql +CREATE EXTERNAL CONNECTION "invalid-creds-gs" AS 'gs://bucket/path?AUTH=specified&CREDENTIALS=123' +---- +pq: failed to construct External Connection details: failed to create gs external connection: error getting credentials from CREDENTIALS: illegal base64 data at input byte 0 + +inspect-system-table +---- +foo-gs STORAGE {"provider": "gs", "simpleUri": {"uri": "gs://bucket/path?AUTH=implicit&ASSUME_ROLE=soccer,cricket,football"}} + +exec-sql +DROP EXTERNAL CONNECTION "foo-gs"; +---- + +enable-check-external-storage +---- + +subtest end diff --git a/pkg/ccl/cloudccl/gcp/BUILD.bazel b/pkg/ccl/cloudccl/gcp/BUILD.bazel index 1536ecf1889b..2428da98b898 100644 --- a/pkg/ccl/cloudccl/gcp/BUILD.bazel +++ b/pkg/ccl/cloudccl/gcp/BUILD.bazel @@ -4,7 +4,7 @@ load("@io_bazel_rules_go//go:def.bzl", "go_test") go_test( name = "gcp_test", srcs = [ - "gcp_kms_connection_test.go", + "gcp_connection_test.go", "main_test.go", ], deps = [ @@ -30,6 +30,7 @@ go_test( "//pkg/util/randutil", "@com_github_stretchr_testify//require", "@com_google_cloud_go_kms//apiv1", + "@com_google_cloud_go_storage//:storage", "@org_golang_x_oauth2//google", ], ) diff --git a/pkg/ccl/cloudccl/gcp/gcp_kms_connection_test.go b/pkg/ccl/cloudccl/gcp/gcp_connection_test.go similarity index 53% rename from pkg/ccl/cloudccl/gcp/gcp_kms_connection_test.go rename to pkg/ccl/cloudccl/gcp/gcp_connection_test.go index 18fae5ac7078..3f3821d8257f 100644 --- a/pkg/ccl/cloudccl/gcp/gcp_kms_connection_test.go +++ b/pkg/ccl/cloudccl/gcp/gcp_connection_test.go @@ -18,6 +18,7 @@ import ( "testing" kms "cloud.google.com/go/kms/apiv1" + gcs "cloud.google.com/go/storage" "github.com/cockroachdb/cockroach/pkg/base" _ "github.com/cockroachdb/cockroach/pkg/ccl" "github.com/cockroachdb/cockroach/pkg/cloud" @@ -35,7 +36,7 @@ import ( "golang.org/x/oauth2/google" ) -func TestGCSKMSExternalConnection(t *testing.T) { +func TestGCPKMSExternalConnection(t *testing.T) { defer leaktest.AfterTest(t)() defer log.Scope(t).Close(t) @@ -79,8 +80,18 @@ func TestGCSKMSExternalConnection(t *testing.T) { skip.IgnoreLint(t, "GOOGLE_KMS_KEY_NAME env var must be set") } + bucket := os.Getenv("GOOGLE_BUCKET") + if bucket == "" { + skip.IgnoreLint(t, "GOOGLE_BUCKET env var must be set") + } + + if !cloudtestutils.IsImplicitAuthConfigured() { + skip.IgnoreLint(t, "implicit auth is not configured") + } + // Create an external connection where we will write the backup. - backupURI := "nodelocal://1/backup" + backupURI := fmt.Sprintf("gs://%s/backup?%s=%s", bucket, + cloud.AuthParam, cloud.AuthParamImplicit) backupExternalConnectionName := "backup" createExternalConnection(backupExternalConnectionName, backupURI) @@ -153,7 +164,7 @@ func TestGCSKMSExternalConnection(t *testing.T) { }) } -func TestGCSExternalConnectionAssumeRole(t *testing.T) { +func TestGCPKMSExternalConnectionAssumeRole(t *testing.T) { defer leaktest.AfterTest(t)() defer log.Scope(t).Close(t) @@ -209,8 +220,18 @@ func TestGCSExternalConnectionAssumeRole(t *testing.T) { assumedAccount := os.Getenv("ASSUME_SERVICE_ACCOUNT") encodedCredentials := base64.StdEncoding.EncodeToString([]byte(os.Getenv("GOOGLE_CREDENTIALS_JSON"))) + bucket := os.Getenv("GOOGLE_BUCKET") + if bucket == "" { + skip.IgnoreLint(t, "GOOGLE_BUCKET env var must be set") + } + + if !cloudtestutils.IsImplicitAuthConfigured() { + skip.IgnoreLint(t, "implicit auth is not configured") + } + // Create an external connection where we will write the backup. - backupURI := "nodelocal://1/backup" + backupURI := fmt.Sprintf("gs://%s/backup?%s=%s", bucket, + cloud.AuthParam, cloud.AuthParamImplicit) backupExternalConnectionName := "backup" createExternalConnection(backupExternalConnectionName, backupURI) @@ -269,3 +290,242 @@ func TestGCSExternalConnectionAssumeRole(t *testing.T) { backupAndRestoreFromExternalConnection(backupExternalConnectionName, "auth-assume-role-chaining") }) } + +func TestGCPAssumeRoleExternalConnection(t *testing.T) { + defer leaktest.AfterTest(t)() + defer log.Scope(t).Close(t) + + dir, dirCleanupFn := testutils.TempDir(t) + defer dirCleanupFn() + + params := base.TestClusterArgs{} + params.ServerArgs.ExternalIODir = dir + + tc := testcluster.StartTestCluster(t, 1, params) + defer tc.Stopper().Stop(context.Background()) + + tc.WaitForNodeLiveness(t) + sqlDB := sqlutils.MakeSQLRunner(tc.Conns[0]) + + // Setup some dummy data. + sqlDB.Exec(t, `CREATE DATABASE foo`) + sqlDB.Exec(t, `USE foo`) + sqlDB.Exec(t, `CREATE TABLE foo (id INT PRIMARY KEY)`) + sqlDB.Exec(t, `INSERT INTO foo VALUES (1), (2), (3)`) + + disallowedCreateExternalConnection := func(externalConnectionName, uri string) { + sqlDB.ExpectErr(t, "(PermissionDenied|AccessDenied|PERMISSION_DENIED|does not have storage.objects.create access)", + fmt.Sprintf(`CREATE EXTERNAL CONNECTION '%s' AS '%s'`, externalConnectionName, uri)) + } + createExternalConnection := func(externalConnectionName, uri string) { + sqlDB.Exec(t, fmt.Sprintf(`CREATE EXTERNAL CONNECTION '%s' AS '%s'`, externalConnectionName, uri)) + } + backupAndRestoreFromExternalConnection := func(backupExternalConnectionName string) { + backupURI := fmt.Sprintf("external://%s", backupExternalConnectionName) + sqlDB.Exec(t, fmt.Sprintf(`BACKUP DATABASE foo INTO '%s'`, backupURI)) + sqlDB.Exec(t, fmt.Sprintf(`RESTORE DATABASE foo FROM LATEST IN '%s' WITH new_db_name = bar`, backupURI)) + sqlDB.CheckQueryResults(t, `SELECT * FROM bar.foo`, [][]string{{"1"}, {"2"}, {"3"}}) + sqlDB.CheckQueryResults(t, `SELECT * FROM crdb_internal.invalid_objects`, [][]string{}) + sqlDB.Exec(t, `DROP DATABASE bar CASCADE`) + } + + limitedBucket := os.Getenv("GOOGLE_LIMITED_BUCKET") + if limitedBucket == "" { + skip.IgnoreLint(t, "GOOGLE_LIMITED_BUCKET env var must be set") + } + assumedAccount := os.Getenv("ASSUME_SERVICE_ACCOUNT") + if assumedAccount == "" { + skip.IgnoreLint(t, "ASSUME_SERVICE_ACCOUNT env var must be set") + } + + t.Run("ec-assume-role-specified", func(t *testing.T) { + ecName := "ec-assume-role-specified" + disallowedECName := "ec-assume-role-specified-disallowed" + credentials := os.Getenv("GOOGLE_CREDENTIALS_JSON") + if credentials == "" { + skip.IgnoreLint(t, "GOOGLE_CREDENTIALS_JSON env var must be set") + } + encoded := base64.StdEncoding.EncodeToString([]byte(credentials)) + disallowedURI := fmt.Sprintf("gs://%s/%s?%s=%s", limitedBucket, disallowedECName, + gcp.CredentialsParam, url.QueryEscape(encoded)) + disallowedCreateExternalConnection(disallowedECName, disallowedURI) + + uri := fmt.Sprintf("gs://%s/%s?%s=%s&%s=%s&%s=%s", + limitedBucket, + ecName, + cloud.AuthParam, + cloud.AuthParamSpecified, + gcp.AssumeRoleParam, + assumedAccount, gcp.CredentialsParam, + url.QueryEscape(encoded), + ) + createExternalConnection(ecName, uri) + backupAndRestoreFromExternalConnection(ecName) + }) + + t.Run("ec-assume-role-implicit", func(t *testing.T) { + if _, err := google.FindDefaultCredentials(context.Background()); err != nil { + skip.IgnoreLint(t, err) + } + ecName := "ec-assume-role-implicit" + disallowedECName := "ec-assume-role-implicit-disallowed" + disallowedURI := fmt.Sprintf("gs://%s/%s?%s=%s", limitedBucket, disallowedECName, + cloud.AuthParam, cloud.AuthParamImplicit) + disallowedCreateExternalConnection(disallowedECName, disallowedURI) + + uri := fmt.Sprintf("gs://%s/%s?%s=%s&%s=%s", + limitedBucket, + ecName, + cloud.AuthParam, + cloud.AuthParamImplicit, + gcp.AssumeRoleParam, + assumedAccount, + ) + createExternalConnection(ecName, uri) + backupAndRestoreFromExternalConnection(ecName) + }) + + t.Run("ec-assume-role-chaining", func(t *testing.T) { + credentials := os.Getenv("GOOGLE_CREDENTIALS_JSON") + if credentials == "" { + skip.IgnoreLint(t, "GOOGLE_CREDENTIALS_JSON env var must be set") + } + encoded := base64.StdEncoding.EncodeToString([]byte(credentials)) + + roleChainStr := os.Getenv("ASSUME_SERVICE_ACCOUNT_CHAIN") + if roleChainStr == "" { + skip.IgnoreLint(t, "ASSUME_SERVICE_ACCOUNT_CHAIN env var must be set") + } + + roleChain := strings.Split(roleChainStr, ",") + + for _, tc := range []struct { + auth string + credentials string + }{ + {cloud.AuthParamSpecified, encoded}, + {cloud.AuthParamImplicit, ""}, + } { + t.Run(tc.auth, func(t *testing.T) { + q := make(url.Values) + q.Set(cloud.AuthParam, tc.auth) + q.Set(gcp.CredentialsParam, tc.credentials) + + // First verify that none of the individual roles in the chain can be used + // to access the storage. + for i, role := range roleChain { + i := i + q.Set(gcp.AssumeRoleParam, role) + disallowedECName := fmt.Sprintf("ec-assume-role-checking-%d", i) + disallowedBackupURI := fmt.Sprintf("gs://%s/%s?%s", limitedBucket, + disallowedECName, q.Encode()) + disallowedCreateExternalConnection(disallowedECName, disallowedBackupURI) + } + + // Finally, check that the chain of roles can be used to access the storage. + q.Set(gcp.AssumeRoleParam, roleChainStr) + ecName := fmt.Sprintf("ec-assume-role-checking-%s", tc.auth) + uri := fmt.Sprintf("gs://%s/%s?%s", + limitedBucket, + ecName, + q.Encode(), + ) + createExternalConnection(ecName, uri) + backupAndRestoreFromExternalConnection(ecName) + }) + } + }) +} + +func TestGCPExternalConnection(t *testing.T) { + defer leaktest.AfterTest(t)() + defer log.Scope(t).Close(t) + + dir, dirCleanupFn := testutils.TempDir(t) + defer dirCleanupFn() + + params := base.TestClusterArgs{} + params.ServerArgs.ExternalIODir = dir + + tc := testcluster.StartTestCluster(t, 1, params) + defer tc.Stopper().Stop(context.Background()) + + tc.WaitForNodeLiveness(t) + sqlDB := sqlutils.MakeSQLRunner(tc.Conns[0]) + + // Setup some dummy data. + sqlDB.Exec(t, `CREATE DATABASE foo`) + sqlDB.Exec(t, `USE foo`) + sqlDB.Exec(t, `CREATE TABLE foo (id INT PRIMARY KEY)`) + sqlDB.Exec(t, `INSERT INTO foo VALUES (1), (2), (3)`) + + createExternalConnection := func(externalConnectionName, uri string) { + sqlDB.Exec(t, fmt.Sprintf(`CREATE EXTERNAL CONNECTION '%s' AS '%s'`, externalConnectionName, uri)) + } + backupAndRestoreFromExternalConnection := func(backupExternalConnectionName string) { + backupURI := fmt.Sprintf("external://%s", backupExternalConnectionName) + sqlDB.Exec(t, fmt.Sprintf(`BACKUP DATABASE foo INTO '%s'`, backupURI)) + sqlDB.Exec(t, fmt.Sprintf(`RESTORE DATABASE foo FROM LATEST IN '%s' WITH new_db_name = bar`, backupURI)) + sqlDB.CheckQueryResults(t, `SELECT * FROM bar.foo`, [][]string{{"1"}, {"2"}, {"3"}}) + sqlDB.CheckQueryResults(t, `SELECT * FROM crdb_internal.invalid_objects`, [][]string{}) + sqlDB.Exec(t, `DROP DATABASE bar CASCADE`) + } + + bucket := os.Getenv("GOOGLE_BUCKET") + if bucket == "" { + skip.IgnoreLint(t, "GOOGLE_BUCKET env var must be set") + } + + t.Run("ec-auth-implicit", func(t *testing.T) { + if !cloudtestutils.IsImplicitAuthConfigured() { + skip.IgnoreLint(t, "implicit auth is not configured") + } + + ecName := "ec-auth-implicit" + backupURI := fmt.Sprintf("gs://%s/%s?%s=%s", bucket, ecName, cloud.AuthParam, + cloud.AuthParamImplicit) + createExternalConnection(ecName, backupURI) + backupAndRestoreFromExternalConnection(ecName) + }) + + t.Run("ec-auth-specified", func(t *testing.T) { + credentials := os.Getenv("GOOGLE_CREDENTIALS_JSON") + if credentials == "" { + skip.IgnoreLint(t, "GOOGLE_CREDENTIALS_JSON env var must be set") + } + encoded := base64.StdEncoding.EncodeToString([]byte(credentials)) + ecName := "ec-auth-specified" + backupURI := fmt.Sprintf("gs://%s/%s?%s=%s", + bucket, + ecName, + gcp.CredentialsParam, + url.QueryEscape(encoded), + ) + createExternalConnection(ecName, backupURI) + backupAndRestoreFromExternalConnection(ecName) + }) + + t.Run("ec-auth-specified-bearer-token", func(t *testing.T) { + credentials := os.Getenv("GOOGLE_CREDENTIALS_JSON") + if credentials == "" { + skip.IgnoreLint(t, "GOOGLE_CREDENTIALS_JSON env var must be set") + } + + ctx := context.Background() + source, err := google.JWTConfigFromJSON([]byte(credentials), gcs.ScopeReadWrite) + require.NoError(t, err, "creating GCS oauth token source from specified credentials") + ts := source.TokenSource(ctx) + + token, err := ts.Token() + require.NoError(t, err, "getting token") + ecName := "ec-auth-specified-bearer-token" + backupURI := fmt.Sprintf("gs://%s/%s?%s=%s", + bucket, + ecName, + gcp.BearerTokenParam, + token.AccessToken, + ) + createExternalConnection(ecName, backupURI) + backupAndRestoreFromExternalConnection(ecName) + }) +} diff --git a/pkg/cloud/externalconn/connectionpb/connection.go b/pkg/cloud/externalconn/connectionpb/connection.go index 2833a2663ca0..b680ec56cf31 100644 --- a/pkg/cloud/externalconn/connectionpb/connection.go +++ b/pkg/cloud/externalconn/connectionpb/connection.go @@ -15,7 +15,7 @@ import "github.com/cockroachdb/errors" // Type returns the ConnectionType of the receiver. func (d *ConnectionDetails) Type() ConnectionType { switch d.Provider { - case ConnectionProvider_nodelocal, ConnectionProvider_s3, ConnectionProvider_userfile: + case ConnectionProvider_nodelocal, ConnectionProvider_s3, ConnectionProvider_userfile, ConnectionProvider_gs: return TypeStorage case ConnectionProvider_gcp_kms: return TypeKMS diff --git a/pkg/cloud/externalconn/connectionpb/connection.proto b/pkg/cloud/externalconn/connectionpb/connection.proto index fae445365bb5..23c7072738f7 100644 --- a/pkg/cloud/externalconn/connectionpb/connection.proto +++ b/pkg/cloud/externalconn/connectionpb/connection.proto @@ -21,6 +21,7 @@ enum ConnectionProvider { nodelocal = 1; s3 = 4; userfile = 5; + gs = 6; // KMS providers. gcp_kms = 2; diff --git a/pkg/cloud/gcp/BUILD.bazel b/pkg/cloud/gcp/BUILD.bazel index 870ae1b2ab71..a56c2bc8222f 100644 --- a/pkg/cloud/gcp/BUILD.bazel +++ b/pkg/cloud/gcp/BUILD.bazel @@ -6,6 +6,7 @@ go_library( srcs = [ "gcp_kms.go", "gcp_kms_connection.go", + "gcs_connection.go", "gcs_retry.go", "gcs_storage.go", ], diff --git a/pkg/cloud/gcp/gcs_connection.go b/pkg/cloud/gcp/gcs_connection.go new file mode 100644 index 000000000000..b553f127e7d1 --- /dev/null +++ b/pkg/cloud/gcp/gcs_connection.go @@ -0,0 +1,47 @@ +// Copyright 2022 The Cockroach Authors. +// +// Use of this software is governed by the Business Source License +// included in the file licenses/BSL.txt. +// +// As of the Change Date specified in that file, in accordance with +// the Business Source License, use of this software will be governed +// by the Apache License, Version 2.0, included in the file +// licenses/APL.txt. + +package gcp + +import ( + "context" + "net/url" + + "github.com/cockroachdb/cockroach/pkg/cloud/externalconn" + "github.com/cockroachdb/cockroach/pkg/cloud/externalconn/connectionpb" + "github.com/cockroachdb/cockroach/pkg/cloud/externalconn/utils" + "github.com/cockroachdb/cockroach/pkg/security/username" + "github.com/cockroachdb/errors" +) + +func parseAndValidateGCSConnectionURI( + ctx context.Context, execCfg interface{}, user username.SQLUsername, uri *url.URL, +) (externalconn.ExternalConnection, error) { + if err := utils.CheckExternalStorageConnection(ctx, execCfg, user, uri.String()); err != nil { + return nil, errors.Wrap(err, "failed to create gs external connection") + } + + connDetails := connectionpb.ConnectionDetails{ + Provider: connectionpb.ConnectionProvider_gs, + Details: &connectionpb.ConnectionDetails_SimpleURI{ + SimpleURI: &connectionpb.SimpleURI{ + URI: uri.String(), + }, + }, + } + return externalconn.NewExternalConnection(connDetails), nil +} + +func init() { + externalconn.RegisterConnectionDetailsFromURIFactory( + gcsScheme, + parseAndValidateGCSConnectionURI, + ) +} diff --git a/pkg/cloud/gcp/gcs_storage.go b/pkg/cloud/gcp/gcs_storage.go index 5afcec479414..b9d9c263a9fa 100644 --- a/pkg/cloud/gcp/gcs_storage.go +++ b/pkg/cloud/gcp/gcs_storage.go @@ -67,20 +67,28 @@ var gcsChunkingEnabled = settings.RegisterBoolSetting( ) func parseGSURL(_ cloud.ExternalStorageURIContext, uri *url.URL) (cloudpb.ExternalStorage, error) { + gsURL := cloud.ConsumeURL{URL: uri} conf := cloudpb.ExternalStorage{} conf.Provider = cloudpb.ExternalStorageProvider_gs - assumeRole, delegateRoles := cloud.ParseRoleString(uri.Query().Get(AssumeRoleParam)) + assumeRole, delegateRoles := cloud.ParseRoleString(gsURL.ConsumeParam(AssumeRoleParam)) conf.GoogleCloudConfig = &cloudpb.ExternalStorage_GCS{ Bucket: uri.Host, Prefix: uri.Path, - Auth: uri.Query().Get(cloud.AuthParam), - BillingProject: uri.Query().Get(GoogleBillingProjectParam), - Credentials: uri.Query().Get(CredentialsParam), + Auth: gsURL.ConsumeParam(cloud.AuthParam), + BillingProject: gsURL.ConsumeParam(GoogleBillingProjectParam), + Credentials: gsURL.ConsumeParam(CredentialsParam), AssumeRole: assumeRole, AssumeRoleDelegates: delegateRoles, - BearerToken: uri.Query().Get(BearerTokenParam), + BearerToken: gsURL.ConsumeParam(BearerTokenParam), } conf.GoogleCloudConfig.Prefix = strings.TrimLeft(conf.GoogleCloudConfig.Prefix, "/") + + // Validate that all the passed in parameters are supported. + if unknownParams := gsURL.RemainingQueryParams(); len(unknownParams) > 0 { + return cloudpb.ExternalStorage{}, errors.Errorf( + `unknown GS query parameters: %s`, strings.Join(unknownParams, ", ")) + } + return conf, nil } From 2ddb990bed5533fa55c04ab90545f227d511c840 Mon Sep 17 00:00:00 2001 From: Yahor Yuzefovich Date: Mon, 15 Aug 2022 14:27:21 -0700 Subject: [PATCH 2/8] roachtest: reduce slowness threshold in tpchvec/streamer Release note: None --- pkg/cmd/roachtest/tests/tpchvec.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/pkg/cmd/roachtest/tests/tpchvec.go b/pkg/cmd/roachtest/tests/tpchvec.go index 2cae19a82caf..42d1ba089e11 100644 --- a/pkg/cmd/roachtest/tests/tpchvec.go +++ b/pkg/cmd/roachtest/tests/tpchvec.go @@ -587,8 +587,7 @@ func registerTPCHVec(r registry.Registry) { Run: func(ctx context.Context, t test.Test, c cluster.Cluster) { runTPCHVec(ctx, t, c, newTpchVecPerfTest( "sql.distsql.use_streamer.enabled", /* settingName */ - // TODO(yuzefovich): reduce the threshold over time. - 3.0, /* slownessThreshold */ + 1.25, /* slownessThreshold */ ), baseTestRun) }, }) From 4710f62e70c1b1fe58c2c584ca6c1e593c19a975 Mon Sep 17 00:00:00 2001 From: Rafi Shamim Date: Mon, 15 Aug 2022 13:10:09 -0400 Subject: [PATCH 3/8] clusterversions: remove RowLevelTTL version Release note: None --- pkg/clusterversion/cockroach_versions.go | 6 -- pkg/clusterversion/key_string.go | 75 +++++++++---------- pkg/sql/alter_table.go | 7 -- pkg/sql/create_table.go | 13 ---- .../logic_test/row_level_ttl_mixed_21.2_22.1 | 10 --- .../tests/local-mixed-21.2-22.1/BUILD.bazel | 2 +- .../local-mixed-21.2-22.1/generated_test.go | 7 -- 7 files changed, 38 insertions(+), 82 deletions(-) delete mode 100644 pkg/sql/logictest/testdata/logic_test/row_level_ttl_mixed_21.2_22.1 diff --git a/pkg/clusterversion/cockroach_versions.go b/pkg/clusterversion/cockroach_versions.go index f2c553097ba9..bdf601eb613f 100644 --- a/pkg/clusterversion/cockroach_versions.go +++ b/pkg/clusterversion/cockroach_versions.go @@ -196,8 +196,6 @@ const ( // ChangefeedIdleness is the version where changefeed aggregators forward // idleness-related information alnog with resolved spans to the frontier ChangefeedIdleness - // RowLevelTTL is the version where we allow row level TTL tables. - RowLevelTTL // EnableNewStoreRebalancer enables the new store rebalancer introduced in // 22.1. EnableNewStoreRebalancer @@ -396,10 +394,6 @@ var versionsSingleton = keyedVersions{ Key: ChangefeedIdleness, Version: roachpb.Version{Major: 21, Minor: 2, Internal: 82}, }, - { - Key: RowLevelTTL, - Version: roachpb.Version{Major: 21, Minor: 2, Internal: 88}, - }, { Key: EnableNewStoreRebalancer, Version: roachpb.Version{Major: 21, Minor: 2, Internal: 96}, diff --git a/pkg/clusterversion/key_string.go b/pkg/clusterversion/key_string.go index e3ad7ef650dc..bd20edcef8ea 100644 --- a/pkg/clusterversion/key_string.go +++ b/pkg/clusterversion/key_string.go @@ -18,47 +18,46 @@ func _() { _ = x[EnablePebbleFormatVersionBlockProperties-7] _ = x[EnableLeaseHolderRemoval-8] _ = x[ChangefeedIdleness-9] - _ = x[RowLevelTTL-10] - _ = x[EnableNewStoreRebalancer-11] - _ = x[ClusterLocksVirtualTable-12] - _ = x[AutoStatsTableSettings-13] - _ = x[SuperRegions-14] - _ = x[EnableNewChangefeedOptions-15] - _ = x[V22_1-16] - _ = x[Start22_2-17] - _ = x[LocalTimestamps-18] - _ = x[PebbleFormatSplitUserKeysMarkedCompacted-19] - _ = x[EnsurePebbleFormatVersionRangeKeys-20] - _ = x[EnablePebbleFormatVersionRangeKeys-21] - _ = x[TrigramInvertedIndexes-22] - _ = x[RemoveGrantPrivilege-23] - _ = x[MVCCRangeTombstones-24] - _ = x[UpgradeSequenceToBeReferencedByID-25] - _ = x[SampledStmtDiagReqs-26] - _ = x[AddSSTableTombstones-27] - _ = x[SystemPrivilegesTable-28] - _ = x[EnablePredicateProjectionChangefeed-29] - _ = x[AlterSystemSQLInstancesAddLocality-30] - _ = x[SystemExternalConnectionsTable-31] - _ = x[AlterSystemStatementStatisticsAddIndexRecommendations-32] - _ = x[RoleIDSequence-33] - _ = x[AddSystemUserIDColumn-34] - _ = x[SystemUsersIDColumnIsBackfilled-35] - _ = x[SetSystemUsersUserIDColumnNotNull-36] - _ = x[SQLSchemaTelemetryScheduledJobs-37] - _ = x[SchemaChangeSupportsCreateFunction-38] - _ = x[DeleteRequestReturnKey-39] - _ = x[PebbleFormatPrePebblev1Marked-40] - _ = x[RoleOptionsTableHasIDColumn-41] - _ = x[RoleOptionsIDColumnIsBackfilled-42] - _ = x[SetRoleOptionsUserIDColumnNotNull-43] - _ = x[UseDelRangeInGCJob-44] - _ = x[WaitedForDelRangeInGCJob-45] + _ = x[EnableNewStoreRebalancer-10] + _ = x[ClusterLocksVirtualTable-11] + _ = x[AutoStatsTableSettings-12] + _ = x[SuperRegions-13] + _ = x[EnableNewChangefeedOptions-14] + _ = x[V22_1-15] + _ = x[Start22_2-16] + _ = x[LocalTimestamps-17] + _ = x[PebbleFormatSplitUserKeysMarkedCompacted-18] + _ = x[EnsurePebbleFormatVersionRangeKeys-19] + _ = x[EnablePebbleFormatVersionRangeKeys-20] + _ = x[TrigramInvertedIndexes-21] + _ = x[RemoveGrantPrivilege-22] + _ = x[MVCCRangeTombstones-23] + _ = x[UpgradeSequenceToBeReferencedByID-24] + _ = x[SampledStmtDiagReqs-25] + _ = x[AddSSTableTombstones-26] + _ = x[SystemPrivilegesTable-27] + _ = x[EnablePredicateProjectionChangefeed-28] + _ = x[AlterSystemSQLInstancesAddLocality-29] + _ = x[SystemExternalConnectionsTable-30] + _ = x[AlterSystemStatementStatisticsAddIndexRecommendations-31] + _ = x[RoleIDSequence-32] + _ = x[AddSystemUserIDColumn-33] + _ = x[SystemUsersIDColumnIsBackfilled-34] + _ = x[SetSystemUsersUserIDColumnNotNull-35] + _ = x[SQLSchemaTelemetryScheduledJobs-36] + _ = x[SchemaChangeSupportsCreateFunction-37] + _ = x[DeleteRequestReturnKey-38] + _ = x[PebbleFormatPrePebblev1Marked-39] + _ = x[RoleOptionsTableHasIDColumn-40] + _ = x[RoleOptionsIDColumnIsBackfilled-41] + _ = x[SetRoleOptionsUserIDColumnNotNull-42] + _ = x[UseDelRangeInGCJob-43] + _ = x[WaitedForDelRangeInGCJob-44] } -const _Key_name = "V21_2Start22_1ProbeRequestPublicSchemasWithDescriptorsEnsureSpanConfigReconciliationEnsureSpanConfigSubscriptionEnableSpanConfigStoreEnablePebbleFormatVersionBlockPropertiesEnableLeaseHolderRemovalChangefeedIdlenessRowLevelTTLEnableNewStoreRebalancerClusterLocksVirtualTableAutoStatsTableSettingsSuperRegionsEnableNewChangefeedOptionsV22_1Start22_2LocalTimestampsPebbleFormatSplitUserKeysMarkedCompactedEnsurePebbleFormatVersionRangeKeysEnablePebbleFormatVersionRangeKeysTrigramInvertedIndexesRemoveGrantPrivilegeMVCCRangeTombstonesUpgradeSequenceToBeReferencedByIDSampledStmtDiagReqsAddSSTableTombstonesSystemPrivilegesTableEnablePredicateProjectionChangefeedAlterSystemSQLInstancesAddLocalitySystemExternalConnectionsTableAlterSystemStatementStatisticsAddIndexRecommendationsRoleIDSequenceAddSystemUserIDColumnSystemUsersIDColumnIsBackfilledSetSystemUsersUserIDColumnNotNullSQLSchemaTelemetryScheduledJobsSchemaChangeSupportsCreateFunctionDeleteRequestReturnKeyPebbleFormatPrePebblev1MarkedRoleOptionsTableHasIDColumnRoleOptionsIDColumnIsBackfilledSetRoleOptionsUserIDColumnNotNullUseDelRangeInGCJobWaitedForDelRangeInGCJob" +const _Key_name = "V21_2Start22_1ProbeRequestPublicSchemasWithDescriptorsEnsureSpanConfigReconciliationEnsureSpanConfigSubscriptionEnableSpanConfigStoreEnablePebbleFormatVersionBlockPropertiesEnableLeaseHolderRemovalChangefeedIdlenessEnableNewStoreRebalancerClusterLocksVirtualTableAutoStatsTableSettingsSuperRegionsEnableNewChangefeedOptionsV22_1Start22_2LocalTimestampsPebbleFormatSplitUserKeysMarkedCompactedEnsurePebbleFormatVersionRangeKeysEnablePebbleFormatVersionRangeKeysTrigramInvertedIndexesRemoveGrantPrivilegeMVCCRangeTombstonesUpgradeSequenceToBeReferencedByIDSampledStmtDiagReqsAddSSTableTombstonesSystemPrivilegesTableEnablePredicateProjectionChangefeedAlterSystemSQLInstancesAddLocalitySystemExternalConnectionsTableAlterSystemStatementStatisticsAddIndexRecommendationsRoleIDSequenceAddSystemUserIDColumnSystemUsersIDColumnIsBackfilledSetSystemUsersUserIDColumnNotNullSQLSchemaTelemetryScheduledJobsSchemaChangeSupportsCreateFunctionDeleteRequestReturnKeyPebbleFormatPrePebblev1MarkedRoleOptionsTableHasIDColumnRoleOptionsIDColumnIsBackfilledSetRoleOptionsUserIDColumnNotNullUseDelRangeInGCJobWaitedForDelRangeInGCJob" -var _Key_index = [...]uint16{0, 5, 14, 26, 54, 84, 112, 133, 173, 197, 215, 226, 250, 274, 296, 308, 334, 339, 348, 363, 403, 437, 471, 493, 513, 532, 565, 584, 604, 625, 660, 694, 724, 777, 791, 812, 843, 876, 907, 941, 963, 992, 1019, 1050, 1083, 1101, 1125} +var _Key_index = [...]uint16{0, 5, 14, 26, 54, 84, 112, 133, 173, 197, 215, 239, 263, 285, 297, 323, 328, 337, 352, 392, 426, 460, 482, 502, 521, 554, 573, 593, 614, 649, 683, 713, 766, 780, 801, 832, 865, 896, 930, 952, 981, 1008, 1039, 1072, 1090, 1114} func (i Key) String() string { if i < 0 || i >= Key(len(_Key_index)-1) { diff --git a/pkg/sql/alter_table.go b/pkg/sql/alter_table.go index 812c5184b76a..75d3f14051a2 100644 --- a/pkg/sql/alter_table.go +++ b/pkg/sql/alter_table.go @@ -1859,13 +1859,6 @@ func handleTTLStorageParamChange( tableDesc *tabledesc.Mutable, before, after *catpb.RowLevelTTL, ) error { - - if before == nil && after != nil { - if err := checkTTLEnabledForCluster(params.ctx, params.p.ExecCfg().Settings); err != nil { - return err - } - } - // update existing config if before != nil && after != nil { diff --git a/pkg/sql/create_table.go b/pkg/sql/create_table.go index 950e6b7a2854..66b94bc3b499 100644 --- a/pkg/sql/create_table.go +++ b/pkg/sql/create_table.go @@ -1475,9 +1475,6 @@ func NewTableDesc( // Create the TTL automatic column (crdb_internal_expiration) if one does not already exist. if ttl := desc.GetRowLevelTTL(); ttl != nil && ttl.HasDurationExpr() { - if err := checkTTLEnabledForCluster(ctx, st); err != nil { - return nil, err - } hasRowLevelTTLColumn := false for _, def := range n.Defs { switch def := def.(type) { @@ -2422,16 +2419,6 @@ func newRowLevelTTLScheduledJob( return sj, nil } -func checkTTLEnabledForCluster(ctx context.Context, st *cluster.Settings) error { - if !st.Version.IsActive(ctx, clusterversion.RowLevelTTL) { - return pgerror.Newf( - pgcode.FeatureNotSupported, - "row level TTL is only available once the cluster is fully upgraded", - ) - } - return nil -} - func checkAutoStatsTableSettingsEnabledForCluster(ctx context.Context, st *cluster.Settings) error { if !st.Version.IsActive(ctx, clusterversion.AutoStatsTableSettings) { return pgerror.Newf( diff --git a/pkg/sql/logictest/testdata/logic_test/row_level_ttl_mixed_21.2_22.1 b/pkg/sql/logictest/testdata/logic_test/row_level_ttl_mixed_21.2_22.1 deleted file mode 100644 index e1538100aade..000000000000 --- a/pkg/sql/logictest/testdata/logic_test/row_level_ttl_mixed_21.2_22.1 +++ /dev/null @@ -1,10 +0,0 @@ -# LogicTest: local-mixed-21.2-22.1 - -statement error row level TTL is only available once the cluster is fully upgraded -CREATE TABLE tbl () WITH (ttl_expire_after = '10 minutes') - -statement ok -CREATE TABLE tbl () - -statement error row level TTL is only available once the cluster is fully upgraded -ALTER TABLE tbl SET (ttl_expire_after = '10 minutes') diff --git a/pkg/sql/logictest/tests/local-mixed-21.2-22.1/BUILD.bazel b/pkg/sql/logictest/tests/local-mixed-21.2-22.1/BUILD.bazel index 540ed1d18ae2..9b36c0c237b3 100644 --- a/pkg/sql/logictest/tests/local-mixed-21.2-22.1/BUILD.bazel +++ b/pkg/sql/logictest/tests/local-mixed-21.2-22.1/BUILD.bazel @@ -9,7 +9,7 @@ go_test( "//c-deps:libgeos", # keep "//pkg/sql/logictest:testdata", # keep ], - shard_count = 2, + shard_count = 1, deps = [ "//pkg/build/bazel", "//pkg/security/securityassets", diff --git a/pkg/sql/logictest/tests/local-mixed-21.2-22.1/generated_test.go b/pkg/sql/logictest/tests/local-mixed-21.2-22.1/generated_test.go index d57320d77329..134b9518798e 100644 --- a/pkg/sql/logictest/tests/local-mixed-21.2-22.1/generated_test.go +++ b/pkg/sql/logictest/tests/local-mixed-21.2-22.1/generated_test.go @@ -72,13 +72,6 @@ func TestLogic_tmp(t *testing.T) { logictest.RunLogicTests(t, logictest.TestServerArgs{}, configIdx, glob) } -func TestLogic_row_level_ttl_mixed_21_2_22_1( - t *testing.T, -) { - defer leaktest.AfterTest(t)() - runLogicTest(t, "row_level_ttl_mixed_21.2_22.1") -} - func TestLogic_super_regions_mixed_version( t *testing.T, ) { From 7f6691e8b49766a2241cf3c367392301b14afb05 Mon Sep 17 00:00:00 2001 From: Jackson Owens Date: Tue, 16 Aug 2022 08:12:54 -0400 Subject: [PATCH 4/8] storage/metamorphic: skip TestPebbleEquivalence Skip TestPebbleEquivalence until #86102 is resolved. Release note: None --- pkg/storage/metamorphic/meta_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/pkg/storage/metamorphic/meta_test.go b/pkg/storage/metamorphic/meta_test.go index b9f906fd91a8..f40c001756a4 100644 --- a/pkg/storage/metamorphic/meta_test.go +++ b/pkg/storage/metamorphic/meta_test.go @@ -169,6 +169,7 @@ func TestPebbleEquivalence(t *testing.T) { defer leaktest.AfterTest(t)() defer log.Scope(t).Close(t) + skip.WithIssue(t, 86102) skip.UnderRace(t) runPebbleEquivalenceTest(t) } From d364c31f0bca700125e53bfed51f094d2311e000 Mon Sep 17 00:00:00 2001 From: Andrew Werner Date: Tue, 16 Aug 2022 09:45:02 -0400 Subject: [PATCH 5/8] sql/logictest: attempt to deflake retry errors When we run transactions in logictests, they can be exposed to retry errors. The framework does not have tools to retry whole blocks. In #58217 we added a mechanism to skip tests which hit such errors. Apply that logic here. (hopefully) Fixes #86215 Release note: None --- pkg/sql/logictest/testdata/logic_test/enums | 2 ++ 1 file changed, 2 insertions(+) diff --git a/pkg/sql/logictest/testdata/logic_test/enums b/pkg/sql/logictest/testdata/logic_test/enums index 17dac4cda789..589cca6d527d 100644 --- a/pkg/sql/logictest/testdata/logic_test/enums +++ b/pkg/sql/logictest/testdata/logic_test/enums @@ -1204,6 +1204,8 @@ CREATE TYPE fakedb.typ AS ENUM ('schema') # Test the behavior of dropping a not-null enum colums +skip_on_retry + subtest drop_not_null statement ok From d71f8340d162f2c47a6d83db5457658402fcacec Mon Sep 17 00:00:00 2001 From: Chengxiong Ruan Date: Wed, 10 Aug 2022 11:32:08 -0400 Subject: [PATCH 6/8] sql: implement CREATE OR REPLACE FUNCTION This commit adds to the `REPLACE` path to the `CREATE OR REPLACE FUNCTION` statement. Major changes are (1) fetch function with same signature if exists, and validate (2) remove refereces before replacing the function, and then add new references. Release note: None --- pkg/sql/catalog/funcdesc/func_desc.go | 10 +- pkg/sql/crdb_internal.go | 94 ++- pkg/sql/create_function.go | 341 ++++++--- pkg/sql/create_function_test.go | 152 ++++ pkg/sql/function_resolver_test.go | 8 +- pkg/sql/logictest/testdata/logic_test/udf | 865 ++++++++++------------ 6 files changed, 848 insertions(+), 622 deletions(-) diff --git a/pkg/sql/catalog/funcdesc/func_desc.go b/pkg/sql/catalog/funcdesc/func_desc.go index cfee72a322fa..b31f7ae4ed23 100644 --- a/pkg/sql/catalog/funcdesc/func_desc.go +++ b/pkg/sql/catalog/funcdesc/func_desc.go @@ -59,7 +59,7 @@ func NewMutableFunctionDescriptor( parentID descpb.ID, parentSchemaID descpb.ID, name string, - argNum int, + args []descpb.FunctionDescriptor_Argument, returnType *types.T, returnSet bool, privs *catpb.PrivilegeDescriptor, @@ -71,7 +71,7 @@ func NewMutableFunctionDescriptor( ID: id, ParentID: parentID, ParentSchemaID: parentSchemaID, - Args: make([]descpb.FunctionDescriptor_Argument, 0, argNum), + Args: args, ReturnType: descpb.FunctionDescriptor_ReturnType{ Type: returnType, ReturnSet: returnSet, @@ -417,9 +417,9 @@ func (desc *Mutable) SetDeclarativeSchemaChangerState(state *scpb.DescriptorStat desc.DeclarativeSchemaChangerState = state } -// AddArgument adds a function argument to argument list. -func (desc *Mutable) AddArgument(arg descpb.FunctionDescriptor_Argument) { - desc.Args = append(desc.Args, arg) +// AddArguments adds function arguments to argument list. +func (desc *Mutable) AddArguments(args ...descpb.FunctionDescriptor_Argument) { + desc.Args = append(desc.Args, args...) } // SetVolatility sets the volatility attribute. diff --git a/pkg/sql/crdb_internal.go b/pkg/sql/crdb_internal.go index 9537718c93f3..86b7b868b6f9 100644 --- a/pkg/sql/crdb_internal.go +++ b/pkg/sql/crdb_internal.go @@ -2591,7 +2591,6 @@ CREATE TABLE crdb_internal.create_function_statements ( ) `, populate: func(ctx context.Context, p *planner, db catalog.DatabaseDescriptor, addRow func(...tree.Datum) error) error { - flags := tree.ObjectLookupFlags{CommonLookupFlags: tree.CommonLookupFlags{AvoidLeased: true}} var dbDescs []catalog.DatabaseDescriptor if db == nil { var err error @@ -2602,45 +2601,68 @@ CREATE TABLE crdb_internal.create_function_statements ( } else { dbDescs = append(dbDescs, db) } - for _, db := range dbDescs { - err := forEachSchema(ctx, p, db, func(sc catalog.SchemaDescriptor) error { + var fnIDs []descpb.ID + fnIDToScName := make(map[descpb.ID]string) + fnIDToScID := make(map[descpb.ID]descpb.ID) + fnIDToDBName := make(map[descpb.ID]string) + fnIDToDBID := make(map[descpb.ID]descpb.ID) + for _, curDB := range dbDescs { + err := forEachSchema(ctx, p, curDB, func(sc catalog.SchemaDescriptor) error { return sc.ForEachFunctionOverload(func(overload descpb.SchemaDescriptor_FunctionOverload) error { - fnDesc, err := p.Descriptors().GetImmutableFunctionByID(ctx, p.txn, overload.ID, flags) - if err != nil { - return err - } - treeNode, err := fnDesc.ToCreateExpr() - treeNode.FuncName.ObjectNamePrefix = tree.ObjectNamePrefix{ - ExplicitSchema: true, - SchemaName: tree.Name(sc.GetName()), - } - if err != nil { - return err - } - for i := range treeNode.Options { - if body, ok := treeNode.Options[i].(tree.FunctionBodyStr); ok { - stmtStrs := strings.Split(string(body), "\n") - for i := range stmtStrs { - stmtStrs[i] = "\t" + stmtStrs[i] - } + fnIDs = append(fnIDs, overload.ID) + fnIDToScName[overload.ID] = sc.GetName() + fnIDToScID[overload.ID] = sc.GetID() + fnIDToDBName[overload.ID] = curDB.GetName() + fnIDToDBID[overload.ID] = curDB.GetID() + return nil + }) + }) + if err != nil { + return err + } + } - p := &treeNode.Options[i] - // Add two new lines just for better formatting. - *p = "\n" + tree.FunctionBodyStr(strings.Join(stmtStrs, "\n")) + "\n" - } + fnDescs, err := p.Descriptors().GetImmutableDescriptorsByID( + ctx, p.txn, tree.CommonLookupFlags{Required: true, AvoidLeased: true}, fnIDs..., + ) + if err != nil { + return err + } + + for _, desc := range fnDescs { + fnDesc := desc.(catalog.FunctionDescriptor) + if err != nil { + return err + } + treeNode, err := fnDesc.ToCreateExpr() + treeNode.FuncName.ObjectNamePrefix = tree.ObjectNamePrefix{ + ExplicitSchema: true, + SchemaName: tree.Name(fnIDToScName[fnDesc.GetID()]), + } + if err != nil { + return err + } + for i := range treeNode.Options { + if body, ok := treeNode.Options[i].(tree.FunctionBodyStr); ok { + stmtStrs := strings.Split(string(body), "\n") + for i := range stmtStrs { + stmtStrs[i] = "\t" + stmtStrs[i] } + p := &treeNode.Options[i] + // Add two new lines just for better formatting. + *p = "\n" + tree.FunctionBodyStr(strings.Join(stmtStrs, "\n")) + "\n" + } + } - return addRow( - tree.NewDInt(tree.DInt(db.GetID())), // database_id - tree.NewDString(db.GetName()), // database_name - tree.NewDInt(tree.DInt(sc.GetID())), // schema_id - tree.NewDString(sc.GetName()), // schema_name - tree.NewDInt(tree.DInt(fnDesc.GetID())), // function_id - tree.NewDString(fnDesc.GetName()), //function_name - tree.NewDString(tree.AsString(treeNode)), // create_statement - ) - }) - }) + err = addRow( + tree.NewDInt(tree.DInt(fnIDToDBID[fnDesc.GetID()])), // database_id + tree.NewDString(fnIDToDBName[fnDesc.GetID()]), // database_name + tree.NewDInt(tree.DInt(fnIDToScID[fnDesc.GetID()])), // schema_id + tree.NewDString(fnIDToScName[fnDesc.GetID()]), // schema_name + tree.NewDInt(tree.DInt(fnDesc.GetID())), // function_id + tree.NewDString(fnDesc.GetName()), //function_name + tree.NewDString(tree.AsString(treeNode)), // create_statement + ) if err != nil { return err } diff --git a/pkg/sql/create_function.go b/pkg/sql/create_function.go index 9c0449858bfc..28609f896c4c 100644 --- a/pkg/sql/create_function.go +++ b/pkg/sql/create_function.go @@ -13,7 +13,6 @@ package sql import ( "context" "fmt" - "sort" "github.com/cockroachdb/cockroach/pkg/clusterversion" "github.com/cockroachdb/cockroach/pkg/roachpb" @@ -68,58 +67,249 @@ func (n *createFunctionNode) startExec(params runParams) error { } mutScDesc := scDesc.(*schemadesc.Mutable) - udfMutableDesc, err := n.getMutableFuncDesc(mutScDesc, params) + udfMutableDesc, isNew, err := n.getMutableFuncDesc(mutScDesc, params) if err != nil { return err } - for _, arg := range n.cf.Args { - pbArg, err := makeFunctionArg(params.ctx, arg, params.p) + if isNew { + return n.createNewFunction(udfMutableDesc, mutScDesc, params) + } + return n.replaceFunction(udfMutableDesc, params) +} + +func (*createFunctionNode) Next(params runParams) (bool, error) { return false, nil } +func (*createFunctionNode) Values() tree.Datums { return tree.Datums{} } +func (*createFunctionNode) Close(ctx context.Context) {} + +func (n *createFunctionNode) createNewFunction( + udfDesc *funcdesc.Mutable, scDesc *schemadesc.Mutable, params runParams, +) error { + for _, option := range n.cf.Options { + err := setFuncOption(params, udfDesc, option) if err != nil { return err } - udfMutableDesc.AddArgument(pbArg) + } + if udfDesc.LeakProof && udfDesc.Volatility != catpb.Function_IMMUTABLE { + return pgerror.Newf( + pgcode.InvalidFunctionDefinition, + "cannot create leakproof function with non-immutable volatility: %s", + udfDesc.Volatility.String(), + ) + } + + if err := n.addUDFReferences(udfDesc, params); err != nil { + return err + } + + err := params.p.createDescriptorWithID( + params.ctx, + roachpb.Key{}, // UDF does not have namespace entry. + udfDesc.GetID(), + udfDesc, + tree.AsStringWithFQNames(&n.cf.FuncName, params.Ann()), + ) + if err != nil { + return err + } + + returnType, err := tree.ResolveType(params.ctx, n.cf.ReturnType.Type, params.p) + if err != nil { + return err + } + argTypes := make([]*types.T, len(udfDesc.Args)) + for i, arg := range udfDesc.Args { + argTypes[i] = arg.Type + } + scDesc.AddFunction( + udfDesc.GetName(), + descpb.SchemaDescriptor_FunctionOverload{ + ID: udfDesc.GetID(), + ArgTypes: argTypes, + ReturnType: returnType, + ReturnSet: udfDesc.ReturnType.ReturnSet, + }, + ) + if err := params.p.writeSchemaDescChange(params.ctx, scDesc, "Create Function"); err != nil { + return err + } + + return nil +} + +func (n *createFunctionNode) replaceFunction(udfDesc *funcdesc.Mutable, params runParams) error { + // TODO(chengxiong): add validation that the function is not referenced. This + // is needed when we start allowing function references from other objects. + + // Make sure argument names are not changed. + for i := range n.cf.Args { + if string(n.cf.Args[i].Name) != udfDesc.Args[i].Name { + return pgerror.Newf( + pgcode.InvalidFunctionDefinition, "cannot change name of input parameter %q", udfDesc.Args[i].Name, + ) + } } - // Set default values before applying options. This simplifies - // the replacing logic. - resetFuncOption(udfMutableDesc) + // Make sure return type is the same. + retType, err := tree.ResolveType(params.ctx, n.cf.ReturnType.Type, params.p) + if err != nil { + return err + } + if n.cf.ReturnType.IsSet != udfDesc.ReturnType.ReturnSet || !retType.Equal(udfDesc.ReturnType.Type) { + return pgerror.Newf(pgcode.InvalidFunctionDefinition, "cannot change return type of existing function") + } + + resetFuncOption(udfDesc) for _, option := range n.cf.Options { - err := setFuncOption(params, udfMutableDesc, option) + err := setFuncOption(params, udfDesc, option) if err != nil { return err } } - if udfMutableDesc.LeakProof && udfMutableDesc.Volatility != catpb.Function_IMMUTABLE { + if udfDesc.LeakProof && udfDesc.Volatility != catpb.Function_IMMUTABLE { return pgerror.Newf( pgcode.InvalidFunctionDefinition, "cannot create leakproof function with non-immutable volatility: %s", - udfMutableDesc.Volatility.String(), + udfDesc.Volatility.String(), + ) + } + + // Removing all existing references before adding new references. + for _, id := range udfDesc.DependsOn { + backRefMutable, err := params.p.Descriptors().GetMutableTableByID( + params.ctx, params.p.txn, id, tree.ObjectLookupFlagsWithRequired(), ) + if err != nil { + return err + } + backRefMutable.DependedOnBy = removeMatchingReferences(backRefMutable.DependedOnBy, udfDesc.ID) + jobDesc := fmt.Sprintf( + "removing udf reference %s(%d) in table %s(%d)", + udfDesc.Name, udfDesc.ID, backRefMutable.Name, backRefMutable.ID, + ) + if err := params.p.writeSchemaChange(params.ctx, backRefMutable, descpb.InvalidMutationID, jobDesc); err != nil { + return err + } + } + jobDesc := fmt.Sprintf("updating type back reference %d for function %d", udfDesc.DependsOnTypes, udfDesc.ID) + if err := params.p.removeTypeBackReferences(params.ctx, udfDesc.DependsOnTypes, udfDesc.ID, jobDesc); err != nil { + return err + } + // Add all new references. + if err := n.addUDFReferences(udfDesc, params); err != nil { + return err + } + + return params.p.writeFuncSchemaChange(params.ctx, udfDesc) +} + +func (n *createFunctionNode) getMutableFuncDesc( + scDesc catalog.SchemaDescriptor, params runParams, +) (fnDesc *funcdesc.Mutable, isNew bool, err error) { + // Resolve argument types. + argTypes := make([]*types.T, len(n.cf.Args)) + pbArgs := make([]descpb.FunctionDescriptor_Argument, len(n.cf.Args)) + argNameSeen := make(map[tree.Name]struct{}) + for i, arg := range n.cf.Args { + if _, ok := argNameSeen[arg.Name]; ok { + // Argument names cannot be used more than once. + return nil, false, pgerror.Newf( + pgcode.InvalidFunctionDefinition, "parameter name %q used more than once", arg.Name, + ) + } + argNameSeen[arg.Name] = struct{}{} + pbArg, err := makeFunctionArg(params.ctx, arg, params.p) + if err != nil { + return nil, false, err + } + pbArgs[i] = pbArg + argTypes[i] = pbArg.Type + } + + // Try to look up an existing function. + fuObj := tree.FuncObj{ + FuncName: n.cf.FuncName, + Args: n.cf.Args, + } + existing, err := params.p.matchUDF(params.ctx, &fuObj, false /* required */) + if err != nil { + return nil, false, err + } + + if existing != nil { + // Return an error if there is an existing match but not a replacement. + if !n.cf.Replace { + return nil, false, pgerror.Newf( + pgcode.DuplicateFunction, + "function %q already exists with same argument types", + n.cf.FuncName.Object(), + ) + } + fnID, err := funcdesc.UserDefinedFunctionOIDToID(existing.Oid) + if err != nil { + return nil, false, err + } + fnDesc, err = params.p.checkPrivilegesForDropFunction(params.ctx, fnID) + if err != nil { + return nil, false, err + } + return fnDesc, false, nil + } + + funcDescID, err := params.EvalContext().DescIDGenerator.GenerateUniqueDescID(params.ctx) + if err != nil { + return nil, false, err } + returnType, err := tree.ResolveType(params.ctx, n.cf.ReturnType.Type, params.p) + if err != nil { + return nil, false, err + } + + privileges := catprivilege.CreatePrivilegesFromDefaultPrivileges( + n.dbDesc.GetDefaultPrivilegeDescriptor(), + scDesc.GetDefaultPrivilegeDescriptor(), + n.dbDesc.GetID(), + params.SessionData().User(), + privilege.Functions, + n.dbDesc.GetPrivileges(), + ) + + newUdfDesc := funcdesc.NewMutableFunctionDescriptor( + funcDescID, + n.dbDesc.GetID(), + scDesc.GetID(), + string(n.cf.FuncName.ObjectName), + pbArgs, + returnType, + n.cf.ReturnType.IsSet, + privileges, + ) + + return &newUdfDesc, true, nil +} + +func (n *createFunctionNode) addUDFReferences(udfDesc *funcdesc.Mutable, params runParams) error { // Get all table IDs for which we need to update back references, including // tables used directly in function body or as implicit types. - backrefTblIDs := make([]descpb.ID, 0, len(n.planDeps)+len(n.typeDeps)) - implicitTypeTblIDs := make(map[descpb.ID]struct{}, len(n.typeDeps)) + backrefTblIDs := catalog.DescriptorIDSet{} + implicitTypeTblIDs := catalog.DescriptorIDSet{} for id := range n.planDeps { - backrefTblIDs = append(backrefTblIDs, id) + backrefTblIDs.Add(id) } for id := range n.typeDeps { if isTable, err := params.p.descIsTable(params.ctx, id); err != nil { return err } else if isTable { - backrefTblIDs = append(backrefTblIDs, id) - implicitTypeTblIDs[id] = struct{}{} + backrefTblIDs.Add(id) + implicitTypeTblIDs.Add(id) } } // Read all referenced tables and update their dependencies. - backRefMutables := make(map[descpb.ID]*tabledesc.Mutable, len(backrefTblIDs)) - for _, id := range backrefTblIDs { - if _, ok := backRefMutables[id]; ok { - continue - } + backRefMutables := make(map[descpb.ID]*tabledesc.Mutable) + for _, id := range backrefTblIDs.Ordered() { backRefMutable, err := params.p.Descriptors().GetMutableTableByID( params.ctx, params.p.txn, id, tree.ObjectLookupFlagsWithRequired(), ) @@ -137,7 +327,7 @@ func (n *createFunctionNode) startExec(params runParams) error { for id, updated := range n.planDeps { backRefMutable := backRefMutables[id] for _, dep := range updated.deps { - dep.ID = udfMutableDesc.ID + dep.ID = udfDesc.ID dep.ByID = updated.desc.IsSequence() backRefMutable.DependedOnBy = append(backRefMutable.DependedOnBy, dep) } @@ -153,9 +343,9 @@ func (n *createFunctionNode) startExec(params runParams) error { return err } } - for id := range implicitTypeTblIDs { + for _, id := range implicitTypeTblIDs.Ordered() { backRefMutable := backRefMutables[id] - backRefMutable.DependedOnBy = append(backRefMutable.DependedOnBy, descpb.TableDescriptor_Reference{ID: udfMutableDesc.ID}) + backRefMutable.DependedOnBy = append(backRefMutable.DependedOnBy, descpb.TableDescriptor_Reference{ID: udfDesc.ID}) if err := params.p.writeSchemaChange( params.ctx, backRefMutable, @@ -171,115 +361,26 @@ func (n *createFunctionNode) startExec(params runParams) error { // Add type back references. Skip table implicit types (we update table back // references above). for id := range n.typeDeps { - if _, ok := implicitTypeTblIDs[id]; ok { + if implicitTypeTblIDs.Contains(id) { continue } - jobDesc := fmt.Sprintf("updating type back reference %d for function %d", id, udfMutableDesc.ID) - if err := params.p.addTypeBackReference(params.ctx, id, udfMutableDesc.ID, jobDesc); err != nil { + jobDesc := fmt.Sprintf("updating type back reference %d for function %d", id, udfDesc.ID) + if err := params.p.addTypeBackReference(params.ctx, id, udfDesc.ID, jobDesc); err != nil { return err } } // Add forward references to UDF descriptor. - udfMutableDesc.DependsOn = make([]descpb.ID, 0, len(backrefTblIDs)) - udfMutableDesc.DependsOn = append(udfMutableDesc.DependsOn, backrefTblIDs...) - - sort.Sort(descpb.IDs(udfMutableDesc.DependsOn)) + udfDesc.DependsOn = backrefTblIDs.Ordered() - udfMutableDesc.DependsOnTypes = []descpb.ID{} + typeDepIDs := catalog.DescriptorIDSet{} for id := range n.typeDeps { - if _, ok := implicitTypeTblIDs[id]; ok { - continue - } - udfMutableDesc.DependsOnTypes = append(udfMutableDesc.DependsOnTypes, id) - } - sort.Sort(descpb.IDs(udfMutableDesc.DependsOnTypes)) - - err = params.p.createDescriptorWithID( - params.ctx, - roachpb.Key{}, // UDF does not have namespace entry. - udfMutableDesc.GetID(), - udfMutableDesc, - tree.AsStringWithFQNames(&n.cf.FuncName, params.Ann()), - ) - if err != nil { - return err - } - - returnType, err := tree.ResolveType(params.ctx, n.cf.ReturnType.Type, params.p) - if err != nil { - return err - } - argTypes := make([]*types.T, len(udfMutableDesc.Args)) - for i, arg := range udfMutableDesc.Args { - argTypes[i] = arg.Type + typeDepIDs.Add(id) } - mutScDesc.AddFunction( - udfMutableDesc.GetName(), - descpb.SchemaDescriptor_FunctionOverload{ - ID: udfMutableDesc.GetID(), - ArgTypes: argTypes, - ReturnType: returnType, - ReturnSet: udfMutableDesc.ReturnType.ReturnSet, - }, - ) - if err := params.p.writeSchemaDescChange(params.ctx, mutScDesc, "Create Function"); err != nil { - return err - } - + udfDesc.DependsOnTypes = typeDepIDs.Difference(implicitTypeTblIDs).Ordered() return nil } -func (*createFunctionNode) Next(params runParams) (bool, error) { return false, nil } -func (*createFunctionNode) Values() tree.Datums { return tree.Datums{} } -func (*createFunctionNode) Close(ctx context.Context) {} - -func (n *createFunctionNode) getMutableFuncDesc( - scDesc catalog.SchemaDescriptor, params runParams, -) (*funcdesc.Mutable, error) { - if n.cf.Replace { - return nil, unimplemented.New("CREATE OR REPLACE FUNCTION", "replacing function") - } - // TODO (Chengxiong) add function resolution and check if it's a Replace. - // Also: - // (1) add validation that return type can't be change. - // (2) add validation that argument names can't be change. - // (3) add validation that if existing function is referenced then it cannot be replace. - // (4) add `if` branch so that we only create new descriptor when it's not a replace. - - funcDescID, err := params.EvalContext().DescIDGenerator.GenerateUniqueDescID(params.ctx) - if err != nil { - return nil, err - } - - returnType, err := tree.ResolveType(params.ctx, n.cf.ReturnType.Type, params.p) - if err != nil { - return nil, err - } - - privileges := catprivilege.CreatePrivilegesFromDefaultPrivileges( - n.dbDesc.GetDefaultPrivilegeDescriptor(), - scDesc.GetDefaultPrivilegeDescriptor(), - n.dbDesc.GetID(), - params.SessionData().User(), - privilege.Functions, - n.dbDesc.GetPrivileges(), - ) - - newUdfDesc := funcdesc.NewMutableFunctionDescriptor( - funcDescID, - n.dbDesc.GetID(), - scDesc.GetID(), - string(n.cf.FuncName.ObjectName), - len(n.cf.Args), - returnType, - n.cf.ReturnType.IsSet, - privileges, - ) - - return &newUdfDesc, nil -} - func setFuncOption(params runParams, udfDesc *funcdesc.Mutable, option tree.FunctionOption) error { switch t := option.(type) { case tree.FunctionVolatility: diff --git a/pkg/sql/create_function_test.go b/pkg/sql/create_function_test.go index c2fa349f3bdf..4b61c46655ea 100644 --- a/pkg/sql/create_function_test.go +++ b/pkg/sql/create_function_test.go @@ -220,3 +220,155 @@ func TestCreateFunctionGating(t *testing.T) { require.Equal(t, "pq: cannot run CREATE FUNCTION before system is fully upgraded to v22.2", err.Error()) }) } + +func TestCreateOrReplaceFunctionUpdateReferences(t *testing.T) { + defer leaktest.AfterTest(t)() + defer log.Scope(t).Close(t) + + ctx := context.Background() + s, sqlDB, _ := serverutils.StartServer(t, base.TestServerArgs{}) + defer s.Stopper().Stop(ctx) + tDB := sqlutils.MakeSQLRunner(sqlDB) + + validateReferences := func( + ctx context.Context, txn *kv.Txn, col *descs.Collection, nonEmptyRelID string, emptyRelID string, + ) { + // Make sure columns and indexes has correct back references. + tn := tree.MakeTableNameWithSchema("defaultdb", "public", tree.Name("t"+nonEmptyRelID)) + _, tbl, err := col.GetImmutableTableByName(ctx, txn, &tn, tree.ObjectLookupFlagsWithRequired()) + require.NoError(t, err) + require.Equal(t, + []descpb.TableDescriptor_Reference{{ID: 112, IndexID: 2, ColumnIDs: []catid.ColumnID{2}}}, + tbl.GetDependedOnBy()) + + // Make sure sequence has correct back references. + sqn := tree.MakeTableNameWithSchema("defaultdb", "public", tree.Name("sq"+nonEmptyRelID)) + _, seq, err := col.GetImmutableTableByName(ctx, txn, &sqn, tree.ObjectLookupFlagsWithRequired()) + require.NoError(t, err) + require.Equal(t, []descpb.TableDescriptor_Reference{{ID: 112, ByID: true}}, seq.GetDependedOnBy()) + + // Make sure view has empty back references. + vn := tree.MakeTableNameWithSchema("defaultdb", "public", tree.Name("v"+nonEmptyRelID)) + _, view, err := col.GetImmutableTableByName(ctx, txn, &vn, tree.ObjectLookupFlagsWithRequired()) + require.NoError(t, err) + require.Equal(t, + []descpb.TableDescriptor_Reference{{ID: 112, ColumnIDs: []catid.ColumnID{1}}}, + view.GetDependedOnBy()) + + // Make sure columns and indexes has empty back references. + tn = tree.MakeTableNameWithSchema("defaultdb", "public", tree.Name("t"+emptyRelID)) + _, tbl, err = col.GetImmutableTableByName(ctx, txn, &tn, tree.ObjectLookupFlagsWithRequired()) + require.NoError(t, err) + require.Nil(t, tbl.GetDependedOnBy()) + + // Make sure sequence has empty back references. + sqn = tree.MakeTableNameWithSchema("defaultdb", "public", tree.Name("sq"+emptyRelID)) + _, seq, err = col.GetImmutableTableByName(ctx, txn, &sqn, tree.ObjectLookupFlagsWithRequired()) + require.NoError(t, err) + require.Nil(t, seq.GetDependedOnBy()) + + // Make sure view has emtpy back references. + vn = tree.MakeTableNameWithSchema("defaultdb", "public", tree.Name("v"+emptyRelID)) + _, view, err = col.GetImmutableTableByName(ctx, txn, &vn, tree.ObjectLookupFlagsWithRequired()) + require.NoError(t, err) + require.Nil(t, view.GetDependedOnBy()) + } + + tDB.Exec(t, ` +CREATE TABLE t1(a INT PRIMARY KEY, b INT, INDEX t1_idx_b(b)); +CREATE TABLE t2(a INT PRIMARY KEY, b INT, INDEX t2_idx_b(b)); +CREATE SEQUENCE sq1; +CREATE SEQUENCE sq2; +CREATE VIEW v1 AS SELECT 1; +CREATE VIEW v2 AS SELECT 2; +CREATE TYPE notmyworkday AS ENUM ('Monday', 'Tuesday'); +CREATE FUNCTION f(a notmyworkday) RETURNS INT IMMUTABLE LANGUAGE SQL AS $$ + SELECT b FROM t1@t1_idx_b; + SELECT a FROM v1; + SELECT nextval('sq1'); +$$; +`, + ) + + err := sql.TestingDescsTxn(ctx, s, func(ctx context.Context, txn *kv.Txn, col *descs.Collection) error { + funcDesc, err := col.GetImmutableFunctionByID(ctx, txn, 112, tree.ObjectLookupFlagsWithRequired()) + require.NoError(t, err) + require.Equal(t, funcDesc.GetName(), "f") + + require.Equal(t, + `SELECT b FROM defaultdb.public.t1@t1_idx_b; +SELECT a FROM defaultdb.public.v1; +SELECT nextval(106:::REGCLASS);`, + funcDesc.GetFunctionBody()) + + sort.Slice(funcDesc.GetDependsOn(), func(i, j int) bool { + return funcDesc.GetDependsOn()[i] < funcDesc.GetDependsOn()[j] + }) + require.Equal(t, []descpb.ID{104, 106, 108}, funcDesc.GetDependsOn()) + sort.Slice(funcDesc.GetDependsOnTypes(), func(i, j int) bool { + return funcDesc.GetDependsOnTypes()[i] < funcDesc.GetDependsOnTypes()[j] + }) + require.Equal(t, []descpb.ID{110, 111}, funcDesc.GetDependsOnTypes()) + + // Make sure type has correct back references. + typn := tree.MakeQualifiedTypeName("defaultdb", "public", "notmyworkday") + _, typ, err := col.GetImmutableTypeByName(ctx, txn, &typn, tree.ObjectLookupFlagsWithRequired()) + require.NoError(t, err) + require.Equal(t, []descpb.ID{112}, typ.GetReferencingDescriptorIDs()) + + // All objects with "1" suffix should have back references to the function, + // "2" should have empty references since it's not used yet. + validateReferences(ctx, txn, col, "1", "2") + return nil + }) + require.NoError(t, err) + + // Replace the function body with another group of objects and make sure + // references are modified correctly. + tDB.Exec(t, ` +CREATE OR REPLACE FUNCTION f(a notmyworkday) RETURNS INT IMMUTABLE LANGUAGE SQL AS $$ + SELECT b FROM t2@t2_idx_b; + SELECT a FROM v2; + SELECT nextval('sq2'); +$$; +`) + + err = sql.TestingDescsTxn(ctx, s, func(ctx context.Context, txn *kv.Txn, col *descs.Collection) error { + flags := tree.ObjectLookupFlags{ + CommonLookupFlags: tree.CommonLookupFlags{ + Required: true, + AvoidLeased: true, + }, + } + funcDesc, err := col.GetImmutableFunctionByID(ctx, txn, 112, flags) + require.NoError(t, err) + require.Equal(t, funcDesc.GetName(), "f") + + require.Equal(t, + `SELECT b FROM defaultdb.public.t2@t2_idx_b; +SELECT a FROM defaultdb.public.v2; +SELECT nextval(107:::REGCLASS);`, + funcDesc.GetFunctionBody()) + + sort.Slice(funcDesc.GetDependsOn(), func(i, j int) bool { + return funcDesc.GetDependsOn()[i] < funcDesc.GetDependsOn()[j] + }) + require.Equal(t, []descpb.ID{105, 107, 109}, funcDesc.GetDependsOn()) + sort.Slice(funcDesc.GetDependsOnTypes(), func(i, j int) bool { + return funcDesc.GetDependsOnTypes()[i] < funcDesc.GetDependsOnTypes()[j] + }) + require.Equal(t, []descpb.ID{110, 111}, funcDesc.GetDependsOnTypes()) + + // Make sure type has correct back references. + typn := tree.MakeQualifiedTypeName("defaultdb", "public", "notmyworkday") + _, typ, err := col.GetImmutableTypeByName(ctx, txn, &typn, tree.ObjectLookupFlagsWithRequired()) + require.NoError(t, err) + require.Equal(t, []descpb.ID{112}, typ.GetReferencingDescriptorIDs()) + + // Now all objects with "2" suffix in name should have back references "1" + // had before, and "1" should have empty references. + validateReferences(ctx, txn, col, "2", "1") + return nil + }) + require.NoError(t, err) +} diff --git a/pkg/sql/function_resolver_test.go b/pkg/sql/function_resolver_test.go index cf3543c2c841..52d648e07727 100644 --- a/pkg/sql/function_resolver_test.go +++ b/pkg/sql/function_resolver_test.go @@ -64,7 +64,7 @@ CREATE FUNCTION f(a notmyworkday) RETURNS INT IMMUTABLE LANGUAGE SQL AS $$ SELECT nextval('sq1'); $$; CREATE FUNCTION f() RETURNS VOID IMMUTABLE LANGUAGE SQL AS $$ SELECT 1 $$; -CREATE FUNCTION f() RETURNS t IMMUTABLE LANGUAGE SQL AS $$ SELECT a, b, c FROM t $$; +CREATE FUNCTION f(INT) RETURNS t IMMUTABLE LANGUAGE SQL AS $$ SELECT a, b, c FROM t $$; `) var sessionData sessiondatapb.SessionData @@ -114,7 +114,8 @@ CREATE FUNCTION f() RETURNS t IMMUTABLE LANGUAGE SQL AS $$ SELECT a, b, c FROM t require.Equal(t, 100112, int(funcDef.Overloads[2].Oid)) require.True(t, funcDef.Overloads[2].UDFContainsOnlySignature) require.True(t, funcDef.Overloads[2].IsUDF) - require.Equal(t, 0, len(funcDef.Overloads[2].Types.Types())) + require.Equal(t, 1, len(funcDef.Overloads[2].Types.Types())) + require.Equal(t, types.Int, funcDef.Overloads[2].Types.Types()[0]) require.Equal(t, types.TupleFamily, funcDef.Overloads[2].ReturnType([]tree.TypedExpr{}).Family()) require.NotZero(t, funcDef.Overloads[2].ReturnType([]tree.TypedExpr{}).TypeMeta) @@ -145,7 +146,8 @@ SELECT nextval(105:::REGCLASS);`, overload.Body) require.Equal(t, `SELECT a, b, c FROM defaultdb.public.t;`, overload.Body) require.True(t, overload.IsUDF) require.False(t, overload.UDFContainsOnlySignature) - require.Equal(t, 0, len(overload.Types.Types())) + require.Equal(t, 1, len(overload.Types.Types())) + require.Equal(t, types.Int, overload.Types.Types()[0]) require.Equal(t, types.TupleFamily, overload.ReturnType([]tree.TypedExpr{}).Family()) require.NotZero(t, overload.ReturnType([]tree.TypedExpr{}).TypeMeta) diff --git a/pkg/sql/logictest/testdata/logic_test/udf b/pkg/sql/logictest/testdata/logic_test/udf index 9f0219e2206f..19df9c714649 100644 --- a/pkg/sql/logictest/testdata/logic_test/udf +++ b/pkg/sql/logictest/testdata/logic_test/udf @@ -2,13 +2,10 @@ statement ok CREATE TABLE ab ( - a INT PRIMARY KEY, - b INT +a INT PRIMARY KEY, +b INT ) -statement error pq: unimplemented: replacing function -CREATE OR REPLACE FUNCTION f(a int) RETURNS INT LANGUAGE SQL AS 'SELECT 1' - statement error pq: cannot create leakproof function with non-immutable volatility: STABLE CREATE FUNCTION f(a int) RETURNS INT LEAKPROOF STABLE LANGUAGE SQL AS 'SELECT 1' @@ -61,153 +58,50 @@ statement error pq: return type mismatch in function declared to return int\nDET CREATE FUNCTION f() RETURNS INT IMMUTABLE LANGUAGE SQL AS $$ SELECT a, b from t_implicit_type $$ statement ok -CREATE FUNCTION f() RETURNS t_implicit_type IMMUTABLE LANGUAGE SQL AS $$ SELECT * from t_implicit_type $$ +CREATE FUNCTION f_star() RETURNS t_implicit_type IMMUTABLE LANGUAGE SQL AS $$ SELECT * from t_implicit_type $$ statement ok -CREATE FUNCTION f() RETURNS t_implicit_type IMMUTABLE LANGUAGE SQL AS $$ SELECT a, b from t_implicit_type $$ - -let $max_desc_id -SELECT max_desc_id FROM [SELECT max(id) as max_desc_id FROM system.descriptor]; +CREATE FUNCTION f_by_cols() RETURNS t_implicit_type IMMUTABLE LANGUAGE SQL AS $$ SELECT a, b from t_implicit_type $$ -# TODO (Chengxiong) replace this test with `SHOW CREATE FUNCTION` when we have -# function resolution in place. query T -SELECT jsonb_pretty( - crdb_internal.pb_to_json('cockroach.sql.sqlbase.Descriptor', descriptor, false) -)::string -FROM system.descriptor -WHERE id = $max_desc_id; ----- -{ - "function": { - "dependsOn": [ - 112, - 112 - ], - "functionBody": "SELECT a, b FROM test.public.t_implicit_type;", - "id": 114, - "lang": "SQL", - "modificationTime": {}, - "name": "f", - "nullInputBehavior": "CALLED_ON_NULL_INPUT", - "parentId": 104, - "parentSchemaId": 105, - "privileges": { - "ownerProto": "root", - "users": [ - { - "privileges": 2, - "userProto": "admin", - "withGrantOption": 2 - }, - { - "privileges": 2, - "userProto": "root", - "withGrantOption": 2 - } - ], - "version": 2 - }, - "returnType": { - "type": { - "family": "TupleFamily", - "oid": 100112, - "tupleContents": [ - { - "family": "IntFamily", - "oid": 20, - "width": 64 - }, - { - "family": "StringFamily", - "oid": 25 - } - ], - "tupleLabels": [ - "a", - "b" - ] - } - }, - "version": "1", - "volatility": "IMMUTABLE" - } -} +SELECT @2 FROM [SHOW CREATE FUNCTION f_by_cols]; +---- +CREATE FUNCTION public.f_by_cols() + RETURNS T_IMPLICIT_TYPE + IMMUTABLE + NOT LEAKPROOF + CALLED ON NULL INPUT + LANGUAGE SQL + AS $$ + SELECT a, b FROM test.public.t_implicit_type; +$$ # Create function with no references. statement ok -CREATE FUNCTION f(a int) RETURNS INT IMMUTABLE AS 'SELECT 1' LANGUAGE SQL +CREATE FUNCTION f_no_ref(a int) RETURNS INT IMMUTABLE AS 'SELECT 1' LANGUAGE SQL -let $max_desc_id -SELECT max_desc_id FROM [SELECT max(id) as max_desc_id FROM system.descriptor]; - -# TODO (Chengxiong) replace this test with `SHOW CREATE FUNCTION` when we have -# function resolution in place. query T -SELECT jsonb_pretty( - crdb_internal.pb_to_json('cockroach.sql.sqlbase.Descriptor', descriptor, false) -)::string -FROM system.descriptor -WHERE id = $max_desc_id; ----- -{ - "function": { - "args": [ - { - "class": "IN", - "name": "a", - "type": { - "family": "IntFamily", - "oid": 20, - "width": 64 - } - } - ], - "functionBody": "SELECT 1;", - "id": 115, - "lang": "SQL", - "modificationTime": {}, - "name": "f", - "nullInputBehavior": "CALLED_ON_NULL_INPUT", - "parentId": 104, - "parentSchemaId": 105, - "privileges": { - "ownerProto": "root", - "users": [ - { - "privileges": 2, - "userProto": "admin", - "withGrantOption": 2 - }, - { - "privileges": 2, - "userProto": "root", - "withGrantOption": 2 - } - ], - "version": 2 - }, - "returnType": { - "type": { - "family": "IntFamily", - "oid": 20, - "width": 64 - } - }, - "version": "1", - "volatility": "IMMUTABLE" - } -} +SELECT @2 FROM [SHOW CREATE FUNCTION f_no_ref]; +---- +CREATE FUNCTION public.f_no_ref(IN a INT8) + RETURNS INT8 + IMMUTABLE + NOT LEAKPROOF + CALLED ON NULL INPUT + LANGUAGE SQL + AS $$ + SELECT 1; +$$ # Make sure that names are qualified, references are tracked and sequence # expression is rewritten. statement ok CREATE TABLE t( - a INT PRIMARY KEY, - b INT, - C INT, - INDEX t_idx_b(b), - INDEX t_idx_c(c) +a INT PRIMARY KEY, +b INT, +C INT, +INDEX t_idx_b(b), +INDEX t_idx_c(c) ); statement ok @@ -218,82 +112,27 @@ CREATE TYPE notmyworkday AS ENUM ('Monday', 'Tuesday'); statement ok CREATE FUNCTION f(a notmyworkday) RETURNS INT IMMUTABLE LANGUAGE SQL AS $$ - SELECT a FROM t; - SELECT b FROM t@t_idx_b; - SELECT c FROM t@t_idx_c; - SELECT nextval('sq1'); +SELECT a FROM t; +SELECT b FROM t@t_idx_b; +SELECT c FROM t@t_idx_c; +SELECT nextval('sq1'); $$ -let $max_desc_id -SELECT max_desc_id FROM [SELECT max(id) as max_desc_id FROM system.descriptor]; - -# TODO (Chengxiong) replace this test with `SHOW CREATE FUNCTION` when we have -# function resolution in place. query T -SELECT jsonb_pretty( - crdb_internal.pb_to_json('cockroach.sql.sqlbase.Descriptor', descriptor, false) -)::string -FROM system.descriptor -WHERE id = $max_desc_id; ----- -{ - "function": { - "args": [ - { - "class": "IN", - "name": "a", - "type": { - "family": "EnumFamily", - "oid": 100118, - "udtMetadata": { - "arrayTypeOid": 100119 - } - } - } - ], - "dependsOn": [ - 116, - 117 - ], - "dependsOnTypes": [ - 118, - 119 - ], - "functionBody": "SELECT a FROM test.public.t;\nSELECT b FROM test.public.t@t_idx_b;\nSELECT c FROM test.public.t@t_idx_c;\nSELECT nextval(117:::REGCLASS);", - "id": 120, - "lang": "SQL", - "modificationTime": {}, - "name": "f", - "nullInputBehavior": "CALLED_ON_NULL_INPUT", - "parentId": 104, - "parentSchemaId": 105, - "privileges": { - "ownerProto": "root", - "users": [ - { - "privileges": 2, - "userProto": "admin", - "withGrantOption": 2 - }, - { - "privileges": 2, - "userProto": "root", - "withGrantOption": 2 - } - ], - "version": 2 - }, - "returnType": { - "type": { - "family": "IntFamily", - "oid": 20, - "width": 64 - } - }, - "version": "1", - "volatility": "IMMUTABLE" - } -} +SELECT @2 FROM [SHOW CREATE FUNCTION f]; +---- +CREATE FUNCTION public.f(IN a test.public.notmyworkday) + RETURNS INT8 + IMMUTABLE + NOT LEAKPROOF + CALLED ON NULL INPUT + LANGUAGE SQL + AS $$ + SELECT a FROM test.public.t; + SELECT b FROM test.public.t@t_idx_b; + SELECT c FROM test.public.t@t_idx_c; + SELECT nextval(117:::REGCLASS); +$$ statement error pq: unimplemented: alter function depends on extension not supported.* ALTER FUNCTION f() DEPENDS ON EXTENSION postgis @@ -333,31 +172,31 @@ WHERE function_name IN ('proc_f', 'proc_f_2') ORDER BY function_name; ---- CREATE FUNCTION public.proc_f(IN INT8) - RETURNS INT8 - VOLATILE - NOT LEAKPROOF - CALLED ON NULL INPUT - LANGUAGE SQL - AS $$ - SELECT 1; + RETURNS INT8 + VOLATILE + NOT LEAKPROOF + CALLED ON NULL INPUT + LANGUAGE SQL + AS $$ + SELECT 1; $$ 104 test 105 public 121 proc_f CREATE FUNCTION public.proc_f(IN STRING, IN b INT8) - RETURNS SETOF STRING - IMMUTABLE - LEAKPROOF - STRICT - LANGUAGE SQL - AS $$ - SELECT 'hello'; + RETURNS SETOF STRING + IMMUTABLE + LEAKPROOF + STRICT + LANGUAGE SQL + AS $$ + SELECT 'hello'; $$ 104 test 105 public 122 proc_f CREATE FUNCTION sc.proc_f_2(IN STRING) - RETURNS STRING - VOLATILE - NOT LEAKPROOF - CALLED ON NULL INPUT - LANGUAGE SQL - AS $$ - SELECT 'hello'; + RETURNS STRING + VOLATILE + NOT LEAKPROOF + CALLED ON NULL INPUT + LANGUAGE SQL + AS $$ + SELECT 'hello'; $$ 104 test 124 sc 125 proc_f_2 statement ok @@ -373,40 +212,40 @@ WHERE function_name IN ('proc_f', 'proc_f_2', 'f_cross_db') ORDER BY database_id, function_name; ---- CREATE FUNCTION public.proc_f(IN INT8) - RETURNS INT8 - VOLATILE - NOT LEAKPROOF - CALLED ON NULL INPUT - LANGUAGE SQL - AS $$ - SELECT 1; + RETURNS INT8 + VOLATILE + NOT LEAKPROOF + CALLED ON NULL INPUT + LANGUAGE SQL + AS $$ + SELECT 1; $$ 104 test 105 public 121 proc_f CREATE FUNCTION public.proc_f(IN STRING, IN b INT8) - RETURNS SETOF STRING - IMMUTABLE - LEAKPROOF - STRICT - LANGUAGE SQL - AS $$ - SELECT 'hello'; + RETURNS SETOF STRING + IMMUTABLE + LEAKPROOF + STRICT + LANGUAGE SQL + AS $$ + SELECT 'hello'; $$ 104 test 105 public 122 proc_f CREATE FUNCTION sc.proc_f_2(IN STRING) - RETURNS STRING - VOLATILE - NOT LEAKPROOF - CALLED ON NULL INPUT - LANGUAGE SQL - AS $$ - SELECT 'hello'; + RETURNS STRING + VOLATILE + NOT LEAKPROOF + CALLED ON NULL INPUT + LANGUAGE SQL + AS $$ + SELECT 'hello'; $$ 104 test 124 sc 125 proc_f_2 CREATE FUNCTION public.f_cross_db() - RETURNS INT8 - VOLATILE - NOT LEAKPROOF - CALLED ON NULL INPUT - LANGUAGE SQL - AS $$ - SELECT 1; + RETURNS INT8 + VOLATILE + NOT LEAKPROOF + CALLED ON NULL INPUT + LANGUAGE SQL + AS $$ + SELECT 1; $$ 126 test_cross_db 127 public 128 f_cross_db subtest show_create_function @@ -415,22 +254,22 @@ query T SELECT @2 FROM [SHOW CREATE FUNCTION proc_f]; ---- CREATE FUNCTION public.proc_f(IN INT8) - RETURNS INT8 - VOLATILE - NOT LEAKPROOF - CALLED ON NULL INPUT - LANGUAGE SQL - AS $$ - SELECT 1; + RETURNS INT8 + VOLATILE + NOT LEAKPROOF + CALLED ON NULL INPUT + LANGUAGE SQL + AS $$ + SELECT 1; $$ CREATE FUNCTION public.proc_f(IN STRING, IN b INT8) - RETURNS SETOF STRING - IMMUTABLE - LEAKPROOF - STRICT - LANGUAGE SQL - AS $$ - SELECT 'hello'; + RETURNS SETOF STRING + IMMUTABLE + LEAKPROOF + STRICT + LANGUAGE SQL + AS $$ + SELECT 'hello'; $$ statement error pq: unknown function: proc_f_2() @@ -440,13 +279,13 @@ query T SELECT @2 FROM [SHOW CREATE FUNCTION sc.proc_f_2]; ---- CREATE FUNCTION sc.proc_f_2(IN STRING) - RETURNS STRING - VOLATILE - NOT LEAKPROOF - CALLED ON NULL INPUT - LANGUAGE SQL - AS $$ - SELECT 'hello'; + RETURNS STRING + VOLATILE + NOT LEAKPROOF + CALLED ON NULL INPUT + LANGUAGE SQL + AS $$ + SELECT 'hello'; $$ statement ok @@ -456,13 +295,13 @@ query T SELECT @2 FROM [SHOW CREATE FUNCTION proc_f_2]; ---- CREATE FUNCTION sc.proc_f_2(IN STRING) - RETURNS STRING - VOLATILE - NOT LEAKPROOF - CALLED ON NULL INPUT - LANGUAGE SQL - AS $$ - SELECT 'hello'; + RETURNS STRING + VOLATILE + NOT LEAKPROOF + CALLED ON NULL INPUT + LANGUAGE SQL + AS $$ + SELECT 'hello'; $$ statement ok @@ -537,35 +376,35 @@ query T SELECT @2 FROM [SHOW CREATE FUNCTION public.f_test_drop]; ---- CREATE FUNCTION public.f_test_drop() - RETURNS INT8 - VOLATILE - NOT LEAKPROOF - CALLED ON NULL INPUT - LANGUAGE SQL - AS $$ - SELECT 1; + RETURNS INT8 + VOLATILE + NOT LEAKPROOF + CALLED ON NULL INPUT + LANGUAGE SQL + AS $$ + SELECT 1; $$ CREATE FUNCTION public.f_test_drop(IN INT8) - RETURNS INT8 - VOLATILE - NOT LEAKPROOF - CALLED ON NULL INPUT - LANGUAGE SQL - AS $$ - SELECT 1; + RETURNS INT8 + VOLATILE + NOT LEAKPROOF + CALLED ON NULL INPUT + LANGUAGE SQL + AS $$ + SELECT 1; $$ query T SELECT @2 FROM [SHOW CREATE FUNCTION sc1.f_test_drop]; ---- CREATE FUNCTION sc1.f_test_drop(IN INT8) - RETURNS INT8 - VOLATILE - NOT LEAKPROOF - CALLED ON NULL INPUT - LANGUAGE SQL - AS $$ - SELECT 1; + RETURNS INT8 + VOLATILE + NOT LEAKPROOF + CALLED ON NULL INPUT + LANGUAGE SQL + AS $$ + SELECT 1; $$ statement error pq: function name \"f_test_drop\" is not unique @@ -594,26 +433,26 @@ query T SELECT @2 FROM [SHOW CREATE FUNCTION public.f_test_drop]; ---- CREATE FUNCTION public.f_test_drop(IN INT8) - RETURNS INT8 - VOLATILE - NOT LEAKPROOF - CALLED ON NULL INPUT - LANGUAGE SQL - AS $$ - SELECT 1; + RETURNS INT8 + VOLATILE + NOT LEAKPROOF + CALLED ON NULL INPUT + LANGUAGE SQL + AS $$ + SELECT 1; $$ query T SELECT @2 FROM [SHOW CREATE FUNCTION sc1.f_test_drop]; ---- CREATE FUNCTION sc1.f_test_drop(IN INT8) - RETURNS INT8 - VOLATILE - NOT LEAKPROOF - CALLED ON NULL INPUT - LANGUAGE SQL - AS $$ - SELECT 1; + RETURNS INT8 + VOLATILE + NOT LEAKPROOF + CALLED ON NULL INPUT + LANGUAGE SQL + AS $$ + SELECT 1; $$ # Drop with two identical function signatures should be ok. And only first match @@ -628,13 +467,13 @@ query T SELECT @2 FROM [SHOW CREATE FUNCTION sc1.f_test_drop]; ---- CREATE FUNCTION sc1.f_test_drop(IN INT8) - RETURNS INT8 - VOLATILE - NOT LEAKPROOF - CALLED ON NULL INPUT - LANGUAGE SQL - AS $$ - SELECT 1; + RETURNS INT8 + VOLATILE + NOT LEAKPROOF + CALLED ON NULL INPUT + LANGUAGE SQL + AS $$ + SELECT 1; $$ statement ok @@ -653,26 +492,26 @@ query T SELECT @2 FROM [SHOW CREATE FUNCTION public.f_test_drop]; ---- CREATE FUNCTION public.f_test_drop() - RETURNS INT8 - VOLATILE - NOT LEAKPROOF - CALLED ON NULL INPUT - LANGUAGE SQL - AS $$ - SELECT 1; + RETURNS INT8 + VOLATILE + NOT LEAKPROOF + CALLED ON NULL INPUT + LANGUAGE SQL + AS $$ + SELECT 1; $$ query T SELECT @2 FROM [SHOW CREATE FUNCTION sc1.f_test_drop]; ---- CREATE FUNCTION sc1.f_test_drop() - RETURNS INT8 - VOLATILE - NOT LEAKPROOF - CALLED ON NULL INPUT - LANGUAGE SQL - AS $$ - SELECT 1; + RETURNS INT8 + VOLATILE + NOT LEAKPROOF + CALLED ON NULL INPUT + LANGUAGE SQL + AS $$ + SELECT 1; $$ statement ok; @@ -766,13 +605,13 @@ query T SELECT @2 FROM [SHOW CREATE FUNCTION test_vf_f]; ---- CREATE FUNCTION public.test_vf_f() - RETURNS STRING - VOLATILE - NOT LEAKPROOF - CALLED ON NULL INPUT - LANGUAGE SQL - AS $$ - SELECT lower('hello'); + RETURNS STRING + VOLATILE + NOT LEAKPROOF + CALLED ON NULL INPUT + LANGUAGE SQL + AS $$ + SELECT lower('hello'); $$ subtest execution @@ -816,7 +655,7 @@ a b statement ok CREATE FUNCTION max_in_values() RETURNS INT LANGUAGE SQL AS $$ - SELECT i FROM (VALUES (1, 0), (2, 0), (3, 0)) AS v(i, j) ORDER BY i DESC +SELECT i FROM (VALUES (1, 0), (2, 0), (3, 0)) AS v(i, j) ORDER BY i DESC $$ query I @@ -826,8 +665,8 @@ SELECT max_in_values() statement ok CREATE FUNCTION fetch_one_then_two() RETURNS INT LANGUAGE SQL AS $$ - SELECT b FROM ab WHERE a = 1; - SELECT b FROM ab WHERE a = 2; +SELECT b FROM ab WHERE a = 1; +SELECT b FROM ab WHERE a = 2; $$ query II @@ -846,7 +685,7 @@ fetch_one_then_two statement ok CREATE TABLE empty (e INT); CREATE FUNCTION empty_result() RETURNS INT LANGUAGE SQL AS $$ - SELECT e FROM empty +SELECT e FROM empty $$ query I @@ -916,7 +755,7 @@ SELECT a * (3 + b - a) + a * b * a, add(mult(a, add(3, sub(b, a))), mult(a, mult statement ok CREATE FUNCTION fetch_b(arg_a INT) RETURNS INT LANGUAGE SQL AS $$ - SELECT b FROM ab WHERE a = arg_a +SELECT b FROM ab WHERE a = arg_a $$ query II @@ -948,32 +787,32 @@ statement ok CREATE TABLE kv (k INT PRIMARY KEY, v INT); INSERT INTO kv VALUES (1, 1), (2, 2), (3, 3); CREATE FUNCTION get_l(i INT) RETURNS INT IMMUTABLE LEAKPROOF LANGUAGE SQL AS $$ - SELECT v FROM kv WHERE k = i; +SELECT v FROM kv WHERE k = i; $$; CREATE FUNCTION get_i(i INT) RETURNS INT IMMUTABLE LANGUAGE SQL AS $$ - SELECT v FROM kv WHERE k = i; +SELECT v FROM kv WHERE k = i; $$; CREATE FUNCTION get_s(i INT) RETURNS INT STABLE LANGUAGE SQL AS $$ - SELECT v FROM kv WHERE k = i; +SELECT v FROM kv WHERE k = i; $$; CREATE FUNCTION get_v(i INT) RETURNS INT VOLATILE LANGUAGE SQL AS $$ - SELECT v FROM kv WHERE k = i; +SELECT v FROM kv WHERE k = i; $$; CREATE FUNCTION int_identity_v(i INT) RETURNS INT VOLATILE LANGUAGE SQL AS $$ - SELECT i; +SELECT i; $$; # Only the volatile functions should see the changes made by the UPDATE in the # CTE. query IIIIIIII colnames WITH u AS ( - UPDATE kv SET v = v + 10 RETURNING k + UPDATE kv SET v = v + 10 RETURNING k ) SELECT - get_l(k) l1, get_l(int_identity_v(k)) l2, - get_i(k) i1, get_i(int_identity_v(k)) i2, - get_s(k) s1, get_s(int_identity_v(k)) s2, - get_v(k) v1, get_v(int_identity_v(k)) v2 +get_l(k) l1, get_l(int_identity_v(k)) l2, +get_i(k) i1, get_i(int_identity_v(k)) i2, +get_s(k) s1, get_s(int_identity_v(k)) s2, +get_v(k) v1, get_v(int_identity_v(k)) v2 FROM u; ---- l1 l2 i1 i2 s1 s2 v1 v2 @@ -1171,13 +1010,13 @@ query T SELECT @2 FROM [SHOW CREATE FUNCTION f_test_alter_opt]; ---- CREATE FUNCTION public.f_test_alter_opt(IN INT8) - RETURNS INT8 - VOLATILE - NOT LEAKPROOF - CALLED ON NULL INPUT - LANGUAGE SQL - AS $$ - SELECT 1; + RETURNS INT8 + VOLATILE + NOT LEAKPROOF + CALLED ON NULL INPUT + LANGUAGE SQL + AS $$ + SELECT 1; $$ statement error pq: conflicting or redundant options @@ -1190,13 +1029,13 @@ query T SELECT @2 FROM [SHOW CREATE FUNCTION f_test_alter_opt]; ---- CREATE FUNCTION public.f_test_alter_opt(IN INT8) - RETURNS INT8 - IMMUTABLE - LEAKPROOF - STRICT - LANGUAGE SQL - AS $$ - SELECT 1; + RETURNS INT8 + IMMUTABLE + LEAKPROOF + STRICT + LANGUAGE SQL + AS $$ + SELECT 1; $$ subtest alter_function_name @@ -1214,13 +1053,13 @@ query T SELECT @2 FROM [SHOW CREATE FUNCTION f_test_alter_name]; ---- CREATE FUNCTION public.f_test_alter_name(IN INT8) - RETURNS INT8 - VOLATILE - NOT LEAKPROOF - CALLED ON NULL INPUT - LANGUAGE SQL - AS $$ - SELECT 1; + RETURNS INT8 + VOLATILE + NOT LEAKPROOF + CALLED ON NULL INPUT + LANGUAGE SQL + AS $$ + SELECT 1; $$ statement error pq: function f_test_alter_name\(IN INT8\) already exists in schema "public" @@ -1239,13 +1078,13 @@ query T SELECT @2 FROM [SHOW CREATE FUNCTION f_test_alter_name_new]; ---- CREATE FUNCTION public.f_test_alter_name_new(IN INT8) - RETURNS INT8 - VOLATILE - NOT LEAKPROOF - CALLED ON NULL INPUT - LANGUAGE SQL - AS $$ - SELECT 1; + RETURNS INT8 + VOLATILE + NOT LEAKPROOF + CALLED ON NULL INPUT + LANGUAGE SQL + AS $$ + SELECT 1; $$ statement ok @@ -1258,22 +1097,22 @@ query T SELECT @2 FROM [SHOW CREATE FUNCTION f_test_alter_name_diff_in]; ---- CREATE FUNCTION public.f_test_alter_name_diff_in() - RETURNS INT8 - VOLATILE - NOT LEAKPROOF - CALLED ON NULL INPUT - LANGUAGE SQL - AS $$ - SELECT 1; + RETURNS INT8 + VOLATILE + NOT LEAKPROOF + CALLED ON NULL INPUT + LANGUAGE SQL + AS $$ + SELECT 1; $$ CREATE FUNCTION public.f_test_alter_name_diff_in(IN INT8) - RETURNS INT8 - VOLATILE - NOT LEAKPROOF - CALLED ON NULL INPUT - LANGUAGE SQL - AS $$ - SELECT 1; + RETURNS INT8 + VOLATILE + NOT LEAKPROOF + CALLED ON NULL INPUT + LANGUAGE SQL + AS $$ + SELECT 1; $$ subtest alter_function_owner @@ -1349,13 +1188,13 @@ FROM pg_catalog.pg_proc WHERE proname IN ('f_test_sc'); query TT WITH fns AS ( - SELECT crdb_internal.pb_to_json('cockroach.sql.sqlbase.Descriptor', descriptor, false)->'function' AS fn - FROM system.descriptor - WHERE id IN (176, 177, 179) +SELECT crdb_internal.pb_to_json('cockroach.sql.sqlbase.Descriptor', descriptor, false)->'function' AS fn +FROM system.descriptor +WHERE id IN (176, 177, 179) ) SELECT - fn->'id', - fn->'parentSchemaId' +fn->'id', +fn->'parentSchemaId' FROM fns; ---- 176 105 @@ -1374,13 +1213,13 @@ ALTER FUNCTION f_test_sc(INT) SET SCHEMA public; query TT WITH fns AS ( - SELECT crdb_internal.pb_to_json('cockroach.sql.sqlbase.Descriptor', descriptor, false)->'function' AS fn - FROM system.descriptor - WHERE id IN (176, 177, 179) +SELECT crdb_internal.pb_to_json('cockroach.sql.sqlbase.Descriptor', descriptor, false)->'function' AS fn +FROM system.descriptor +WHERE id IN (176, 177, 179) ) SELECT - fn->'id', - fn->'parentSchemaId' +fn->'id', +fn->'parentSchemaId' FROM fns; ---- 176 105 @@ -1391,22 +1230,22 @@ query T SELECT @2 FROM [SHOW CREATE FUNCTION public.f_test_sc]; ---- CREATE FUNCTION public.f_test_sc() - RETURNS INT8 - VOLATILE - NOT LEAKPROOF - CALLED ON NULL INPUT - LANGUAGE SQL - AS $$ - SELECT 1; + RETURNS INT8 + VOLATILE + NOT LEAKPROOF + CALLED ON NULL INPUT + LANGUAGE SQL + AS $$ + SELECT 1; $$ CREATE FUNCTION public.f_test_sc(IN INT8) - RETURNS INT8 - VOLATILE - NOT LEAKPROOF - CALLED ON NULL INPUT - LANGUAGE SQL - AS $$ - SELECT 2; + RETURNS INT8 + VOLATILE + NOT LEAKPROOF + CALLED ON NULL INPUT + LANGUAGE SQL + AS $$ + SELECT 2; $$ # Make sure moving to another schema changes function's parentSchemaId and @@ -1416,13 +1255,13 @@ ALTER FUNCTION f_test_sc(INT) SET SCHEMA test_alter_sc; query TT WITH fns AS ( - SELECT crdb_internal.pb_to_json('cockroach.sql.sqlbase.Descriptor', descriptor, false)->'function' AS fn - FROM system.descriptor - WHERE id IN (176, 177, 179) +SELECT crdb_internal.pb_to_json('cockroach.sql.sqlbase.Descriptor', descriptor, false)->'function' AS fn +FROM system.descriptor +WHERE id IN (176, 177, 179) ) SELECT - fn->'id', - fn->'parentSchemaId' +fn->'id', +fn->'parentSchemaId' FROM fns; ---- 176 105 @@ -1433,33 +1272,143 @@ query T SELECT @2 FROM [SHOW CREATE FUNCTION public.f_test_sc]; ---- CREATE FUNCTION public.f_test_sc() - RETURNS INT8 - VOLATILE - NOT LEAKPROOF - CALLED ON NULL INPUT - LANGUAGE SQL - AS $$ - SELECT 1; + RETURNS INT8 + VOLATILE + NOT LEAKPROOF + CALLED ON NULL INPUT + LANGUAGE SQL + AS $$ + SELECT 1; $$ query T SELECT @2 FROM [SHOW CREATE FUNCTION test_alter_sc.f_test_sc]; ---- CREATE FUNCTION test_alter_sc.f_test_sc() - RETURNS INT8 - VOLATILE - NOT LEAKPROOF - CALLED ON NULL INPUT - LANGUAGE SQL - AS $$ - SELECT 3; + RETURNS INT8 + VOLATILE + NOT LEAKPROOF + CALLED ON NULL INPUT + LANGUAGE SQL + AS $$ + SELECT 3; $$ CREATE FUNCTION test_alter_sc.f_test_sc(IN INT8) - RETURNS INT8 - VOLATILE - NOT LEAKPROOF - CALLED ON NULL INPUT - LANGUAGE SQL - AS $$ - SELECT 2; + RETURNS INT8 + VOLATILE + NOT LEAKPROOF + CALLED ON NULL INPUT + LANGUAGE SQL + AS $$ + SELECT 2; +$$ + + +subtest create_or_replace_function + +statement error pq: parameter name "a" used more than once +CREATE FUNCTION f_test_cor(a INT, a INT) RETURNS INT IMMUTABLE LANGUAGE SQL AS $$ SELECT 1 $$; + +statement ok +CREATE FUNCTION f_test_cor(a INT, b INT) RETURNS INT IMMUTABLE LEAKPROOF STRICT LANGUAGE SQL AS $$ SELECT 1 $$; + +statement error pq: function "f_test_cor" already exists with same argument types +CREATE FUNCTION f_test_cor(a INT, b INT) RETURNS INT IMMUTABLE LANGUAGE SQL AS $$ SELECT 1 $$; + +statement ok +CREATE OR REPLACE FUNCTION f_test_cor_not_exist(a INT, b INT) RETURNS INT IMMUTABLE LANGUAGE SQL AS $$ SELECT 1 $$; + +statement error pq: cannot change name of input parameter "b" +CREATE OR REPLACE FUNCTION f_test_cor(a INT, c INT) RETURNS INT IMMUTABLE LANGUAGE SQL AS $$ SELECT 1 $$; + +statement error pq: cannot change return type of existing function +CREATE OR REPLACE FUNCTION f_test_cor(a INT, b INT) RETURNS STRING IMMUTABLE LANGUAGE SQL AS $$ SELECT 'hello' $$; + +statement error pq: cannot change return type of existing function +CREATE OR REPLACE FUNCTION f_test_cor(a INT, b INT) RETURNS SETOF INT IMMUTABLE LANGUAGE SQL AS $$ SELECT 1 $$; + +statement error pq: cannot create leakproof function with non-immutable volatility: VOLATILE +CREATE OR REPLACE FUNCTION f_test_cor(a INT, b INT) RETURNS INT LEAKPROOF LANGUAGE SQL AS $$ SELECT 1 $$; + +query T +SELECT @2 FROM [SHOW CREATE FUNCTION f_test_cor]; +---- +CREATE FUNCTION public.f_test_cor(IN a INT8, IN b INT8) + RETURNS INT8 + IMMUTABLE + LEAKPROOF + STRICT + LANGUAGE SQL + AS $$ + SELECT 1; +$$ + +# Make sure volatility, leakproof and null input behavior are default values +# after replacing with a definition not specifying them. +statement ok +CREATE OR REPLACE FUNCTION f_test_cor(a INT, b INT) RETURNS INT LANGUAGE SQL AS $$ SELECT 2 $$; + +query T +SELECT @2 FROM [SHOW CREATE FUNCTION f_test_cor]; +---- +CREATE FUNCTION public.f_test_cor(IN a INT8, IN b INT8) + RETURNS INT8 + VOLATILE + NOT LEAKPROOF + CALLED ON NULL INPUT + LANGUAGE SQL + AS $$ + SELECT 2; +$$ + +statement ok +CREATE OR REPLACE FUNCTION f_test_cor(a INT, b INT) RETURNS INT IMMUTABLE LEAKPROOF STRICT LANGUAGE SQL AS $$ SELECT 3 $$; + +query T +SELECT @2 FROM [SHOW CREATE FUNCTION f_test_cor]; +---- +CREATE FUNCTION public.f_test_cor(IN a INT8, IN b INT8) + RETURNS INT8 + IMMUTABLE + LEAKPROOF + STRICT + LANGUAGE SQL + AS $$ + SELECT 3; +$$ + +# Make sure function using implicit type can be replaced properly. +statement ok +CREATE FUNCTION f_test_cor_implicit() RETURNS t_implicit_type IMMUTABLE LANGUAGE SQL AS $$ SELECT a, b from t_implicit_type $$ + +query T +SELECT @2 FROM [SHOW CREATE FUNCTION f_test_cor_implicit]; +---- +CREATE FUNCTION public.f_test_cor_implicit() + RETURNS T_IMPLICIT_TYPE + IMMUTABLE + NOT LEAKPROOF + CALLED ON NULL INPUT + LANGUAGE SQL + AS $$ + SELECT a, b FROM test.public.t_implicit_type; +$$ + +statement error pq: function "f_test_cor_implicit" already exists with same argument types +CREATE FUNCTION f_test_cor_implicit() RETURNS t_implicit_type IMMUTABLE LANGUAGE SQL AS $$ SELECT a, b from t_implicit_type $$ + +statement ok +CREATE OR REPLACE FUNCTION f_test_cor_implicit() RETURNS t_implicit_type STABLE LANGUAGE SQL AS $$ SELECT a, b from t_implicit_type $$ + +query T +SELECT @2 FROM [SHOW CREATE FUNCTION f_test_cor_implicit]; +---- +CREATE FUNCTION public.f_test_cor_implicit() + RETURNS T_IMPLICIT_TYPE + STABLE + NOT LEAKPROOF + CALLED ON NULL INPUT + LANGUAGE SQL + AS $$ + SELECT a, b FROM test.public.t_implicit_type; $$ From 0ee5479199394b4be8c856094e18ba7d14ccfaf5 Mon Sep 17 00:00:00 2001 From: irfan sharif Date: Mon, 15 Aug 2022 18:34:35 -0400 Subject: [PATCH 7/8] grunning: improve some tests Release note: None Release justification: test-only changes --- pkg/util/grunning/BUILD.bazel | 39 ---------------------- pkg/util/grunning/enabled_test.go | 54 +++++++++++++++---------------- 2 files changed, 26 insertions(+), 67 deletions(-) diff --git a/pkg/util/grunning/BUILD.bazel b/pkg/util/grunning/BUILD.bazel index 9fe68e765990..ccbd400fd10c 100644 --- a/pkg/util/grunning/BUILD.bazel +++ b/pkg/util/grunning/BUILD.bazel @@ -22,49 +22,41 @@ go_test( "@io_bazel_rules_go//go/platform:aix_ppc64": [ ":grunning", "//pkg/testutils/skip", - "//pkg/util/syncutil", "@com_github_stretchr_testify//require", ], "@io_bazel_rules_go//go/platform:android_386": [ ":grunning", "//pkg/testutils/skip", - "//pkg/util/syncutil", "@com_github_stretchr_testify//require", ], "@io_bazel_rules_go//go/platform:android_amd64": [ ":grunning", "//pkg/testutils/skip", - "//pkg/util/syncutil", "@com_github_stretchr_testify//require", ], "@io_bazel_rules_go//go/platform:android_arm": [ ":grunning", "//pkg/testutils/skip", - "//pkg/util/syncutil", "@com_github_stretchr_testify//require", ], "@io_bazel_rules_go//go/platform:android_arm64": [ ":grunning", "//pkg/testutils/skip", - "//pkg/util/syncutil", "@com_github_stretchr_testify//require", ], "@io_bazel_rules_go//go/platform:darwin_386": [ ":grunning", "//pkg/testutils/skip", - "//pkg/util/syncutil", "@com_github_stretchr_testify//require", ], "@io_bazel_rules_go//go/platform:darwin_amd64": [ ":grunning", "//pkg/testutils/skip", - "//pkg/util/syncutil", "@com_github_stretchr_testify//require", ], "@io_bazel_rules_go//go/platform:darwin_arm": [ ":grunning", "//pkg/testutils/skip", - "//pkg/util/syncutil", "@com_github_stretchr_testify//require", ], "@io_bazel_rules_go//go/platform:darwin_arm64": [ @@ -74,7 +66,6 @@ go_test( "@io_bazel_rules_go//go/platform:dragonfly_amd64": [ ":grunning", "//pkg/testutils/skip", - "//pkg/util/syncutil", "@com_github_stretchr_testify//require", ], "@io_bazel_rules_go//go/platform:freebsd_386": [ @@ -96,13 +87,11 @@ go_test( "@io_bazel_rules_go//go/platform:illumos_amd64": [ ":grunning", "//pkg/testutils/skip", - "//pkg/util/syncutil", "@com_github_stretchr_testify//require", ], "@io_bazel_rules_go//go/platform:ios_amd64": [ ":grunning", "//pkg/testutils/skip", - "//pkg/util/syncutil", "@com_github_stretchr_testify//require", ], "@io_bazel_rules_go//go/platform:ios_arm64": [ @@ -112,169 +101,141 @@ go_test( "@io_bazel_rules_go//go/platform:js_wasm": [ ":grunning", "//pkg/testutils/skip", - "//pkg/util/syncutil", "@com_github_stretchr_testify//require", ], "@io_bazel_rules_go//go/platform:linux_386": [ ":grunning", "//pkg/testutils/skip", - "//pkg/util/syncutil", "@com_github_stretchr_testify//require", ], "@io_bazel_rules_go//go/platform:linux_amd64": [ ":grunning", "//pkg/testutils/skip", - "//pkg/util/syncutil", "@com_github_stretchr_testify//require", ], "@io_bazel_rules_go//go/platform:linux_arm": [ ":grunning", "//pkg/testutils/skip", - "//pkg/util/syncutil", "@com_github_stretchr_testify//require", ], "@io_bazel_rules_go//go/platform:linux_arm64": [ ":grunning", "//pkg/testutils/skip", - "//pkg/util/syncutil", "@com_github_stretchr_testify//require", ], "@io_bazel_rules_go//go/platform:linux_mips": [ ":grunning", "//pkg/testutils/skip", - "//pkg/util/syncutil", "@com_github_stretchr_testify//require", ], "@io_bazel_rules_go//go/platform:linux_mips64": [ ":grunning", "//pkg/testutils/skip", - "//pkg/util/syncutil", "@com_github_stretchr_testify//require", ], "@io_bazel_rules_go//go/platform:linux_mips64le": [ ":grunning", "//pkg/testutils/skip", - "//pkg/util/syncutil", "@com_github_stretchr_testify//require", ], "@io_bazel_rules_go//go/platform:linux_mipsle": [ ":grunning", "//pkg/testutils/skip", - "//pkg/util/syncutil", "@com_github_stretchr_testify//require", ], "@io_bazel_rules_go//go/platform:linux_ppc64": [ ":grunning", "//pkg/testutils/skip", - "//pkg/util/syncutil", "@com_github_stretchr_testify//require", ], "@io_bazel_rules_go//go/platform:linux_ppc64le": [ ":grunning", "//pkg/testutils/skip", - "//pkg/util/syncutil", "@com_github_stretchr_testify//require", ], "@io_bazel_rules_go//go/platform:linux_riscv64": [ ":grunning", "//pkg/testutils/skip", - "//pkg/util/syncutil", "@com_github_stretchr_testify//require", ], "@io_bazel_rules_go//go/platform:linux_s390x": [ ":grunning", "//pkg/testutils/skip", - "//pkg/util/syncutil", "@com_github_stretchr_testify//require", ], "@io_bazel_rules_go//go/platform:netbsd_386": [ ":grunning", "//pkg/testutils/skip", - "//pkg/util/syncutil", "@com_github_stretchr_testify//require", ], "@io_bazel_rules_go//go/platform:netbsd_amd64": [ ":grunning", "//pkg/testutils/skip", - "//pkg/util/syncutil", "@com_github_stretchr_testify//require", ], "@io_bazel_rules_go//go/platform:netbsd_arm": [ ":grunning", "//pkg/testutils/skip", - "//pkg/util/syncutil", "@com_github_stretchr_testify//require", ], "@io_bazel_rules_go//go/platform:netbsd_arm64": [ ":grunning", "//pkg/testutils/skip", - "//pkg/util/syncutil", "@com_github_stretchr_testify//require", ], "@io_bazel_rules_go//go/platform:openbsd_386": [ ":grunning", "//pkg/testutils/skip", - "//pkg/util/syncutil", "@com_github_stretchr_testify//require", ], "@io_bazel_rules_go//go/platform:openbsd_amd64": [ ":grunning", "//pkg/testutils/skip", - "//pkg/util/syncutil", "@com_github_stretchr_testify//require", ], "@io_bazel_rules_go//go/platform:openbsd_arm": [ ":grunning", "//pkg/testutils/skip", - "//pkg/util/syncutil", "@com_github_stretchr_testify//require", ], "@io_bazel_rules_go//go/platform:openbsd_arm64": [ ":grunning", "//pkg/testutils/skip", - "//pkg/util/syncutil", "@com_github_stretchr_testify//require", ], "@io_bazel_rules_go//go/platform:plan9_386": [ ":grunning", "//pkg/testutils/skip", - "//pkg/util/syncutil", "@com_github_stretchr_testify//require", ], "@io_bazel_rules_go//go/platform:plan9_amd64": [ ":grunning", "//pkg/testutils/skip", - "//pkg/util/syncutil", "@com_github_stretchr_testify//require", ], "@io_bazel_rules_go//go/platform:plan9_arm": [ ":grunning", "//pkg/testutils/skip", - "//pkg/util/syncutil", "@com_github_stretchr_testify//require", ], "@io_bazel_rules_go//go/platform:solaris_amd64": [ ":grunning", "//pkg/testutils/skip", - "//pkg/util/syncutil", "@com_github_stretchr_testify//require", ], "@io_bazel_rules_go//go/platform:windows_386": [ ":grunning", "//pkg/testutils/skip", - "//pkg/util/syncutil", "@com_github_stretchr_testify//require", ], "@io_bazel_rules_go//go/platform:windows_amd64": [ ":grunning", "//pkg/testutils/skip", - "//pkg/util/syncutil", "@com_github_stretchr_testify//require", ], "@io_bazel_rules_go//go/platform:windows_arm": [ ":grunning", "//pkg/testutils/skip", - "//pkg/util/syncutil", "@com_github_stretchr_testify//require", ], "//conditions:default": [], diff --git a/pkg/util/grunning/enabled_test.go b/pkg/util/grunning/enabled_test.go index 068a86c71e95..649a6f202236 100644 --- a/pkg/util/grunning/enabled_test.go +++ b/pkg/util/grunning/enabled_test.go @@ -20,12 +20,10 @@ package grunning_test import ( "runtime" "sync" - "sync/atomic" "testing" "github.com/cockroachdb/cockroach/pkg/testutils/skip" "github.com/cockroachdb/cockroach/pkg/util/grunning" - "github.com/cockroachdb/cockroach/pkg/util/syncutil" "github.com/stretchr/testify/require" ) @@ -46,13 +44,7 @@ func TestEnabled(t *testing.T) { func TestEquivalentGoroutines(t *testing.T) { skip.UnderStress(t, "not applicable") - mu := struct { - syncutil.Mutex - nanos map[int]int64 - }{} - mu.nanos = make(map[int]int64) - - f := func(wg *sync.WaitGroup, id int) { + f := func(wg *sync.WaitGroup, result *int64) { defer wg.Done() var sum int @@ -64,31 +56,27 @@ func TestEquivalentGoroutines(t *testing.T) { } nanos := grunning.Time().Nanoseconds() - mu.Lock() - mu.nanos[id] = nanos - mu.Unlock() + *result = nanos } const threads = 10 var wg sync.WaitGroup + results := make([]int64, threads) for i := 0; i < threads; i++ { i := i // copy loop variable wg.Add(1) - go f(&wg, i) + go f(&wg, &results[i]) } wg.Wait() - mu.Lock() - defer mu.Unlock() - total := int64(0) - for _, nanos := range mu.nanos { - total += nanos + for _, result := range results { + total += result } exp := 1.0 / threads - for i, nanos := range mu.nanos { - got := float64(nanos) / float64(total) + for i, result := range results { + got := float64(result) / float64(total) t.Logf("thread=%02d expected≈%5.2f%% got=%5.2f%% of on-cpu time", i+1, exp*100, got*100) @@ -112,7 +100,7 @@ func TestProportionalGoroutines(t *testing.T) { } nanos := grunning.Time().Nanoseconds() - atomic.AddInt64(result, nanos) + *result = nanos } results := make([]int64, 10) @@ -144,13 +132,17 @@ func TestProportionalGoroutines(t *testing.T) { } // TestPingPongHog is adapted from a benchmark in the Go runtime, forcing the -// scheduler to continually schedule goroutines. +// scheduler to continually schedule goroutines. It demonstrates that if two +// goroutines alternately cycle between running and waiting, they will get +// similar running times. func TestPingPongHog(t *testing.T) { skip.UnderStress(t, "not applicable") defer runtime.GOMAXPROCS(runtime.GOMAXPROCS(1)) - // Create a CPU hog. + // Create a CPU hog. It makes the two goroutines that want to cycle between + // running and waiting also have to wait in runnable state, until the CPU + // hog is finished with its time slice. stop, done := make(chan bool), make(chan bool) go func() { for { @@ -172,7 +164,7 @@ func TestPingPongHog(t *testing.T) { pong <- <-ping } pingern = grunning.Time().Nanoseconds() - close(stop) + close(stop) // stop the CPU hog done <- true }() go func() { @@ -183,9 +175,9 @@ func TestPingPongHog(t *testing.T) { done <- true }() ping <- true // start ping-pong - <-stop - <-ping // let last ponger exit - <-done // make sure goroutines exit + <-stop // wait until the pinger tells the CPU hog to stop + <-ping // wait for the ponger to finish + <-done // make sure goroutines exit <-done <-done @@ -194,7 +186,13 @@ func TestPingPongHog(t *testing.T) { } // BenchmarkGRunningTime measures how costly it is to read the current -// goroutine's running time. +// goroutine's running time. Results: +// +// goos: linux +// goarch: amd64 +// cpu: Intel(R) Xeon(R) CPU @ 2.20GHz +// BenchmarkGRunningTime +// BenchmarkGRunningTime-24 38336452 31.59 ns/op func BenchmarkGRunningTime(b *testing.B) { for n := 0; n < b.N; n++ { _ = grunning.Time() From 47d0339b4c842d38b57ddfe82a5e1f1db674ad87 Mon Sep 17 00:00:00 2001 From: Yahor Yuzefovich Date: Tue, 16 Aug 2022 09:01:09 -0700 Subject: [PATCH 8/8] sqlsmith: skip crdb_internal.set_compaction_concurrency Release justification: test-only change. Release note: None --- pkg/internal/sqlsmith/schema.go | 1 + 1 file changed, 1 insertion(+) diff --git a/pkg/internal/sqlsmith/schema.go b/pkg/internal/sqlsmith/schema.go index a8e7bdd07b49..4c5e91015978 100644 --- a/pkg/internal/sqlsmith/schema.go +++ b/pkg/internal/sqlsmith/schema.go @@ -517,6 +517,7 @@ var functions = func() map[tree.FunctionClass]map[oid.Oid][]function { "crdb_internal.complete_replication_stream", "crdb_internal.revalidate_unique_constraint", "crdb_internal.request_statement_bundle", + "crdb_internal.set_compaction_concurrency", } { skip = skip || strings.Contains(def.Name, substr) }