sql: new session variable strict_ddl_atomicity

Previously, CockroachDB always accepted DDL inside explicit
txns (BEGIN...COMMIT) and would issue some of these DDL statements as
asynchronous jobs after the rest of the txn was committed
successfully. If that DDL subsequently failed, a client would also get
error XXA00 to signal this error. However at that point the txn also
would be both partially committed and partially rolled back. This is
in effect an atomicity violation.

It was considered to be acceptable because in practice most clients
using DDL inside BEGIN...COMMIT do not, in fact, expect ACID semantics
in those cases. So it was considered OK to not deliver ACID semantics
in certain cases.

Except that we now have clients that do want us to "do the right
thing" and never allow a situation that _could_ yield atomicity
violations.

This patch makes a step in this direction by introducing a new session
variable `strict_ddl_atomicity`, which defaults to `false` for
compatibility with existing CockroachDB 19.2 clients and that of
previous CockroachDB versions.

When this setting is enabled / set to `true`, any DDL that would
be otherwise be processed asynchronously and risk triggering error
XXA00 is now fully disallowed, with the following error:

```
pq: unimplemented: cannot run this DDL statement inside BEGIN..COMMIT as its atomicity cannot be guaranteed
HINT: You have attempted to use a feature that is not yet implemented.
See: cockroachdb#42061
--
You can set the session variable 'strict_ddl_atomicity' to false if you are willing to accept atomicity violations.
```

Other DDL statements without async processing are still allowed
without triggering that error.

Examples of rejected DDL include but are not limited to: CREATE
INDEX, ALTER ADD CONSTRAINT, ADD COLUMN with DEFAULT, DROP INDEX.

Examples of DDL that's still processed: RENAME COLUMN, RENAME TABLE,
ALTER INDEX CONFIGURE ZONE.

Release note (sql change): a SQL client can now request strict
atomicity for mixed DDL/DML transactions with the new session variable
`strict_ddl_atomicity`, which defaults to `false`. When this variable
is set to `true`, CockroachDB will refuse to accept processing those
specific DDL statements inside BEGIN...COMMIT for which it cannot
guarantee atomic processing (other DDL statements are still allowed).

pkg/sql/exec_util.go

@@ -1804,6 +1804,10 @@ func (m *sessionDataMutator) SetSafeUpdates(val bool) {
 	m.data.SafeUpdates = val
 }
 
+func (m *sessionDataMutator) SetStrictDDLAtomicity(val bool) {
+	m.data.StrictDDLAtomicity = val
+}
+
 func (m *sessionDataMutator) SetSearchPath(val sessiondata.SearchPath) {
 	m.data.SearchPath = val
 }

pkg/sql/logictest/testdata/logic_test/pg_catalog

@@ -1567,6 +1567,7 @@ session_user                         root                NULL      NULL        N
 sql_safe_updates                     off                 NULL      NULL        NULL        string
 standard_conforming_strings          on                  NULL      NULL        NULL        string
 statement_timeout                    0                   NULL      NULL        NULL        string
+strict_ddl_atomicity                 off                 NULL      NULL        NULL        string
 synchronize_seqscans                 on                  NULL      NULL        NULL        string
 timezone                             UTC                 NULL      NULL        NULL        string
 tracing                              off                 NULL      NULL        NULL        string
@@ -1623,6 +1624,7 @@ session_user                         root                NULL  user     NULL
 sql_safe_updates                     off                 NULL  user     NULL      off                 off
 standard_conforming_strings          on                  NULL  user     NULL      on                  on
 statement_timeout                    0                   NULL  user     NULL      0                   0
+strict_ddl_atomicity                 off                 NULL  user     NULL      off                 off
 synchronize_seqscans                 on                  NULL  user     NULL      on                  on
 timezone                             UTC                 NULL  user     NULL      UTC                 UTC
 tracing                              off                 NULL  user     NULL      off                 off
@@ -1676,6 +1678,7 @@ session_user                         NULL    NULL     NULL     NULL        NULL
 sql_safe_updates                     NULL    NULL     NULL     NULL        NULL
 standard_conforming_strings          NULL    NULL     NULL     NULL        NULL
 statement_timeout                    NULL    NULL     NULL     NULL        NULL
+strict_ddl_atomicity                 NULL    NULL     NULL     NULL        NULL
 synchronize_seqscans                 NULL    NULL     NULL     NULL        NULL
 timezone                             NULL    NULL     NULL     NULL        NULL
 tracing                              NULL    NULL     NULL     NULL        NULL

