Skip to content

Commit f07cd76

Browse files
craig[bot]alyshanjahani-crlfqazi
committed
144167: sql: Fix flaky TestInternalAppNamePrefix r=alyshanjahani-crl a=alyshanjahani-crl Previously the test would assert that the internal metrics and user metrics increased or stayed the same based on the app name. However, for the internal metrics this was flaky, so instead of looking for the metric count to increase by 1, we just require that it increased, and most importantly that the user metrics don't increase when internal app name is set. Release note: None Fixes: #144094 144242: sql/schemachanger: enforce order for swapping default column values r=fqazi a=fqazi Previously if the default value of an expression was being swapped, the order was implicit enforced by the order in which objects were added. This works fine until more complex rule evaluation is needed, for example schema_locked is set on the table, so the order of operations can end up shifting. To address this, this patch adds an explicit order for default column expression swaps. Fixes: #144241 Release note: None Co-authored-by: Alyshan Jahani <[email protected]> Co-authored-by: Faizan Qazi <[email protected]>
3 parents 2cc6cac + 96f5189 + 97811b1 commit f07cd76

File tree

9 files changed

+227
-8
lines changed

9 files changed

+227
-8
lines changed

‎pkg/ccl/schemachangerccl/backup_base_generated_test.go‎

Lines changed: 28 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎pkg/sql/conn_executor_test.go‎

Lines changed: 6 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -2457,11 +2457,11 @@ func TestInternalAppNamePrefix(t *testing.T) {
24572457
initialInternalMetrics := sqlServer.InternalMetrics.ExecutedStatementCounters.InsertCount.Count()
24582458
initialUserMetrics := sqlServer.Metrics.ExecutedStatementCounters.InsertCount.Count()
24592459
runner.Exec(t, "INSERT into test values (1, 1)")
2460-
// Confirm only internal metrics increased.
2460+
// Confirm user metrics did not increase.
24612461
finalInternalMetrics := sqlServer.InternalMetrics.ExecutedStatementCounters.InsertCount.Count()
24622462
finalUserMetrics := sqlServer.Metrics.ExecutedStatementCounters.InsertCount.Count()
24632463
require.Equal(t, initialUserMetrics, finalUserMetrics)
2464-
require.Equal(t, initialInternalMetrics+1, finalInternalMetrics)
2464+
require.Greater(t, finalInternalMetrics, initialInternalMetrics)
24652465
})
24662466

24672467
t.Run("app name set in session", func(t *testing.T) {
@@ -2486,20 +2486,18 @@ func TestInternalAppNamePrefix(t *testing.T) {
24862486
runner.Exec(t, fmt.Sprintf("set application_name='%v'", catconstants.InternalAppNamePrefix+"mytest"))
24872487
runner.Exec(t, "INSERT into test values (2, 1)")
24882488

2489-
// Confirm only internal metrics increased.
2489+
// Confirm user metrics did not increase.
24902490
finalInternalMetrics := sqlServer.InternalMetrics.ExecutedStatementCounters.InsertCount.Count()
24912491
finalUserMetrics := sqlServer.Metrics.ExecutedStatementCounters.InsertCount.Count()
24922492
require.Equal(t, initialUserMetrics, finalUserMetrics)
2493-
require.Equal(t, initialInternalMetrics+1, finalInternalMetrics)
2493+
require.Greater(t, finalInternalMetrics, initialInternalMetrics)
24942494

24952495
// Reset app name.
24962496
runner.Exec(t, "set application_name='mytest'")
24972497
runner.Exec(t, "INSERT into test values (3, 1)")
24982498

2499-
// Confirm only user metrics increased.
2500-
finalInternalMetrics = sqlServer.InternalMetrics.ExecutedStatementCounters.InsertCount.Count()
2499+
// Confirm user metrics increased.
25012500
finalUserMetrics = sqlServer.Metrics.ExecutedStatementCounters.InsertCount.Count()
2502-
require.Equal(t, initialUserMetrics+1, finalUserMetrics)
2503-
require.Equal(t, initialInternalMetrics+1, finalInternalMetrics)
2501+
require.Greater(t, finalUserMetrics, initialUserMetrics)
25042502
})
25052503
}

‎pkg/sql/schemachanger/scplan/internal/rules/current/dep_drop_column.go‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -305,3 +305,25 @@ func init() {
305305
},
306306
)
307307
}
308+
309+
// Special rules to ensure that swapping default expressions is done in order.
310+
func init() {
311+
registerDepRule(
312+
"handle default column expression swaps",
313+
scgraph.Precedence,
314+
"old-column-expression", "new-column-expression",
315+
func(from, to NodeVars) rel.Clauses {
316+
return rel.Clauses{
317+
from.Type((*scpb.ColumnDefaultExpression)(nil), (*scpb.ColumnOnUpdateExpression)(nil)),
318+
to.Type((*scpb.ColumnDefaultExpression)(nil), (*scpb.ColumnOnUpdateExpression)(nil)),
319+
from.El.AttrEqVar(rel.Type, "same-type"),
320+
to.El.AttrEqVar(rel.Type, "same-type"),
321+
JoinOnColumnID(from, to, "table-id", "col-id"),
322+
from.TargetStatus(scpb.ToAbsent),
323+
from.CurrentStatus(scpb.Status_ABSENT),
324+
to.TargetStatus(scpb.ToPublic),
325+
to.CurrentStatus(scpb.Status_PUBLIC),
326+
}
327+
},
328+
)
329+
}

