Skip to content

Commit

Permalink
sql: session variable to allow multiple modification subqueries of table
Browse files Browse the repository at this point in the history
Add a new session variable, enable_multiple_modifications_of_table,
which can be used instead of sql.multiple_modifications_of_table.enabled
to allow execution of statements with multiple modification subqueries
of the same table.

Instead of making the original cluster setting the GlobalDefault of this
new session setting, the original cluster setting is kept in the
optbuilder logic. This is to avoid breaking applications that are
already toggling the cluster setting mid-session to allow statements.

Fixes: #76261

Release note (sql change): Add a new session variable,
enable_multiple_modifications_of_table, which can be used instead of
cluster variable sql.multiple_modifications_of_table.enabled to allow
statements containing multiple INSERT ON CONFLICT, UPSERT, UPDATE, or
DELETE subqueries modifying the same table. Note that underlying issue
70731 is not fixed. As with sql.multiple_modifications_of_table.enabled,
be warned that with this session variable enabled there is nothing to
prevent the table corruption seen in issue 70731 from occuring if the
same row is modified multiple times by different subqueries of a single
statment. It's best to rewrite these statements, but the session
variable is provided as an aid if this is not possible.

Release justification: low risk, high benefit change to existing
functionality.
  • Loading branch information
michae2 committed May 4, 2022
1 parent 31c42eb commit f03888a
Show file tree
Hide file tree
Showing 8 changed files with 60 additions and 1 deletion.
4 changes: 4 additions & 0 deletions pkg/sql/exec_util.go
Original file line number Diff line number Diff line change
Expand Up @@ -3205,6 +3205,10 @@ func (m *sessionDataMutator) SetExpectAndIgnoreNotVisibleColumnsInCopy(val bool)
m.data.ExpectAndIgnoreNotVisibleColumnsInCopy = val
}

func (m *sessionDataMutator) SetMultipleModificationsOfTable(val bool) {
m.data.MultipleModificationsOfTable = val
}

// Utility functions related to scrubbing sensitive information on SQL Stats.

// quantizeCounts ensures that the Count field in the
Expand Down
1 change: 1 addition & 0 deletions pkg/sql/logictest/testdata/logic_test/information_schema
Original file line number Diff line number Diff line change
Expand Up @@ -4719,6 +4719,7 @@ enable_experimental_stream_replication off
enable_implicit_select_for_update on
enable_implicit_transaction_for_batch_statements off
enable_insert_fast_path on
enable_multiple_modifications_of_table off
enable_multiregion_placement_policy off
enable_seqscan on
enable_super_regions off
Expand Down
3 changes: 3 additions & 0 deletions pkg/sql/logictest/testdata/logic_test/pg_catalog
Original file line number Diff line number Diff line change
Expand Up @@ -4100,6 +4100,7 @@ enable_experimental_stream_replication off NULL
enable_implicit_select_for_update on NULL NULL NULL string
enable_implicit_transaction_for_batch_statements off NULL NULL NULL string
enable_insert_fast_path on NULL NULL NULL string
enable_multiple_modifications_of_table off NULL NULL NULL string
enable_multiregion_placement_policy off NULL NULL NULL string
enable_seqscan on NULL NULL NULL string
enable_super_regions off NULL NULL NULL string
Expand Down Expand Up @@ -4220,6 +4221,7 @@ enable_experimental_stream_replication off NULL
enable_implicit_select_for_update on NULL user NULL on on
enable_implicit_transaction_for_batch_statements off NULL user NULL off off
enable_insert_fast_path on NULL user NULL on on
enable_multiple_modifications_of_table off NULL user NULL off off
enable_multiregion_placement_policy off NULL user NULL off off
enable_seqscan on NULL user NULL on on
enable_super_regions off NULL user NULL off off
Expand Down Expand Up @@ -4335,6 +4337,7 @@ enable_experimental_stream_replication NULL NULL NULL
enable_implicit_select_for_update NULL NULL NULL NULL NULL
enable_implicit_transaction_for_batch_statements NULL NULL NULL NULL NULL
enable_insert_fast_path NULL NULL NULL NULL NULL
enable_multiple_modifications_of_table NULL NULL NULL NULL NULL
enable_multiregion_placement_policy NULL NULL NULL NULL NULL
enable_seqscan NULL NULL NULL NULL NULL
enable_super_regions NULL NULL NULL NULL NULL
Expand Down
1 change: 1 addition & 0 deletions pkg/sql/logictest/testdata/logic_test/show_source
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ enable_experimental_stream_replication off
enable_implicit_select_for_update on
enable_implicit_transaction_for_batch_statements off
enable_insert_fast_path on
enable_multiple_modifications_of_table off
enable_multiregion_placement_policy off
enable_seqscan on
enable_super_regions off
Expand Down
25 changes: 25 additions & 0 deletions pkg/sql/logictest/testdata/logic_test/with
Original file line number Diff line number Diff line change
Expand Up @@ -997,6 +997,31 @@ EXPERIMENTAL SCRUB TABLE t WITH OPTIONS INDEX ALL
statement ok
RESET CLUSTER SETTING sql.multiple_modifications_of_table.enabled