pkg/sql/logictest/testdata/logic_test/show_source

@@ -63,6 +63,7 @@ session_user                         root
 sql_safe_updates                     off
 standard_conforming_strings          on
 statement_timeout                    0
+strict_ddl_atomicity                 off
 synchronize_seqscans                 on
 timezone                             UTC
 tracing                              off

pkg/sql/logictest/testdata/logic_test/strict_ddl_atomicity

@@ -26,3 +26,15 @@ SELECT * FROM testing
 4  b
 5  c
 
+statement ok
+DELETE FROM testing WHERE k = 5
+
+# Now try again with the strict setting
+statement ok
+SET strict_ddl_atomicity = true
+
+statement ok
+BEGIN
+
+statement error unimplemented: cannot run this DDL statement inside BEGIN..COMMIT as its atomicity cannot be guaranteed.*\n.*\n.*issues/42061
+ALTER TABLE testing ADD CONSTRAINT "unique_values" UNIQUE(v)

pkg/sql/sessiondata/session_data.go

@@ -53,6 +53,18 @@ type SessionData struct {
 	// SafeUpdates causes errors when the client
 	// sends syntax that may have unwanted side effects.
 	SafeUpdates bool
+	// StrictDDLAtomicity causes errors when the client attempts DDL
+	// operations inside an explicit txn and CockroachDB cannot
+	// guarantee the DDL to be performed atomically.
+	//
+	// When this is not set, a transaction may commit its DML
+	// statements but fail its DDL statements, resulting
+	// in error XXA00 - TransactionCommittedWithSchemaChangeFailure.
+	//
+	// When this is set, that particular atomicity violation should
+	// not occur any more (at the expense of disabling certain
+	// forms of DDL inside explicit txns).
+	StrictDDLAtomicity bool
 	// RemoteAddr is used to generate logging events.
 	RemoteAddr net.Addr
 	// ZigzagJoinEnabled indicates whether the optimizer should try and plan a

pkg/sql/table.go

@@ -22,6 +22,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/roachpb"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
 	"github.com/cockroachdb/cockroach/pkg/sql/sqlbase"
+	"github.com/cockroachdb/cockroach/pkg/util/errorutil/unimplemented"
 	"github.com/cockroachdb/cockroach/pkg/util/log"
 	"github.com/cockroachdb/errors"
 )
@@ -701,6 +702,13 @@ type tableCollectionModifier interface {
 func (p *planner) createOrUpdateSchemaChangeJob(
 	ctx context.Context, tableDesc *sqlbase.MutableTableDescriptor, stmt string,
 ) (sqlbase.MutationID, error) {
+	if !p.ExtendedEvalContext().TxnImplicit &&
+		p.SessionData().StrictDDLAtomicity {
+		return sqlbase.InvalidMutationID, unimplemented.NewWithIssueHint(42061,
+			"cannot run this DDL statement inside BEGIN..COMMIT as its atomicity cannot be guaranteed",
+			"You can set the session variable 'strict_ddl_atomicity' to false if you are willing to accept atomicity violations.")
+	}
+
 	mutationID := tableDesc.ClusterVersion.NextMutationID
 
 	// If the table being schema changed was created in the same txn, we do not

pkg/sql/vars.go

@@ -587,6 +587,23 @@ var varGen = map[string]sessionVar{
 		GlobalDefault: globalFalse,
 	},
 
+	// CockroachDB extension (inspired by MySQL).
+	`strict_ddl_atomicity`: {
+		Get: func(evalCtx *extendedEvalContext) string {
+			return formatBoolAsPostgresSetting(evalCtx.SessionData.StrictDDLAtomicity)
+		},
+		GetStringVal: makeBoolGetStringValFn("strict_ddl_atomicity"),
+		Set: func(_ context.Context, m *sessionDataMutator, s string) error {
+			b, err := parsePostgresBool(s)
+			if err != nil {
+				return err
+			}
+			m.SetStrictDDLAtomicity(b)
+			return nil
+		},
+		GlobalDefault: globalFalse,
+	},
+
 	// See https://www.postgresql.org/docs/10/static/ddl-schemas.html#DDL-SCHEMAS-PATH
 	// https://www.postgresql.org/docs/9.6/static/runtime-config-client.html
 	`search_path`: {