‎pkg/sql/schemachanger/scplan/internal/rules/current/testdata/deprules‎

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3676,6 +3676,22 @@ deprules
36763676
- $function-parent-Node[CurrentStatus] = PUBLIC
36773677
- joinTargetNode($function-name, $function-name-Target, $function-name-Node)
36783678
- joinTargetNode($function-parent, $function-parent-Target, $function-parent-Node)
3679+
- name: handle default column expression swaps
3680+
from: old-column-expression-Node
3681+
kind: Precedence
3682+
to: new-column-expression-Node
3683+
query:
3684+
- $old-column-expression[Type] IN ['*scpb.ColumnDefaultExpression', '*scpb.ColumnOnUpdateExpression']
3685+
- $new-column-expression[Type] IN ['*scpb.ColumnDefaultExpression', '*scpb.ColumnOnUpdateExpression']
3686+
- $old-column-expression[Type] = $same-type
3687+
- $new-column-expression[Type] = $same-type
3688+
- joinOnColumnID($old-column-expression, $new-column-expression, $table-id, $col-id)
3689+
- $old-column-expression-Target[TargetStatus] = ABSENT
3690+
- $old-column-expression-Node[CurrentStatus] = ABSENT
3691+
- $new-column-expression-Target[TargetStatus] = PUBLIC
3692+
- $new-column-expression-Node[CurrentStatus] = PUBLIC
3693+
- joinTargetNode($old-column-expression, $old-column-expression-Target, $old-column-expression-Node)
3694+
- joinTargetNode($new-column-expression, $new-column-expression-Target, $new-column-expression-Node)
36793695
- name: index data exists as soon as index accepts backfills
36803696
from: index-name-Node
36813697
kind: SameStagePrecedence
@@ -8581,6 +8597,22 @@ deprules
85818597
- $function-parent-Node[CurrentStatus] = PUBLIC
85828598
- joinTargetNode($function-name, $function-name-Target, $function-name-Node)
85838599
- joinTargetNode($function-parent, $function-parent-Target, $function-parent-Node)
8600+
- name: handle default column expression swaps
8601+
from: old-column-expression-Node
8602+
kind: Precedence
8603+
to: new-column-expression-Node
8604+
query:
8605+
- $old-column-expression[Type] IN ['*scpb.ColumnDefaultExpression', '*scpb.ColumnOnUpdateExpression']
8606+
- $new-column-expression[Type] IN ['*scpb.ColumnDefaultExpression', '*scpb.ColumnOnUpdateExpression']
8607+
- $old-column-expression[Type] = $same-type
8608+
- $new-column-expression[Type] = $same-type
8609+
- joinOnColumnID($old-column-expression, $new-column-expression, $table-id, $col-id)
8610+
- $old-column-expression-Target[TargetStatus] = ABSENT
8611+
- $old-column-expression-Node[CurrentStatus] = ABSENT
8612+
- $new-column-expression-Target[TargetStatus] = PUBLIC
8613+
- $new-column-expression-Node[CurrentStatus] = PUBLIC
8614+
- joinTargetNode($old-column-expression, $old-column-expression-Target, $old-column-expression-Node)
8615+
- joinTargetNode($new-column-expression, $new-column-expression-Target, $new-column-expression-Node)
85848616
- name: index data exists as soon as index accepts backfills
85858617
from: index-name-Node
85868618
kind: SameStagePrecedence

‎pkg/sql/schemachanger/sctest_generated_test.go‎

Lines changed: 42 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎pkg/sql/schemachanger/testdata/end_to_end/alter_table_alter_column_swap_default/alter_table_alter_column_swap_default.definition‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
setup
2+
CREATE TABLE t (i INT PRIMARY KEY, j INT DEFAULT 32);
3+
----
4+
5+
test
6+
ALTER TABLE t ALTER COLUMN j SET DEFAULT 42;
7+
----

