Skip to content

Commit 1174814

Browse files
spilchenspilchen
committed
sql/importer: make INSPECT-after-IMPORT validation a metamorphic setting
Previously, the setting `bulkio.import.row_count_validation.unsafe.enabled` was a boolean that controlled whether an INSPECT job would be triggered after an IMPORT operation. This commit replaces that boolean with a metamorphic enum setting to improve test coverage and provide finer control. The new enum has three values: - `off`: No validation (default; preserves current behavior) - `async`: Run INSPECT asynchronously in the background (future production default) - `sync`: Run INSPECT synchronously and wait for completion (testing only) The prior `true` value now maps to `async`. The new `sync` option is added specifically for testing. If the INSPECT job fails, it causes the IMPORT to fail as well, enabling roachtests to detect issues more easily without manual validation. The setting has been renamed to `bulkio.import.row_count_validation.unsafe.mode` to reflect the new values. As part of validation testing, we observed limitations with: - hash-sharded indexes - expression indexes Both cases are now blocked by this change. Informs: #154049 Epic: CRDB-30356 Release note: none
1 parent 84b4b91 commit 1174814

File tree

7 files changed

+133
-14
lines changed

7 files changed

+133
-14
lines changed

‎pkg/sql/catalog/descriptor.go‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -515,6 +515,9 @@ type TableDescriptor interface {
515515
// keys, even if they're not defined by the user.
516516
HasPrimaryKey() bool
517517

518+
// IsExpressionIndex returns if the given index is an expression index.
519+
IsExpressionIndex(idx Index) bool
520+
518521
// AllColumns returns a slice of Column interfaces containing the
519522
// table's public columns and column mutations, in the canonical order:
520523
// - all public columns in the same order as in the underlying

‎pkg/sql/catalog/tabledesc/structured.go‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2236,6 +2236,18 @@ func (desc *wrapper) HasPrimaryKey() bool {
22362236
return !desc.PrimaryIndex.Disabled
22372237
}
22382238

2239+
// IsExpressionIndex implements the TableDescriptor interface.
2240+
func (desc *wrapper) IsExpressionIndex(idx catalog.Index) bool {
2241+
for i := 0; i < idx.NumKeyColumns(); i++ {
2242+
colID := idx.GetKeyColumnID(i)
2243+
col := catalog.FindColumnByID(desc, colID)
2244+
if col != nil && col.IsExpressionIndexColumn() {
2245+
return true
2246+
}
2247+
}
2248+
return false
2249+
}
2250+
22392251
// HasColumnBackfillMutation implements the TableDescriptor interface.
22402252
func (desc *wrapper) HasColumnBackfillMutation() bool {
22412253
for _, m := range desc.AllMutations() {

‎pkg/sql/importer/BUILD.bazel‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -88,6 +88,7 @@ go_library(
8888
"//pkg/util/log",
8989
"//pkg/util/log/eventpb",
9090
"//pkg/util/log/logutil",
91+
"//pkg/util/metamorphic",
9192
"//pkg/util/protoutil",
9293
"//pkg/util/retry",
9394
"//pkg/util/syncutil",

‎pkg/sql/importer/import_job.go‎

Lines changed: 52 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,7 @@ import (
3939
"github.com/cockroachdb/cockroach/pkg/util/log"
4040
"github.com/cockroachdb/cockroach/pkg/util/log/eventpb"
4141
"github.com/cockroachdb/cockroach/pkg/util/log/logutil"
42+
"github.com/cockroachdb/cockroach/pkg/util/metamorphic"
4243
"github.com/cockroachdb/cockroach/pkg/util/protoutil"
4344
"github.com/cockroachdb/cockroach/pkg/util/retry"
4445
"github.com/cockroachdb/cockroach/pkg/util/timeutil"
@@ -106,16 +107,44 @@ var performConstraintValidation = settings.RegisterBoolSetting(
106107
settings.WithUnsafe,
107108
)
108109

109-
// TODO(janexing): tune the default to True when INSPECT is merged in stable release.
110-
var importRowCountValidation = settings.RegisterBoolSetting(
110+
// ImportRowCountValidationMode represents the mode for row count validation after import.
111+
type ImportRowCountValidationMode int64
112+
113+
const (
114+
// ImportRowCountValidationOff disables row count validation after import.
115+
ImportRowCountValidationOff ImportRowCountValidationMode = iota
116+
// ImportRowCountValidationAsync enables asynchronous row count validation after import.
117+
ImportRowCountValidationAsync
118+
// ImportRowCountValidationSync enables synchronous (blocking) row count validation after import.
119+
ImportRowCountValidationSync
120+
)
121+
122+
// importRowCountValidationMetamorphicValue determines the default value for
123+
// importRowCountValidation in metamorphic test builds. It randomly selects
124+
// between "off", "async", and "sync" modes to increase test coverage.
125+
var importRowCountValidationMetamorphicValue = metamorphic.ConstantWithTestChoice(
126+
"import-row-count-validation",
127+
"off", // no validation
128+
"async", // background validation
129+
"sync", // blocking validation for tests
130+
)
131+
132+
// TODO(janexing): tune the default to async when INSPECT is merged in stable release.
133+
var importRowCountValidation = settings.RegisterEnumSetting(
111134
settings.ApplicationLevel,
112-
"bulkio.import.row_count_validation.unsafe.enabled",
113-
"enables asynchronous validation of imported data via INSPECT "+
114-
"jobs. When enabled, an INSPECT job runs after import completion to "+
115-
"detect potential data corruption. Disabling this setting may result "+
116-
"in undetected data corruption if the import process fails.",
117-
false,
135+
"bulkio.import.row_count_validation.unsafe.mode",
136+
"controls validation of imported data via INSPECT jobs. "+
137+
"Options: 'off' (no validation), 'async' (background validation), "+
138+
"'sync' (blocking validation). "+
139+
"If disabled, IMPORT will not perform a post-import row count check.",
140+
importRowCountValidationMetamorphicValue,
141+
map[ImportRowCountValidationMode]string{
142+
ImportRowCountValidationOff: "off",
143+
ImportRowCountValidationAsync: "async",
144+
ImportRowCountValidationSync: "sync",
145+
},
118146
settings.WithUnsafe,
147+
settings.WithRetiredName("bulkio.import.row_count_validation.unsafe.enabled"),
119148
)
120149

121150
func getTable(details jobspb.ImportDetails) (jobspb.ImportDetails_Table, error) {
@@ -288,14 +317,18 @@ func (r *importResumer) Resume(ctx context.Context, execCtx interface{}) error {
288317
return err
289318
}
290319

291-
if importRowCountValidation.Get(&p.ExecCfg().Settings.SV) {
320+
validationMode := importRowCountValidation.Get(&p.ExecCfg().Settings.SV)
321+
switch validationMode {
322+
case ImportRowCountValidationOff:
323+
// No validation required.
324+
case ImportRowCountValidationAsync, ImportRowCountValidationSync:
292325
table, err := getTable(details)
293326
if err != nil {
294327
return err
295328
}
296329
tblDesc := tabledesc.NewBuilder(table.Desc).BuildImmutableTable()
297330
if len(tblDesc.PublicNonPrimaryIndexes()) > 0 {
298-
_, err := sql.TriggerInspectJob(
331+
jobID, err := sql.TriggerInspectJob(
299332
ctx,
300333
fmt.Sprintf("import-validation-%s", tblDesc.GetName()),
301334
p.ExecCfg(),
@@ -305,7 +338,15 @@ func (r *importResumer) Resume(ctx context.Context, execCtx interface{}) error {
305338
if err != nil {
306339
return errors.Wrapf(err, "failed to trigger inspect for import validation for table %s", tblDesc.GetName())
307340
}
308-
log.Eventf(ctx, "triggered inspect job for import validation for table %s with AOST %s", tblDesc.GetName(), setPublicTimestamp)
341+
log.Eventf(ctx, "triggered inspect job %d for import validation for table %s with AOST %s", jobID, tblDesc.GetName(), setPublicTimestamp)
342+
343+
// For sync mode, wait for the inspect job to complete.
344+
if validationMode == ImportRowCountValidationSync {
345+
if err := p.ExecCfg().JobRegistry.WaitForJobs(ctx, []jobspb.JobID{jobID}); err != nil {
346+
return errors.Wrapf(err, "failed to wait for inspect job %d for table %s", jobID, tblDesc.GetName())
347+
}
348+
log.Eventf(ctx, "inspect job %d completed for table %s", jobID, tblDesc.GetName())
349+
}
309350
}
310351
}
311352

‎pkg/sql/inspect/index_consistency_check.go‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -379,12 +379,26 @@ func (c *indexConsistencyCheck) loadCatalogInfo(ctx context.Context) error {
379379
// We can only check a secondary index that has a 1-to-1 mapping between
380380
// keys in the primary index. Unsupported indexes should be filtered out
381381
// when the job is created.
382+
// TODO(154862): support partial indexes
382383
if idx.IsPartial() {
383384
return errors.AssertionFailedf(
384385
"unsupported index type for consistency check: partial index",
385386
)
386387
}
388+
// TODO(154762): support hash sharded indexes
389+
if idx.IsSharded() {
390+
return errors.AssertionFailedf(
391+
"unsupported index type for consistency check: hash-sharded index",
392+
)
393+
}
394+
// TODO(154772): support expression indexes
395+
if c.tableDesc.IsExpressionIndex(idx) {
396+
return errors.AssertionFailedf(
397+
"unsupported index type for consistency check: expression index",
398+
)
399+
}
387400
switch idx.GetType() {
401+
// TODO(154860): support inverted indexes
388402
case idxtype.INVERTED, idxtype.VECTOR:
389403
return errors.AssertionFailedf(
390404
"unsupported index type for consistency check: %s", idx.GetType(),

‎pkg/sql/logictest/testdata/logic_test/inspect‎

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -137,3 +137,43 @@ statement ok
137137
INSPECT DATABASE test;
138138

139139
subtest end
140+
141+
subtest inspect_unsupported_indexes
142+
143+
user root
144+
145+
# Test that unsupported indexes (hash-sharded, expression) are skipped.
146+
# Table has both unsupported indexes and a regular index.
147+
statement ok
148+
CREATE TABLE t2 (x INT, y INT, z INT, INDEX hash_idx (x) USING HASH, INDEX expr_idx ((y + z)), INDEX regular_idx (z));
149+
150+
statement ok
151+
INSERT INTO t2 (x, y, z) VALUES (1, 1, 1), (2, 2, 2), (3, 3, 3);
152+
153+
# Should succeed, checking only the regular index.
154+
skipif config local-mixed-25.2 local-mixed-25.3
155+
statement ok
156+
EXPERIMENTAL SCRUB TABLE t2 AS OF SYSTEM TIME '-1us';
157+
158+
# Verify only one check was created (for the regular index).
159+
skipif config local-mixed-25.2 local-mixed-25.3
160+
query TI
161+
SELECT
162+
json_extract_path_text(
163+
crdb_internal.pb_to_json('cockroach.sql.jobs.jobspb.Payload', payload),
164+
'inspectDetails', 'checks', '0', 'type'
165+
) AS check_type,
166+
jsonb_array_length(
167+
crdb_internal.pb_to_json('cockroach.sql.jobs.jobspb.Payload', payload) -> 'inspectDetails' -> 'checks'
168+
) AS num_checks
169+
FROM crdb_internal.system_jobs
170+
WHERE job_type = 'INSPECT'
171+
ORDER BY created DESC
172+
LIMIT 1
173+
----
174+
INSPECT_CHECK_INDEX_CONSISTENCY 1
175+
176+
statement ok
177+
DROP TABLE t2;
178+
179+
subtest end

‎pkg/sql/scrub.go‎

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ import (
1919
"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode"
2020
"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror"
2121
"github.com/cockroachdb/cockroach/pkg/sql/privilege"
22+
"github.com/cockroachdb/cockroach/pkg/sql/sem/idxtype"
2223
"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
2324
"github.com/cockroachdb/cockroach/pkg/sql/sqlerrors"
2425
"github.com/cockroachdb/cockroach/pkg/sql/syntheticprivilege"
@@ -505,11 +506,18 @@ func TriggerInspectJob(
505506
return jobID, errors.AssertionFailedf("must have at least one secondary index")
506507
}
507508

508-
// Create checks for all secondary indexes
509-
// TODO(148365): When INSPECT is added, we want to skip unsupported indexes
510-
// and return a NOTICE.
509+
// Create checks for all secondary indexes, filtering out unsupported types.
510+
// TODO(148365): When INSPECT is added, we want to return a NOTICE for each index skipped.
511511
checks := make([]*jobspb.InspectDetails_Check, 0, len(secIndexes))
512512
for _, index := range secIndexes {
513+
// Skip unsupported index types.
514+
if index.IsPartial() || index.IsSharded() || tableDesc.IsExpressionIndex(index) {
515+
continue
516+
}
517+
switch index.GetType() {
518+
case idxtype.INVERTED, idxtype.VECTOR:
519+
continue
520+
}
513521
checks = append(checks, &jobspb.InspectDetails_Check{
514522
Type: jobspb.InspectCheckIndexConsistency,
515523
TableID: tableDesc.GetID(),

0 commit comments

Comments
 (0)