Skip to content

Commit 8467da0

Browse files
fqazifqazi
committed
sql: add support for automatically repairing dangling comments
Previously, we added logic to detect dangling comments on descriptors but did not include a mechanism to clean them up. This could block initial upgrades due to dangling entries in the system.comments table. This patch adds support for automatically cleaning up these dangling comments. Fixes: #151497 Release note (bug fix): Added an automatic repair for dangling or invalid entries in the system.comment table.
1 parent 9ab7185 commit 8467da0

File tree

8 files changed

+80
-9
lines changed

8 files changed

+80
-9
lines changed

‎pkg/sql/crdb_internal.go‎

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6793,10 +6793,23 @@ CREATE VIEW crdb_internal.kv_repairable_catalog_corruptions (
67936793
FROM
67946794
system.namespace AS ns FULL JOIN system.descriptor AS d ON ns.id = d.id
67956795
),
6796+
orphaned_comments
6797+
AS (
6798+
SELECT
6799+
0 AS parent_id,
6800+
0 AS parent_schema_id,
6801+
'' AS name,
6802+
object_id AS id,
6803+
'comment' AS corruption
6804+
FROM
6805+
system.comments
6806+
WHERE
6807+
object_id NOT IN (SELECT id FROM system.descriptor)
6808+
),
67966809
diag
67976810
AS (
67986811
SELECT
6799-
*,
6812+
parent_id, parent_schema_id, name, id,
68006813
CASE
68016814
WHEN descriptor IS NULL AND id != 29 THEN 'namespace'
68026815
WHEN updated_descriptor != repaired_descriptor THEN 'descriptor'
@@ -6805,6 +6818,8 @@ CREATE VIEW crdb_internal.kv_repairable_catalog_corruptions (
68056818
AS corruption
68066819
FROM
68076820
data
6821+
UNION
6822+
SELECT * FROM orphaned_comments
68086823
)
68096824
SELECT
68106825
parent_id, parent_schema_id, name, id, corruption

‎pkg/sql/faketreeeval/evalctx.go‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -227,6 +227,11 @@ func (ep *DummyEvalPlanner) UpsertDroppedRelationGCTTL(
227227
return errors.WithStack(errEvalPlanner)
228228
}
229229

230+
// UnsafeDeleteComment is part of the Planner interface.
231+
func (ep *DummyEvalPlanner) UnsafeDeleteComment(ctx context.Context, objectID int64) error {
232+
return errors.WithStack(errEvalPlanner)
233+
}
234+
230235
// UserHasAdminRole is part of the Planner interface.
231236
func (ep *DummyEvalPlanner) UserHasAdminRole(
232237
ctx context.Context, user username.SQLUsername,

‎pkg/sql/pgwire/testdata/pgtest/procedure‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -67,11 +67,11 @@ until
6767
ReadyForQuery
6868
----
6969
{"Type":"RowDescription","Fields":null}
70-
{"Severity":"NOTICE","SeverityUnlocalized":"NOTICE","Code":"00000","Message":"foo","Detail":"","Hint":"","Position":0,"InternalPosition":0,"InternalQuery":"","Where":"","SchemaName":"","TableName":"","ColumnName":"","DataTypeName":"","ConstraintName":"","File":"builtins.go","Line":0,"Routine":"func378","UnknownFields":null}
70+
{"Severity":"NOTICE","SeverityUnlocalized":"NOTICE","Code":"00000","Message":"foo","Detail":"","Hint":"","Position":0,"InternalPosition":0,"InternalQuery":"","Where":"","SchemaName":"","TableName":"","ColumnName":"","DataTypeName":"","ConstraintName":"","File":"builtins.go","Line":0,"Routine":"func379","UnknownFields":null}
7171
{"Type":"RowDescription","Fields":null}
72-
{"Severity":"NOTICE","SeverityUnlocalized":"NOTICE","Code":"00000","Message":"bar","Detail":"","Hint":"","Position":0,"InternalPosition":0,"InternalQuery":"","Where":"","SchemaName":"","TableName":"","ColumnName":"","DataTypeName":"","ConstraintName":"","File":"builtins.go","Line":0,"Routine":"func378","UnknownFields":null}
72+
{"Severity":"NOTICE","SeverityUnlocalized":"NOTICE","Code":"00000","Message":"bar","Detail":"","Hint":"","Position":0,"InternalPosition":0,"InternalQuery":"","Where":"","SchemaName":"","TableName":"","ColumnName":"","DataTypeName":"","ConstraintName":"","File":"builtins.go","Line":0,"Routine":"func379","UnknownFields":null}
7373
{"Type":"RowDescription","Fields":null}
74-
{"Severity":"NOTICE","SeverityUnlocalized":"NOTICE","Code":"00000","Message":"baz","Detail":"","Hint":"","Position":0,"InternalPosition":0,"InternalQuery":"","Where":"","SchemaName":"","TableName":"","ColumnName":"","DataTypeName":"","ConstraintName":"","File":"builtins.go","Line":0,"Routine":"func378","UnknownFields":null}
74+
{"Severity":"NOTICE","SeverityUnlocalized":"NOTICE","Code":"00000","Message":"baz","Detail":"","Hint":"","Position":0,"InternalPosition":0,"InternalQuery":"","Where":"","SchemaName":"","TableName":"","ColumnName":"","DataTypeName":"","ConstraintName":"","File":"builtins.go","Line":0,"Routine":"func379","UnknownFields":null}
7575
{"Type":"CommandComplete","CommandTag":"CALL"}
7676
{"Type":"ReadyForQuery","TxStatus":"I"}
7777

@@ -87,10 +87,10 @@ ReadyForQuery
8787
----
8888
{"Type":"ParseComplete"}
8989
{"Type":"BindComplete"}
90-
{"Severity":"NOTICE","SeverityUnlocalized":"NOTICE","Code":"00000","Message":"foo","Detail":"","Hint":"","Position":0,"InternalPosition":0,"InternalQuery":"","Where":"","SchemaName":"","TableName":"","ColumnName":"","DataTypeName":"","ConstraintName":"","File":"builtins.go","Line":0,"Routine":"func378","UnknownFields":null}
90+
{"Severity":"NOTICE","SeverityUnlocalized":"NOTICE","Code":"00000","Message":"foo","Detail":"","Hint":"","Position":0,"InternalPosition":0,"InternalQuery":"","Where":"","SchemaName":"","TableName":"","ColumnName":"","DataTypeName":"","ConstraintName":"","File":"builtins.go","Line":0,"Routine":"func379","UnknownFields":null}
9191
{"Type":"RowDescription","Fields":null}
92-
{"Severity":"NOTICE","SeverityUnlocalized":"NOTICE","Code":"00000","Message":"bar","Detail":"","Hint":"","Position":0,"InternalPosition":0,"InternalQuery":"","Where":"","SchemaName":"","TableName":"","ColumnName":"","DataTypeName":"","ConstraintName":"","File":"builtins.go","Line":0,"Routine":"func378","UnknownFields":null}
92+
{"Severity":"NOTICE","SeverityUnlocalized":"NOTICE","Code":"00000","Message":"bar","Detail":"","Hint":"","Position":0,"InternalPosition":0,"InternalQuery":"","Where":"","SchemaName":"","TableName":"","ColumnName":"","DataTypeName":"","ConstraintName":"","File":"builtins.go","Line":0,"Routine":"func379","UnknownFields":null}
9393
{"Type":"RowDescription","Fields":null}
94-
{"Severity":"NOTICE","SeverityUnlocalized":"NOTICE","Code":"00000","Message":"baz","Detail":"","Hint":"","Position":0,"InternalPosition":0,"InternalQuery":"","Where":"","SchemaName":"","TableName":"","ColumnName":"","DataTypeName":"","ConstraintName":"","File":"builtins.go","Line":0,"Routine":"func378","UnknownFields":null}
94+
{"Severity":"NOTICE","SeverityUnlocalized":"NOTICE","Code":"00000","Message":"baz","Detail":"","Hint":"","Position":0,"InternalPosition":0,"InternalQuery":"","Where":"","SchemaName":"","TableName":"","ColumnName":"","DataTypeName":"","ConstraintName":"","File":"builtins.go","Line":0,"Routine":"func379","UnknownFields":null}
9595
{"Type":"CommandComplete","CommandTag":"CALL"}
9696
{"Type":"ReadyForQuery","TxStatus":"I"}

‎pkg/sql/repair.go‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -851,3 +851,19 @@ func (p *planner) UpsertDroppedRelationGCTTL(
851851
}
852852
return p.txn.Run(ctx, b)
853853
}
854+
855+
// UnsafeDeleteComment deletes all comments under a given object_id.
856+
func (p *planner) UnsafeDeleteComment(ctx context.Context, objectID int64) error {
857+
// Privilege check.
858+
const method = "crdb_internal.unsafe_delete_comment()"
859+
err := checkPlannerStateForRepairFunctions(ctx, p, method)
860+
if err != nil {
861+
return err
862+
}
863+
_, err = p.InternalSQLTxn().Exec(ctx,
864+
"delete-comment",
865+
p.txn,
866+
"DELETE FROM system.comments WHERE object_id = $1",
867+
objectID)
868+
return err
869+
}

‎pkg/sql/sem/builtins/builtins.go‎

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5816,6 +5816,13 @@ SELECT
58165816
WHERE
58175817
id = $1 AND id NOT IN (SELECT id FROM system.descriptor)
58185818
)
5819+
WHEN 'comment'
5820+
THEN (
5821+
SELECT
5822+
crdb_internal.unsafe_delete_comment(
5823+
$1
5824+
)
5825+
)
58195826
ELSE NULL
58205827
END
58215828
`,
@@ -7241,6 +7248,28 @@ SELECT
72417248
Volatility: volatility.Volatile,
72427249
},
72437250
),
7251+
"crdb_internal.unsafe_delete_comment": makeBuiltin(
7252+
tree.FunctionProperties{
7253+
Category: builtinconstants.CategorySystemRepair,
7254+
DistsqlBlocklist: true,
7255+
Undocumented: true,
7256+
},
7257+
tree.Overload{
7258+
Types: tree.ParamTypes{
7259+
{Name: "object_id", Typ: types.Int},
7260+
},
7261+
ReturnType: tree.FixedReturnType(types.Bool),
7262+
Fn: func(ctx context.Context, evalCtx *eval.Context, args tree.Datums) (tree.Datum, error) {
7263+
if err := evalCtx.Planner.UnsafeDeleteComment(ctx, int64(*args[0].(*tree.DInt))); err != nil {
7264+
return nil, err
7265+
}
7266+
return tree.DBoolTrue, nil
7267+
},
7268+
Info: "Deletes all system.comments under an object_id, which can be used" +
7269+
" to clean dangling comments",
7270+
Volatility: volatility.Volatile,
7271+
},
7272+
),
72447273

72457274
// Generate some objects.
72467275
"crdb_internal.generate_test_objects": makeBuiltin(

‎pkg/sql/sem/builtins/fixed_oids.go‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2747,6 +2747,7 @@ var builtinOidsArray = []string{
27472747
2786: `citext(jsonpath: jsonpath) -> citext`,
27482748
2787: `citext(box2d: box2d) -> citext`,
27492749
2788: `citext(vector: vector) -> citext`,
2750+
2844: `crdb_internal.unsafe_delete_comment(object_id: int) -> bool`,
27502751
}
27512752

27522753
var builtinOidsBySignature map[string]oid.Oid

‎pkg/sql/sem/eval/deps.go‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -281,6 +281,9 @@ type Planner interface {
281281
force bool,
282282
) error
283283

284+
// UnsafeDeleteComment is used to delete comments for a non-existent object.
285+
UnsafeDeleteComment(ctx context.Context, objectID int64) error
286+
284287
// UserHasAdminRole returns tuple of bool and error:
285288
// (true, nil) means that the user has an admin role (i.e. root or node)
286289
// (false, nil) means that the user has NO admin role

‎pkg/upgrade/upgrades/first_upgrade_test.go‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -206,6 +206,8 @@ func TestFirstUpgradeRepair(t *testing.T) {
206206
"CREATE SCHEMA bar",
207207
"CREATE TYPE bar.bar AS ENUM ('hello')",
208208
"CREATE FUNCTION bar.bar(a INT) RETURNS INT AS 'SELECT a*a' LANGUAGE SQL",
209+
// Insert an invalid object into the system.comments table
210+
"INSERT INTO system.comments VALUES(0, 4124323, 0, 'comment for dead object')",
209211
)
210212

211213
dbDesc := desctestutils.TestingGetDatabaseDescriptor(kvDB, keys.SystemSQLCodec, "test")
@@ -276,12 +278,12 @@ func TestFirstUpgradeRepair(t *testing.T) {
276278

277279
// Check that the corruption is detected by invalid_objects.
278280
const qDetectCorruption = `SELECT count(*) FROM "".crdb_internal.invalid_objects`
279-
tdb.CheckQueryResults(t, qDetectCorruption, [][]string{{"2"}})
281+
tdb.CheckQueryResults(t, qDetectCorruption, [][]string{{"3"}})
280282

281283
// Check that the corruption is detected by kv_repairable_catalog_corruptions.
282284
const qDetectRepairableCorruption = `
283285
SELECT count(*) FROM "".crdb_internal.kv_repairable_catalog_corruptions`
284-
tdb.CheckQueryResults(t, qDetectRepairableCorruption, [][]string{{"2"}})
286+
tdb.CheckQueryResults(t, qDetectRepairableCorruption, [][]string{{"3"}})
285287

286288
// Wait long enough for precondition check to be effective.
287289
tdb.Exec(t, "CREATE DATABASE test2")

0 commit comments

Comments
 (0)