From 9a6b91c6584f60a53de46e439d2d4e33e753fc11 Mon Sep 17 00:00:00 2001 From: Jane Xing Date: Wed, 29 Mar 2023 13:00:48 -0500 Subject: [PATCH] sql: replace cluster setting with session var for multiple active portals In 87776cbb1f1b11eb3debb4485bbf55d84a419448 we introduced the cluster setting `sql.pgwire.multiple_active_portal.enabled`. Now we'd like to replace it with the session variable `multiple_active_portals_enabled`. This is to limit usages of pausable portals and reduce the hazard of messing the whole clsuter. Release note (sql): replace cluster setting `sql.pgwire.multiple_active_portal.enabled` with session variable `multiple_active_portals_enabled` for the preview feature -- multiple active portals. --- .../settings/settings-for-tenants.txt | 1 - docs/generated/settings/settings.html | 1 - pkg/server/BUILD.bazel | 1 - pkg/server/server_sql.go | 8 ------- pkg/sql/distsql_running.go | 6 +++--- pkg/sql/exec_util.go | 12 ++++------- pkg/sql/execinfra/base.go | 5 +++-- .../testdata/logic_test/information_schema | 1 + .../logictest/testdata/logic_test/pg_catalog | 3 +++ .../logictest/testdata/logic_test/show_source | 1 + .../testdata/pgtest/multiple_active_portals | 4 ++-- pkg/sql/pgwire/testdata/pgtest/portals_crbugs | 4 ++-- pkg/sql/prepared_stmt.go | 21 +++++++++++-------- .../local_only_session_data.proto | 6 ++++++ pkg/sql/sqltelemetry/pgwire.go | 8 ++++--- pkg/sql/vars.go | 17 +++++++++++++++ 16 files changed, 59 insertions(+), 40 deletions(-) diff --git a/docs/generated/settings/settings-for-tenants.txt b/docs/generated/settings/settings-for-tenants.txt index a40331d84d4b..801a786d8737 100644 --- a/docs/generated/settings/settings-for-tenants.txt +++ b/docs/generated/settings/settings-for-tenants.txt @@ -249,7 +249,6 @@ sql.multiple_modifications_of_table.enabled boolean false if true, allow stateme sql.multiregion.drop_primary_region.enabled boolean true allows dropping the PRIMARY REGION of a database if it is the last region sql.notices.enabled boolean true enable notices in the server/client protocol being sent sql.optimizer.uniqueness_checks_for_gen_random_uuid.enabled boolean false if enabled, uniqueness checks may be planned for mutations of UUID columns updated with gen_random_uuid(); otherwise, uniqueness is assumed due to near-zero collision probability -sql.pgwire.multiple_active_portals.enabled boolean false if true, portals with read-only SELECT query without sub/post queries can be executed in interleaving manner, but with local execution plan sql.schema.telemetry.recurrence string @weekly cron-tab recurrence for SQL schema telemetry job sql.show_ranges_deprecated_behavior.enabled boolean true if set, SHOW RANGES and crdb_internal.ranges{_no_leases} behave with deprecated pre-v23.1 semantics. NB: the new SHOW RANGES interface has richer WITH options than pre-v23.1 SHOW RANGES. sql.spatial.experimental_box2d_comparison_operators.enabled boolean false enables the use of certain experimental box2d comparison operators diff --git a/docs/generated/settings/settings.html b/docs/generated/settings/settings.html index 01de5764264d..46ae1d53fe6a 100644 --- a/docs/generated/settings/settings.html +++ b/docs/generated/settings/settings.html @@ -200,7 +200,6 @@
sql.multiregion.drop_primary_region.enabled
booleantrueallows dropping the PRIMARY REGION of a database if it is the last region
sql.notices.enabled
booleantrueenable notices in the server/client protocol being sent
sql.optimizer.uniqueness_checks_for_gen_random_uuid.enabled
booleanfalseif enabled, uniqueness checks may be planned for mutations of UUID columns updated with gen_random_uuid(); otherwise, uniqueness is assumed due to near-zero collision probability -
sql.pgwire.multiple_active_portals.enabled
booleanfalseif true, portals with read-only SELECT query without sub/post queries can be executed in interleaving manner, but with local execution plan
sql.schema.telemetry.recurrence
string@weeklycron-tab recurrence for SQL schema telemetry job
sql.show_ranges_deprecated_behavior.enabled
booleantrueif set, SHOW RANGES and crdb_internal.ranges{_no_leases} behave with deprecated pre-v23.1 semantics. NB: the new SHOW RANGES interface has richer WITH options than pre-v23.1 SHOW RANGES.
sql.spatial.experimental_box2d_comparison_operators.enabled
booleanfalseenables the use of certain experimental box2d comparison operators diff --git a/pkg/server/BUILD.bazel b/pkg/server/BUILD.bazel index a425cbc8aa79..fbeb780df28e 100644 --- a/pkg/server/BUILD.bazel +++ b/pkg/server/BUILD.bazel @@ -250,7 +250,6 @@ go_library( "//pkg/sql/sqlstats/insights", "//pkg/sql/sqlstats/persistedsqlstats", "//pkg/sql/sqlstats/persistedsqlstats/sqlstatsutil", - "//pkg/sql/sqltelemetry", "//pkg/sql/stats", "//pkg/sql/stmtdiagnostics", "//pkg/sql/syntheticprivilege", diff --git a/pkg/server/server_sql.go b/pkg/server/server_sql.go index f950d7de9f61..717c1c00f271 100644 --- a/pkg/server/server_sql.go +++ b/pkg/server/server_sql.go @@ -61,7 +61,6 @@ import ( "github.com/cockroachdb/cockroach/pkg/server/settingswatcher" "github.com/cockroachdb/cockroach/pkg/server/status" "github.com/cockroachdb/cockroach/pkg/server/systemconfigwatcher" - "github.com/cockroachdb/cockroach/pkg/server/telemetry" "github.com/cockroachdb/cockroach/pkg/server/tracedumper" "github.com/cockroachdb/cockroach/pkg/settings" "github.com/cockroachdb/cockroach/pkg/settings/cluster" @@ -104,7 +103,6 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/sqlliveness/slinstance" "github.com/cockroachdb/cockroach/pkg/sql/sqlliveness/slprovider" "github.com/cockroachdb/cockroach/pkg/sql/sqlstats" - "github.com/cockroachdb/cockroach/pkg/sql/sqltelemetry" "github.com/cockroachdb/cockroach/pkg/sql/stats" "github.com/cockroachdb/cockroach/pkg/sql/stmtdiagnostics" "github.com/cockroachdb/cockroach/pkg/sql/syntheticprivilegecache" @@ -1344,12 +1342,6 @@ func newSQLServer(ctx context.Context, cfg sqlServerArgs) (*SQLServer, error) { vmoduleSetting.SetOnChange(&cfg.Settings.SV, fn) fn(ctx) - sql.EnableMultipleActivePortals.SetOnChange(&cfg.Settings.SV, func(ctx context.Context) { - if sql.EnableMultipleActivePortals.Get(&cfg.Settings.SV) { - telemetry.Inc(sqltelemetry.MultipleActivePortalCounter) - } - }) - return &SQLServer{ ambientCtx: cfg.BaseConfig.AmbientCtx, stopper: cfg.stopper, diff --git a/pkg/sql/distsql_running.go b/pkg/sql/distsql_running.go index f51ea59747c6..1b119e768653 100644 --- a/pkg/sql/distsql_running.go +++ b/pkg/sql/distsql_running.go @@ -1544,16 +1544,16 @@ func (r *DistSQLReceiver) PushBatch( var ( // ErrLimitedResultNotSupported is an error produced by pgwire // indicating the user attempted to have multiple active portals but - // either without setting sql.pgwire.multiple_active_portals.enabled to + // either without setting session variable multiple_active_portals_enabled to // true or the underlying query does not satisfy the restriction. ErrLimitedResultNotSupported = unimplemented.NewWithIssue( 40195, "multiple active portals not supported, "+ - "please set sql.pgwire.multiple_active_portals.enabled to true. "+ + "please set session variable multiple_active_portals_enabled to true. "+ "Note: this feature is in preview", ) // ErrStmtNotSupportedForPausablePortal is returned when the user have set - // sql.pgwire.multiple_active_portals.enabled to true but set an unsupported + // session variable multiple_active_portals_enabled to true but set an unsupported // statement for a portal. ErrStmtNotSupportedForPausablePortal = unimplemented.NewWithIssue( 98911, diff --git a/pkg/sql/exec_util.go b/pkg/sql/exec_util.go index 208a9042e8c3..df1ea9c0e298 100644 --- a/pkg/sql/exec_util.go +++ b/pkg/sql/exec_util.go @@ -733,14 +733,6 @@ var overrideAlterPrimaryRegionInSuperRegion = settings.RegisterBoolSetting( false, ).WithPublic() -var EnableMultipleActivePortals = settings.RegisterBoolSetting( - settings.TenantWritable, - "sql.pgwire.multiple_active_portals.enabled", - "if true, portals with read-only SELECT query without sub/post queries "+ - "can be executed in interleaving manner, but with local execution plan", - false, -).WithPublic() - var errNoTransactionInProgress = errors.New("there is no transaction in progress") var errTransactionInProgress = errors.New("there is already a transaction in progress") @@ -3512,6 +3504,10 @@ func (m *sessionDataMutator) SetStreamerEnabled(val bool) { m.data.StreamerEnabled = val } +func (m *sessionDataMutator) SetMultipleActivePortalsEnabled(val bool) { + m.data.MultipleActivePortalsEnabled = val +} + // Utility functions related to scrubbing sensitive information on SQL Stats. // quantizeCounts ensures that the Count field in the diff --git a/pkg/sql/execinfra/base.go b/pkg/sql/execinfra/base.go index 496e6bce51fa..737302352526 100644 --- a/pkg/sql/execinfra/base.go +++ b/pkg/sql/execinfra/base.go @@ -43,8 +43,9 @@ const ( NeedMoreRows ConsumerStatus = iota // SwitchToAnotherPortal indicates that the we received exec command for // a different portal, and may come back to continue executing the current - // portal later. If the cluster setting sql.pgwire.multiple_active_portals.enabled - // is set to be true, we do nothing and return the control to the connExecutor. + // portal later. If the cluster setting session variable + // multiple_active_portals_enabled is set to be true, we do nothing and return + // the control to the connExecutor. SwitchToAnotherPortal // DrainRequested indicates that the consumer will not process any more data // rows, but will accept trailing metadata from the producer. diff --git a/pkg/sql/logictest/testdata/logic_test/information_schema b/pkg/sql/logictest/testdata/logic_test/information_schema index f4424e891664..771a5640ab0b 100644 --- a/pkg/sql/logictest/testdata/logic_test/information_schema +++ b/pkg/sql/logictest/testdata/logic_test/information_schema @@ -5261,6 +5261,7 @@ lock_timeout 0 log_timezone UTC max_identifier_length 128 max_index_keys 32 +multiple_active_portals_enabled off node_id 1 null_ordered_last off on_update_rehome_row_enabled on diff --git a/pkg/sql/logictest/testdata/logic_test/pg_catalog b/pkg/sql/logictest/testdata/logic_test/pg_catalog index b5f7840e7ea4..82d8f0db8fed 100644 --- a/pkg/sql/logictest/testdata/logic_test/pg_catalog +++ b/pkg/sql/logictest/testdata/logic_test/pg_catalog @@ -2756,6 +2756,7 @@ lock_timeout 0 NULL log_timezone UTC NULL NULL NULL string max_identifier_length 128 NULL NULL NULL string max_index_keys 32 NULL NULL NULL string +multiple_active_portals_enabled off NULL NULL NULL string node_id 1 NULL NULL NULL string null_ordered_last off NULL NULL NULL string on_update_rehome_row_enabled on NULL NULL NULL string @@ -2909,6 +2910,7 @@ lock_timeout 0 NULL log_timezone UTC NULL user NULL UTC UTC max_identifier_length 128 NULL user NULL 128 128 max_index_keys 32 NULL user NULL 32 32 +multiple_active_portals_enabled off NULL user NULL off off node_id 1 NULL user NULL 1 1 null_ordered_last off NULL user NULL off off on_update_rehome_row_enabled on NULL user NULL on on @@ -3060,6 +3062,7 @@ lock_timeout NULL NULL NULL log_timezone NULL NULL NULL NULL NULL max_identifier_length NULL NULL NULL NULL NULL max_index_keys NULL NULL NULL NULL NULL +multiple_active_portals_enabled NULL NULL NULL NULL NULL node_id NULL NULL NULL NULL NULL null_ordered_last NULL NULL NULL NULL NULL on_update_rehome_row_enabled NULL NULL NULL NULL NULL diff --git a/pkg/sql/logictest/testdata/logic_test/show_source b/pkg/sql/logictest/testdata/logic_test/show_source index fdbfc92f2c39..0328b8f7dcd8 100644 --- a/pkg/sql/logictest/testdata/logic_test/show_source +++ b/pkg/sql/logictest/testdata/logic_test/show_source @@ -108,6 +108,7 @@ lock_timeout 0 log_timezone UTC max_identifier_length 128 max_index_keys 32 +multiple_active_portals_enabled off node_id 1 null_ordered_last off on_update_rehome_row_enabled on diff --git a/pkg/sql/pgwire/testdata/pgtest/multiple_active_portals b/pkg/sql/pgwire/testdata/pgtest/multiple_active_portals index 96fc33e5a3bc..167627a40437 100644 --- a/pkg/sql/pgwire/testdata/pgtest/multiple_active_portals +++ b/pkg/sql/pgwire/testdata/pgtest/multiple_active_portals @@ -1,11 +1,11 @@ send crdb_only -Query {"String": "SET CLUSTER SETTING sql.pgwire.multiple_active_portals.enabled = true"} +Query {"String": "SET multiple_active_portals_enabled = true"} ---- until crdb_only ignore=NoticeResponse ReadyForQuery ---- -{"Type":"CommandComplete","CommandTag":"SET CLUSTER SETTING"} +{"Type":"CommandComplete","CommandTag":"SET"} {"Type":"ReadyForQuery","TxStatus":"I"} subtest select_from_individual_resources diff --git a/pkg/sql/pgwire/testdata/pgtest/portals_crbugs b/pkg/sql/pgwire/testdata/pgtest/portals_crbugs index 1afeb7f1788f..14211a72b413 100644 --- a/pkg/sql/pgwire/testdata/pgtest/portals_crbugs +++ b/pkg/sql/pgwire/testdata/pgtest/portals_crbugs @@ -29,7 +29,7 @@ ReadyForQuery {"Type":"BindComplete"} {"Type":"DataRow","Values":[{"text":"1"}]} {"Type":"PortalSuspended"} -{"Type":"ErrorResponse","Code":"0A000","Message":"unimplemented: multiple active portals not supported, please set sql.pgwire.multiple_active_portals.enabled to true. Note: this feature is in preview"} +{"Type":"ErrorResponse","Code":"0A000","Message":"unimplemented: multiple active portals not supported, please set session variable multiple_active_portals_enabled to true. Note: this feature is in preview"} {"Type":"ReadyForQuery","TxStatus":"E"} {"Type":"ReadyForQuery","TxStatus":"E"} @@ -70,7 +70,7 @@ ReadyForQuery {"Type":"BindComplete"} {"Type":"DataRow","Values":[{"text":"1"}]} {"Type":"PortalSuspended"} -{"Type":"ErrorResponse","Code":"0A000","Message":"unimplemented: multiple active portals not supported, please set sql.pgwire.multiple_active_portals.enabled to true. Note: this feature is in preview"} +{"Type":"ErrorResponse","Code":"0A000","Message":"unimplemented: multiple active portals not supported, please set session variable multiple_active_portals_enabled to true. Note: this feature is in preview"} {"Type":"ReadyForQuery","TxStatus":"E"} send diff --git a/pkg/sql/prepared_stmt.go b/pkg/sql/prepared_stmt.go index ec0bed295ea1..8a28942fafa1 100644 --- a/pkg/sql/prepared_stmt.go +++ b/pkg/sql/prepared_stmt.go @@ -15,12 +15,14 @@ import ( "time" "unsafe" + "github.com/cockroachdb/cockroach/pkg/server/telemetry" "github.com/cockroachdb/cockroach/pkg/sql/clusterunique" "github.com/cockroachdb/cockroach/pkg/sql/flowinfra" "github.com/cockroachdb/cockroach/pkg/sql/opt/memo" "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgwirebase" "github.com/cockroachdb/cockroach/pkg/sql/querycache" "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" + "github.com/cockroachdb/cockroach/pkg/sql/sqltelemetry" "github.com/cockroachdb/cockroach/pkg/sql/types" "github.com/cockroachdb/cockroach/pkg/util/fsm" "github.com/cockroachdb/cockroach/pkg/util/log" @@ -130,15 +132,15 @@ type PortalPausablity int64 const ( // PortalPausabilityDisabled is the default status of a portal when - // sql.pgwire.multiple_active_portals.enabled is false. + // the session variable multiple_active_portals_enabled is false. PortalPausabilityDisabled PortalPausablity = iota - // PausablePortal is set when sql.pgwire.multiple_active_portals.enabled is - // set to true and the underlying statement is a read-only SELECT query with - // no sub-queries or post-queries. + // PausablePortal is set when the session variable multiple_active_portals_enabled + // is set to true and the underlying statement is a read-only SELECT query + // with no sub-queries or post-queries. PausablePortal // NotPausablePortalForUnsupportedStmt is used when the cluster setting - // sql.pgwire.multiple_active_portals.enabled is set to true, while we don't - // support underlying statement. + // the session variable multiple_active_portals_enabled is set to true, while + // we don't support underlying statement. NotPausablePortalForUnsupportedStmt ) @@ -186,13 +188,14 @@ func (ex *connExecutor) makePreparedPortal( // TODO(sql-session): address the compatibility of pausable portals and // prepared_statements_cache_size. // https://github.com/cockroachdb/cockroach/issues/99959 - if EnableMultipleActivePortals.Get(&ex.server.cfg.Settings.SV) && ex.executorType != executorTypeInternal { + if ex.sessionData().MultipleActivePortalsEnabled && ex.executorType != executorTypeInternal { + telemetry.Inc(sqltelemetry.StmtTriedWithPausablePortals) if tree.IsAllowedToPause(stmt.AST) { portal.pauseInfo = &portalPauseInfo{queryStats: &topLevelQueryStats{}} portal.portalPausablity = PausablePortal } else { - // We have set sql.defaults.multiple_active_portals.enabled to true, but - // we don't support the underlying query for a pausable portal. + // We have set the session variable multiple_active_portals_enabled to + // true, but we don't support the underlying query for a pausable portal. portal.portalPausablity = NotPausablePortalForUnsupportedStmt } } diff --git a/pkg/sql/sessiondatapb/local_only_session_data.proto b/pkg/sql/sessiondatapb/local_only_session_data.proto index 15e46dce7262..4d00b0800079 100644 --- a/pkg/sql/sessiondatapb/local_only_session_data.proto +++ b/pkg/sql/sessiondatapb/local_only_session_data.proto @@ -368,6 +368,12 @@ message LocalOnlySessionData { // StreamerEnabled controls whether the Streamer API can be used. bool streamer_enabled = 98; + // MultipleActivePortalEnabled determines if the pgwire portal execution + // for certain queries can be paused. If true, portals with read-only SELECT + // query without sub/post queries can be executed in interleaving manner, but + // with a local execution plan. + bool multiple_active_portals_enabled = 99; + /////////////////////////////////////////////////////////////////////////// // WARNING: consider whether a session parameter you're adding needs to // // be propagated to the remote nodes. If so, that parameter should live // diff --git a/pkg/sql/sqltelemetry/pgwire.go b/pkg/sql/sqltelemetry/pgwire.go index 18a7ef68e867..3289d5942d92 100644 --- a/pkg/sql/sqltelemetry/pgwire.go +++ b/pkg/sql/sqltelemetry/pgwire.go @@ -69,6 +69,8 @@ var CloseRequestCounter = telemetry.GetCounterOnce("pgwire.command.close") // is made. var FlushRequestCounter = telemetry.GetCounterOnce("pgwire.command.flush") -// MultipleActivePortalCounter is to be incremented every time the cluster setting -// sql.pgwire.multiple_active_portals.enabled is set true. -var MultipleActivePortalCounter = telemetry.GetCounterOnce("pgwire.multiple_active_portals") +// StmtTriedWithPausablePortals is to be incremented every time there's a +// not-internal statement executed with a pgwire portal and the session variable +// multiple_active_portals_enabled has been set to true. +// The statement might not satisfy the restriction for a pausable portal. +var StmtTriedWithPausablePortals = telemetry.GetCounterOnce("pgwire.pausable_portal_stmt") diff --git a/pkg/sql/vars.go b/pkg/sql/vars.go index 8c10a9a85ead..0a57da1d42f7 100644 --- a/pkg/sql/vars.go +++ b/pkg/sql/vars.go @@ -2621,6 +2621,23 @@ var varGen = map[string]sessionVar{ return formatBoolAsPostgresSetting(execinfra.UseStreamerEnabled.Get(sv)) }, }, + + // CockroachDB extension. + `multiple_active_portals_enabled`: { + GetStringVal: makePostgresBoolGetStringValFn(`multiple_active_portals_enabled`), + Set: func(_ context.Context, m sessionDataMutator, s string) error { + b, err := paramparse.ParseBoolVar("multiple_active_portals_enabled", s) + if err != nil { + return err + } + m.SetMultipleActivePortalsEnabled(b) + return nil + }, + Get: func(evalCtx *extendedEvalContext, _ *kv.Txn) (string, error) { + return formatBoolAsPostgresSetting(evalCtx.SessionData().MultipleActivePortalsEnabled), nil + }, + GlobalDefault: globalFalse, + }, } // We want test coverage for this on and off so make it metamorphic.