Skip to content

Commit

Permalink
sql: disallow schema changes for READ COMMITTED transactions
Browse files Browse the repository at this point in the history
Due to how descriptor leasing works, schema changes are not safe in
weaker isolation transactions. Until they are safe, we disallow them.

Release note: None
  • Loading branch information
rafiss committed Jul 21, 2023
1 parent b4dfdc0 commit 6f535ca
Show file tree
Hide file tree
Showing 11 changed files with 156 additions and 58 deletions.
14 changes: 7 additions & 7 deletions pkg/ccl/logictestccl/tests/3node-tenant/generated_test.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
# LogicTest: !local-mixed-22.2-23.1

subtest select_for_update

# SELECT FOR UPDATE is prohibited under weaker isolation levels until we improve
# locking. See #57031, #75457, #100144.

Expand Down Expand Up @@ -88,22 +90,27 @@ SELECT aisle + 1 FROM s
statement ok
ROLLBACK

statement ok
SET SESSION CHARACTERISTICS AS TRANSACTION ISOLATION LEVEL READ COMMITTED

# Creating a UDF using SELECT FOR UPDATE should succeed under read committed.
statement ok
CREATE FUNCTION wrangle (name STRING) RETURNS INT LANGUAGE SQL AS $$
SELECT aisle FROM supermarket WHERE person = name FOR UPDATE
$$

statement ok
SET SESSION CHARACTERISTICS AS TRANSACTION ISOLATION LEVEL READ COMMITTED

# But calling that function should fail.
query error pgcode 0A000 cannot execute SELECT FOR UPDATE statements under ReadCommitted isolation
INSERT INTO supermarket (person, aisle) VALUES ('grandma', wrangle('matilda'))

statement ok
SET SESSION CHARACTERISTICS AS TRANSACTION ISOLATION LEVEL SERIALIZABLE

statement ok
DROP FUNCTION wrangle

statement ok
SET SESSION CHARACTERISTICS AS TRANSACTION ISOLATION LEVEL READ COMMITTED

# Preparing a SELECT FOR UPDATE should succeed under read committed.
statement ok
PREPARE psa AS SELECT aisle FROM supermarket WHERE person = $1::STRING FOR UPDATE
Expand Down Expand Up @@ -138,5 +145,92 @@ SELECT aisle
WHERE starts_with = 'm' AND ends_with = 'lda'
FOR UPDATE

subtest end

subtest schema_changes

# Schema changes are prohibited under weaker isolation levels.

statement error transaction involving a schema change needs to be SERIALIZABLE
ALTER TABLE supermarket ADD COLUMN age INT

statement error transaction involving a schema change needs to be SERIALIZABLE
CREATE TABLE foo(a INT)

statement error transaction involving a schema change needs to be SERIALIZABLE
DROP TABLE supermarket

statement error transaction involving a schema change needs to be SERIALIZABLE
CREATE USER foo

statement error transaction involving a schema change needs to be SERIALIZABLE
DROP USER testuser

statement error transaction involving a schema change needs to be SERIALIZABLE
GRANT admin TO testuser

statement error transaction involving a schema change needs to be SERIALIZABLE
GRANT SELECT ON supermarket TO testuser

statement error transaction involving a schema change needs to be SERIALIZABLE
GRANT USAGE ON SCHEMA public TO testuser

statement error transaction involving a schema change needs to be SERIALIZABLE
GRANT CONNECT ON DATABASE postgres TO testuser

statement error transaction involving a schema change needs to be SERIALIZABLE
CREATE INDEX foo ON supermarket(ends_with, starts_with)

statement error transaction involving a schema change needs to be SERIALIZABLE
CREATE FUNCTION f (x INT) RETURNS INT LANGUAGE SQL AS $$
SELECT x+1
$$

statement ok
BEGIN TRANSACTION ISOLATION LEVEL SERIALIZABLE;
CREATE FUNCTION f (x INT) RETURNS INT LANGUAGE SQL AS $$
SELECT x+1
$$;
COMMIT

statement error transaction involving a schema change needs to be SERIALIZABLE
ALTER FUNCTION f (x INT) RENAME TO g

statement error transaction involving a schema change needs to be SERIALIZABLE
GRANT EXECUTE ON FUNCTION f (x INT) TO testuser

statement error transaction involving a schema change needs to be SERIALIZABLE
CREATE TYPE typ AS ENUM('a', 'b')

statement ok
BEGIN TRANSACTION ISOLATION LEVEL SERIALIZABLE;
CREATE TYPE typ AS ENUM('a', 'b');
COMMIT

statement error transaction involving a schema change needs to be SERIALIZABLE
ALTER TYPE typ ADD VALUE 'c'