‎pkg/sql/schemachanger/testdata/end_to_end/alter_table_alter_column_swap_default/alter_table_alter_column_swap_default.explain‎

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
/* setup */
2+
CREATE TABLE t (i INT PRIMARY KEY, j INT DEFAULT 32);
3+
4+
/* test */
5+
EXPLAIN (DDL) ALTER TABLE t ALTER COLUMN j SET DEFAULT 42;
6+
----
7+
Schema change plan for ALTER TABLE ‹defaultdb›.‹public›.‹t› ALTER COLUMN ‹j› SET DEFAULT ‹42›;
8+
├── StatementPhase
9+
│ └── Stage 1 of 1 in StatementPhase
10+
│ ├── 1 element transitioning toward PUBLIC
11+
│ │ └── ABSENT → PUBLIC ColumnDefaultExpression:{DescID: 104 (t), ColumnID: 2 (j), Expr: 42:::INT8}
12+
│ ├── 1 element transitioning toward ABSENT
13+
│ │ └── PUBLIC → ABSENT ColumnDefaultExpression:{DescID: 104 (t), ColumnID: 2 (j), Expr: 32:::INT8}
14+
│ └── 2 Mutation operations
15+
│ ├── RemoveColumnDefaultExpression {"ColumnID":2,"TableID":104}
16+
│ └── AddColumnDefaultExpression {"Default":{"ColumnID":2,"TableID":104}}
17+
└── PreCommitPhase
18+
├── Stage 1 of 2 in PreCommitPhase
19+
│ ├── 1 element transitioning toward PUBLIC
20+
│ │ └── PUBLIC → ABSENT ColumnDefaultExpression:{DescID: 104 (t), ColumnID: 2 (j), Expr: 42:::INT8}
21+
│ ├── 1 element transitioning toward ABSENT
22+
│ │ └── ABSENT → PUBLIC ColumnDefaultExpression:{DescID: 104 (t), ColumnID: 2 (j), Expr: 32:::INT8}
23+
│ └── 1 Mutation operation
24+
│ └── UndoAllInTxnImmediateMutationOpSideEffects
25+
└── Stage 2 of 2 in PreCommitPhase
26+
├── 1 element transitioning toward PUBLIC
27+
│ └── ABSENT → PUBLIC ColumnDefaultExpression:{DescID: 104 (t), ColumnID: 2 (j), Expr: 42:::INT8}
28+
├── 1 element transitioning toward ABSENT
29+
│ └── PUBLIC → ABSENT ColumnDefaultExpression:{DescID: 104 (t), ColumnID: 2 (j), Expr: 32:::INT8}
30+
└── 2 Mutation operations
31+
├── RemoveColumnDefaultExpression {"ColumnID":2,"TableID":104}
32+
└── AddColumnDefaultExpression {"Default":{"ColumnID":2,"TableID":104}}

‎pkg/sql/schemachanger/testdata/end_to_end/alter_table_alter_column_swap_default/alter_table_alter_column_swap_default.explain_shape‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
/* setup */
2+
CREATE TABLE t (i INT PRIMARY KEY, j INT DEFAULT 32);
3+
4+
/* test */
5+
EXPLAIN (DDL, SHAPE) ALTER TABLE t ALTER COLUMN j SET DEFAULT 42;
6+
----
7+
Schema change plan for ALTER TABLE ‹defaultdb›.‹public›.‹t› ALTER COLUMN ‹j› SET DEFAULT ‹42›;
8+
└── execute 1 system table mutations transaction

‎pkg/sql/schemachanger/testdata/end_to_end/alter_table_alter_column_swap_default/alter_table_alter_column_swap_default.side_effects‎

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,50 @@
1+
/* setup */
2+
CREATE TABLE t (i INT PRIMARY KEY, j INT DEFAULT 32);
3+
----
4+
...
5+
+object {100 101 t} -> 104
6+
7+
/* test */
8+
ALTER TABLE t ALTER COLUMN j SET DEFAULT 42;
9+
----
10+
begin transaction #1
11+
# begin StatementPhase
12+
checking for feature: ALTER TABLE
13+
increment telemetry for sql.schema.alter_table
14+
increment telemetry for sql.schema.alter_table.set_default
15+
## StatementPhase stage 1 of 1 with 2 MutationType ops
16+
upsert descriptor #104
17+
...
18+
oid: 20
19+
width: 64
20+
- - defaultExpr: 32:::INT8
21+
+ - defaultExpr: 42:::INT8
22+
id: 2
23+
name: j
24+
...
25+
time: {}
26+
unexposedParentSchemaId: 101
27+
- version: "1"
28+
+ version: "2"
29+
# end StatementPhase
30+
# begin PreCommitPhase
31+
## PreCommitPhase stage 1 of 2 with 1 MutationType op
32+
undo all catalog changes within txn #1
33+
persist all catalog changes to storage
34+
## PreCommitPhase stage 2 of 2 with 2 MutationType ops
35+
upsert descriptor #104
36+
...
37+
oid: 20
38+
width: 64
39+
- - defaultExpr: 32:::INT8
40+
+ - defaultExpr: 42:::INT8
41+
id: 2
42+
name: j
43+
...
44+
time: {}
45+
unexposedParentSchemaId: 101
46+
- version: "1"
47+
+ version: "2"
48+
persist all catalog changes to storage
49+
# end PreCommitPhase
50+
commit transaction #1

0 commit comments

Comments
 (0)