sql: replace cluster setting with session var for multiple active por…

…tals

In 87776cb 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.

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

docs/generated/settings/settings.html

@@ -200,7 +200,6 @@
 <tr><td><div id="setting-sql-multiregion-drop-primary-region-enabled" class="anchored"><code>sql.multiregion.drop_primary_region.enabled</code></div></td><td>boolean</td><td><code>true</code></td><td>allows dropping the PRIMARY REGION of a database if it is the last region</td></tr>
 <tr><td><div id="setting-sql-notices-enabled" class="anchored"><code>sql.notices.enabled</code></div></td><td>boolean</td><td><code>true</code></td><td>enable notices in the server/client protocol being sent</td></tr>
 <tr><td><div id="setting-sql-optimizer-uniqueness-checks-for-gen-random-uuid-enabled" class="anchored"><code>sql.optimizer.uniqueness_checks_for_gen_random_uuid.enabled</code></div></td><td>boolean</td><td><code>false</code></td><td>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</td></tr>
-<tr><td><div id="setting-sql-pgwire-multiple-active-portals-enabled" class="anchored"><code>sql.pgwire.multiple_active_portals.enabled</code></div></td><td>boolean</td><td><code>false</code></td><td>if true, portals with read-only SELECT query without sub/post queries can be executed in interleaving manner, but with local execution plan</td></tr>
 <tr><td><div id="setting-sql-schema-telemetry-recurrence" class="anchored"><code>sql.schema.telemetry.recurrence</code></div></td><td>string</td><td><code>@weekly</code></td><td>cron-tab recurrence for SQL schema telemetry job</td></tr>
 <tr><td><div id="setting-sql-show-ranges-deprecated-behavior-enabled" class="anchored"><code>sql.show_ranges_deprecated_behavior.enabled</code></div></td><td>boolean</td><td><code>true</code></td><td>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.</td></tr>
 <tr><td><div id="setting-sql-spatial-experimental-box2d-comparison-operators-enabled" class="anchored"><code>sql.spatial.experimental_box2d_comparison_operators.enabled</code></div></td><td>boolean</td><td><code>false</code></td><td>enables the use of certain experimental box2d comparison operators</td></tr>

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",

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,

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,

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

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.

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

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

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

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

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

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
 		}
 	}

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  //

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")

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.