statement error transaction involving a schema change needs to be SERIALIZABLE
GRANT USAGE ON TYPE typ TO testuser

statement error transaction involving a schema change needs to be SERIALIZABLE
CREATE DATABASE foo

statement error transaction involving a schema change needs to be SERIALIZABLE
ALTER DATABASE postgres RENAME TO foo

statement error transaction involving a schema change needs to be SERIALIZABLE
CREATE SCHEMA s

statement ok
BEGIN TRANSACTION ISOLATION LEVEL SERIALIZABLE;
CREATE SCHEMA s;
COMMIT

statement error transaction involving a schema change needs to be SERIALIZABLE
ALTER SCHEMA s RENAME TO foo

subtest end

statement ok
SET SESSION CHARACTERISTICS AS TRANSACTION ISOLATION LEVEL SERIALIZABLE
14 changes: 7 additions & 7 deletions pkg/sql/logictest/tests/fakedist-disk/generated_test.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

14 changes: 7 additions & 7 deletions pkg/sql/logictest/tests/fakedist-vec-off/generated_test.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

14 changes: 7 additions & 7 deletions pkg/sql/logictest/tests/fakedist/generated_test.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

14 changes: 7 additions & 7 deletions pkg/sql/logictest/tests/local-vec-off/generated_test.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

14 changes: 7 additions & 7 deletions pkg/sql/logictest/tests/local/generated_test.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions pkg/sql/opt/exec/execbuilder/relational.go
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,11 @@ func (b *Builder) buildRelational(e memo.RelExpr) (execPlan, error) {
var err error

if opt.IsDDLOp(e) {
if b.evalCtx.Txn.IsoLevel().ToleratesWriteSkew() {
return execPlan{}, pgerror.Newf(
pgcode.FeatureNotSupported, "transaction involving a schema change needs to be SERIALIZABLE",
)
}
// Mark the statement as containing DDL for use
// in the SQL executor.
b.IsDDL = true
Expand Down
8 changes: 4 additions & 4 deletions pkg/sql/sem/tree/stmt.go
Original file line number Diff line number Diff line change
Expand Up @@ -554,7 +554,7 @@ func (*AlterSequence) StatementType() StatementType { return TypeDDL }
func (*AlterSequence) StatementTag() string { return "ALTER SEQUENCE" }

// StatementReturnType implements the Statement interface.
func (*AlterRole) StatementReturnType() StatementReturnType { return Ack }
func (*AlterRole) StatementReturnType() StatementReturnType { return DDL }

// StatementType implements the Statement interface.
func (*AlterRole) StatementType() StatementType { return TypeDCL }
Expand All @@ -565,7 +565,7 @@ func (*AlterRole) StatementTag() string { return "ALTER ROLE" }
func (*AlterRole) hiddenFromShowQueries() {}

// StatementReturnType implements the Statement interface.
func (*AlterRoleSet) StatementReturnType() StatementReturnType { return Ack }
func (*AlterRoleSet) StatementReturnType() StatementReturnType { return DDL }

// StatementType implements the Statement interface.
func (*AlterRoleSet) StatementType() StatementType { return TypeDCL }
Expand Down Expand Up @@ -932,7 +932,7 @@ func (*CreateType) StatementTag() string { return "CREATE TYPE" }
func (*CreateType) modifiesSchema() bool { return true }

// StatementReturnType implements the Statement interface.
func (*CreateRole) StatementReturnType() StatementReturnType { return Ack }
func (*CreateRole) StatementReturnType() StatementReturnType { return DDL }

// StatementType implements the Statement interface.
func (*CreateRole) StatementType() StatementType { return TypeDCL }
Expand Down Expand Up @@ -1063,7 +1063,7 @@ func (*DropSequence) StatementType() StatementType { return TypeDDL }
func (*DropSequence) StatementTag() string { return "DROP SEQUENCE" }

// StatementReturnType implements the Statement interface.
func (*DropRole) StatementReturnType() StatementReturnType { return Ack }
func (*DropRole) StatementReturnType() StatementReturnType { return DDL }

// StatementType implements the Statement interface.
func (*DropRole) StatementType() StatementType { return TypeDCL }
Expand Down
1 change: 0 additions & 1 deletion pkg/sql/table.go
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,6 @@ func (p *planner) createNonDropDatabaseChangeJob(
func (p *planner) createOrUpdateSchemaChangeJob(
ctx context.Context, tableDesc *tabledesc.Mutable, jobDesc string, mutationID descpb.MutationID,
) error {

// If there is a concurrent schema change using the declarative schema
// changer, then we must fail and wait for that schema change to conclude.
// The error here will be dealt with in
Expand Down

0 comments on commit 6f535ca

Please sign in to comment.