# Multiple mutations can also be explicitly allowed with a session setting.
statement ok
SET enable_multiple_modifications_of_table = true

# Multiple updates of different rows in the same table.
query II rowsort
WITH
u1 AS (UPDATE t SET j = j - 40 WHERE i < 20 RETURNING *),
u2 AS (UPDATE t SET j = j + 40 WHERE i >= 20 RETURNING *)
TABLE u1 UNION ALL TABLE u2
----
2 60
4 60
6 60
8 60
20 440

# Check for corruption.
query TTTTTTTT
EXPERIMENTAL SCRUB TABLE t WITH OPTIONS INDEX ALL
----

statement ok
RESET enable_multiple_modifications_of_table

# Tests with multiple mutations on the same table, modifying the same
# rows. These testcases vary the type of mutation and the form. All should fail
# with an error. When issue 70731 is fixed, some might no longer fail.
Expand Down
5 changes: 4 additions & 1 deletion pkg/sql/opt/optbuilder/util.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import (
"github.com/cockroachdb/errors"
)

// TODO(michae2): Remove this when #70731 is fixed.
var multipleModificationsOfTableEnabled = settings.RegisterBoolSetting(
settings.TenantWritable,
"sql.multiple_modifications_of_table.enabled",
Expand Down Expand Up @@ -493,7 +494,9 @@ func (b *Builder) checkMultipleMutations(tab cat.Table, simpleInsert bool) {
}
allSimpleInserts = allSimpleInserts && simpleInsert
b.areAllTableMutationsSimpleInserts[tab.ID()] = allSimpleInserts
if !allSimpleInserts && !multipleModificationsOfTableEnabled.Get(&b.evalCtx.Settings.SV) {
if !allSimpleInserts &&
!multipleModificationsOfTableEnabled.Get(&b.evalCtx.Settings.SV) &&
!b.evalCtx.SessionData().MultipleModificationsOfTable {
panic(pgerror.Newf(
pgcode.FeatureNotSupported,
"multiple modification subqueries of the same table %q are not supported unless "+
Expand Down
4 changes: 4 additions & 0 deletions pkg/sql/sessiondatapb/local_only_session_data.proto
Original file line number Diff line number Diff line change
Expand Up @@ -246,6 +246,10 @@ message LocalOnlySessionData {
// (with no column name specifiers) to expect and ignore not visible column
// fields.
bool expect_and_ignore_not_visible_columns_in_copy = 67;
// MultipleModificationsOfTable allows statements containing multiple INSERT
// ON CONFLICT, UPSERT, UPDATE, or DELETE subqueries modifying the same table,
// at the risk of data corruption if the same row is modified multiple times.
bool multiple_modifications_of_table = 68;

///////////////////////////////////////////////////////////////////////////
// WARNING: consider whether a session parameter you're adding needs to //
Expand Down
18 changes: 18 additions & 0 deletions pkg/sql/vars.go
Original file line number Diff line number Diff line change
Expand Up @@ -2049,6 +2049,24 @@ var varGen = map[string]sessionVar{
},
GlobalDefault: globalFalse,
},

// TODO(michae2): Remove this when #70731 is fixed.
// CockroachDB extension.
`enable_multiple_modifications_of_table`: {
GetStringVal: makePostgresBoolGetStringValFn(`enable_multiple_modifications_of_table`),
Set: func(_ context.Context, m sessionDataMutator, s string) error {
b, err := paramparse.ParseBoolVar("enable_multiple_modifications_of_table", s)
if err != nil {
return err
}
m.SetMultipleModificationsOfTable(b)
return nil
},
Get: func(evalCtx *extendedEvalContext) (string, error) {
return formatBoolAsPostgresSetting(evalCtx.SessionData().MultipleModificationsOfTable), nil
},
GlobalDefault: globalFalse,
},
}

const compatErrMsg = "this parameter is currently recognized only for compatibility and has no effect in CockroachDB."
Expand Down

0 comments on commit f03888a

Please sign in to comment.