Skip to content

Commit

Permalink
sql/schemachanger: prevent concurrent declarative/legacy usage for types
Browse files Browse the repository at this point in the history
Fixes: #89831, #87572

Previously, it was possible for the declarative schema changer
drop type operations and other legacy schema changer operations
for types to overlap. This was problematic because if any legacy
type descriptor-related schema change job saw that a type descriptor
is labelled as dropped, we would delete the descriptor as a part
the legacy schema changer job.

This would happen for example if we had a DROP TYPE and ALTER TYPE
operations running concurrently, with the former in the declarative
schema changer. The declarative schema changer during the commit phase
will mark the descriptor as dropped, and delete the descriptor using a job.
The alter type legacy schema changer job if started afterwards will drop
the type descriptor if it's marked as dropped before the declarative
schema change job starts, causing the declarative to hang later on if
the descriptor is missing.

To address this, this patch will prevent the legacy schema changer jobs
from dropping a type descriptor, if a declarative schema changer exists
inside it.

Release note: None
  • Loading branch information
fqazi committed Oct 12, 2022
1 parent 575684e commit 08ad816
Show file tree
Hide file tree
Showing 2 changed files with 46 additions and 2 deletions.
43 changes: 43 additions & 0 deletions pkg/sql/logictest/testdata/logic_test/drop_type
Original file line number Diff line number Diff line change
Expand Up @@ -485,3 +485,46 @@ DROP TYPE sc2.typ;
# This is just cleanup.
statement ok
DROP SCHEMA sc2 CASCADE;

# Concurrent DROP TYPE and other ALTER TYPE statements can potentially
# collide with each other, since the code used to as a part of TYPE SCHEMA
# CHANGER jobs clean up the descriptor if it was marked as dropped. This
# should no longer happen, if a declarative schema changer state exists.
# Without the fix this test would hang.
subtest concurrent_declarative_89831

statement ok
CREATE DATABASE db1 OWNER testuser;
CREATE SCHEMA db1.sc1 AUTHORIZATION testuser;

statement ok
CREATE TYPE db1.sc1.typ AS ENUM ('a');

statement ok
SET CLUSTER SETTING jobs.debug.pausepoints="typeschemachanger.before.exec"

skipif config local-legacy-schema-changer
statement error job \d+ was paused before it completed with reason: pause point "typeschemachanger.before.exec" hit
ALTER TYPE db1.sc1.typ OWNER TO testuser

statement ok
SET CLUSTER SETTING jobs.debug.pausepoints=""

statement ok
SET CLUSTER SETTING jobs.debug.pausepoints="newschemachanger.before.exec"

skipif config local-legacy-schema-changer
statement error job \d+ was paused before it completed with reason: pause point "newschemachanger.before.exec" hit
DROP SCHEMA db1.sc1 CASCADE

statement ok
SET CLUSTER SETTING jobs.debug.pausepoints=""

statement ok
RESUME JOB (SELECT job_id FROM crdb_internal.jobs WHERE description LIKE 'ALTER TYPE%' AND status='paused' FETCH FIRST 1 ROWS ONLY);

statement ok
RESUME JOB (SELECT job_id FROM crdb_internal.jobs WHERE description LIKE 'ALTER TYPE%' AND status='paused' FETCH FIRST 1 ROWS ONLY);

statement ok
RESUME JOB (SELECT job_id FROM crdb_internal.jobs WHERE description LIKE 'DROP SCHEMA%' AND status='paused' FETCH FIRST 1 ROWS ONLY);
5 changes: 3 additions & 2 deletions pkg/sql/type_change.go
Original file line number Diff line number Diff line change
Expand Up @@ -484,8 +484,9 @@ func (t *typeSchemaChanger) exec(ctx context.Context) error {
}
}

// If the type is being dropped, remove the descriptor here.
if typeDesc.Dropped() {
// If the type is being dropped, remove the descriptor here only
// if the declarative schema changer is not in use.
if typeDesc.Dropped() && typeDesc.GetDeclarativeSchemaChangerState() == nil {
if err := t.execCfg.DB.Txn(ctx, func(ctx context.Context, txn *kv.Txn) error {
b := txn.NewBatch()
b.Del(catalogkeys.MakeDescMetadataKey(codec, typeDesc.GetID()))
Expand Down

0 comments on commit 08ad816

Please sign in to comment.