Skip to content

Commit 284847f

Browse files
rafissrafiss
authored
Merge pull request #152471 from fqazi/backport25.2-151737
release-25.2: sql: add support for automatically repairing dangling comments
2 parents 6fff896 + 7fb6f5a commit 284847f

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
@@ -6994,10 +6994,23 @@ CREATE VIEW crdb_internal.kv_repairable_catalog_corruptions (
69946994
FROM
69956995
system.namespace AS ns FULL JOIN system.descriptor AS d ON ns.id = d.id
69966996
),
6997+
orphaned_comments
6998+
AS (
6999+
SELECT
7000+
0 AS parent_id,
7001+
0 AS parent_schema_id,
7002+
'' AS name,
7003+
object_id AS id,
7004+
'comment' AS corruption
7005+
FROM
7006+
system.comments
7007+
WHERE
7008+
object_id NOT IN (SELECT id FROM system.descriptor)
7009+
),
69977010
diag
69987011
AS (
69997012
SELECT
7000-
*,
7013+
parent_id, parent_schema_id, name, id,
70017014
CASE
70027015
WHEN descriptor IS NULL AND id != 29 THEN 'namespace'
70037016
WHEN updated_descriptor != repaired_descriptor THEN 'descriptor'
@@ -7006,6 +7019,8 @@ CREATE VIEW crdb_internal.kv_repairable_catalog_corruptions (
70067019
AS corruption
70077020
FROM
70087021
data
7022+
UNION
7023+
SELECT * FROM orphaned_comments
70097024
)
70107025
SELECT
70117026
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":"func377","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":"func378","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":"func377","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":"func378","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":"func377","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":"func378","UnknownFields":null}
7575
{"Type":"CommandComplete","CommandTag":"CALL"}
7676
{"Type":"ReadyForQuery","TxStatus":"I"}
7777

@@ -97,11 +97,11 @@ ReadyForQuery
9797
----
9898
{"Type":"ParseComplete"}
9999
{"Type":"BindComplete"}
100-
{"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":"func377","UnknownFields":null}
100+
{"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}
101101
{"Type":"RowDescription","Fields":null}
102-
{"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":"func377","UnknownFields":null}
102+
{"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}
103103
{"Type":"RowDescription","Fields":null}
104-
{"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":"func377","UnknownFields":null}
104+
{"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}
105105
{"Type":"CommandComplete","CommandTag":"CALL"}
106106
{"Type":"ReadyForQuery","TxStatus":"I"}
107107

‎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
@@ -5791,6 +5791,13 @@ SELECT
57915791
WHERE
57925792
id = $1 AND id NOT IN (SELECT id FROM system.descriptor)
57935793
)
5794+
WHEN 'comment'
5795+
THEN (
5796+
SELECT
5797+
crdb_internal.unsafe_delete_comment(
5798+
$1
5799+
)
5800+
)
57945801
ELSE NULL
57955802
END
57965803
`,
@@ -7172,6 +7179,28 @@ SELECT
71727179
Volatility: volatility.Volatile,
71737180
},
71747181
),
7182+
"crdb_internal.unsafe_delete_comment": makeBuiltin(
7183+
tree.FunctionProperties{
7184+
Category: builtinconstants.CategorySystemRepair,
7185+
DistsqlBlocklist: true,
7186+
Undocumented: true,
7187+
},
7188+
tree.Overload{
7189+
Types: tree.ParamTypes{
7190+
{Name: "object_id", Typ: types.Int},
7191+
},
7192+
ReturnType: tree.FixedReturnType(types.Bool),
7193+
Fn: func(ctx context.Context, evalCtx *eval.Context, args tree.Datums) (tree.Datum, error) {
7194+
if err := evalCtx.Planner.UnsafeDeleteComment(ctx, int64(*args[0].(*tree.DInt))); err != nil {
7195+
return nil, err
7196+
}
7197+
return tree.DBoolTrue, nil
7198+
},
7199+
Info: "Deletes all system.comments under an object_id, which can be used" +
7200+
" to clean dangling comments",
7201+
Volatility: volatility.Volatile,
7202+
},
7203+
),
71757204

71767205
// Generate some objects.
71777206
"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
@@ -2661,6 +2661,7 @@ var builtinOidsArray = []string{
26612661
2699: `jsonb_path_match(target: jsonb, path: jsonpath) -> bool`,
26622662
2700: `jsonb_path_match(target: jsonb, path: jsonpath, vars: jsonb) -> bool`,
26632663
2701: `jsonb_path_match(target: jsonb, path: jsonpath, vars: jsonb, silent: bool) -> bool`,
2664+
2844: `crdb_internal.unsafe_delete_comment(object_id: int) -> bool`,
26642665
}
26652666

26662667
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)