Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reporting Foreign Key references Partitioned table in analyze and assessment #2145

Merged
merged 10 commits into from
Jan 6, 2025
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ CREATE TABLE sales (

-- cases for multi column list partition, to be reported during analyze-schema
CREATE TABLE test_1 (
id numeric NOT NULL,
id numeric NOT NULL REFERENCES sales_data(sales_id),
country_code varchar(3),
record_type varchar(5),
descriptions varchar(50),
Expand Down
14 changes: 12 additions & 2 deletions migtests/tests/analyze-schema/expected_issues.json
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,16 @@
"GH": "",
"MinimumVersionsFixedIn": null
},
{
"IssueType": "unsupported_features",
"ObjectType": "TABLE",
"ObjectName": "test_1, constraint: (test_1_id_fkey)",
"Reason": "Foreign key constraint references partitioned table",
"SqlStatement": "CREATE TABLE test_1 (\n\tid numeric NOT NULL REFERENCES sales_data(sales_id),\n\tcountry_code varchar(3),\n\trecord_type varchar(5),\n\tdescriptions varchar(50),\n\tPRIMARY KEY (id)\n) PARTITION BY LIST (country_code, record_type) ;",
"Suggestion": "No workaround available ",
"GH": "",
"MinimumVersionsFixedIn": null
},
{
"IssueType": "unsupported_features",
"ObjectType": "MVIEW",
Expand Down Expand Up @@ -416,7 +426,7 @@
"ObjectType": "TABLE",
"ObjectName": "test_1",
"Reason": "cannot use \"list\" partition strategy with more than one column",
"SqlStatement": "CREATE TABLE test_1 (\n\tid numeric NOT NULL,\n\tcountry_code varchar(3),\n\trecord_type varchar(5),\n\tdescriptions varchar(50),\n\tPRIMARY KEY (id)\n) PARTITION BY LIST (country_code, record_type) ;",
"SqlStatement": "CREATE TABLE test_1 (\n\tid numeric NOT NULL REFERENCES sales_data(sales_id),\n\tcountry_code varchar(3),\n\trecord_type varchar(5),\n\tdescriptions varchar(50),\n\tPRIMARY KEY (id)\n) PARTITION BY LIST (country_code, record_type) ;",
"DocsLink": "https://docs.yugabyte.com/preview/yugabyte-voyager/known-issues/mysql/#multi-column-partition-by-list-is-not-supported",
"Suggestion": "Make it a single column partition by list or choose other supported Partitioning methods",
"GH": "https://github.com/yugabyte/yb-voyager/issues/699",
Expand Down Expand Up @@ -1465,7 +1475,7 @@
"ObjectType": "TABLE",
"ObjectName": "test_1",
"Reason": "insufficient columns in the PRIMARY KEY constraint definition in CREATE TABLE - (country_code, record_type)",
"SqlStatement": "CREATE TABLE test_1 (\n\tid numeric NOT NULL,\n\tcountry_code varchar(3),\n\trecord_type varchar(5),\n\tdescriptions varchar(50),\n\tPRIMARY KEY (id)\n) PARTITION BY LIST (country_code, record_type) ;",
"SqlStatement": "CREATE TABLE test_1 (\n\tid numeric NOT NULL REFERENCES sales_data(sales_id),\n\tcountry_code varchar(3),\n\trecord_type varchar(5),\n\tdescriptions varchar(50),\n\tPRIMARY KEY (id)\n) PARTITION BY LIST (country_code, record_type) ;",
"Suggestion": "Add all Partition columns to Primary Key",
"GH": "https://github.com/yugabyte/yb-voyager/issues/578",
"DocsLink": "https://docs.yugabyte.com/preview/yugabyte-voyager/known-issues/oracle/#partition-key-column-not-part-of-primary-key-columns",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@
{
"ObjectType": "TABLE",
"TotalCount": 82,
"InvalidCount": 35,
"InvalidCount": 36,
"ObjectNames": "public.employeesforview, schema2.employeesforview, public.\"Case_Sensitive_Columns\", public.\"Mixed_Case_Table_Name_Test\", public.\"Recipients\", public.\"WITH\", public.audit, public.bigint_multirange_table, public.boston, public.c, public.child_table, public.citext_type, public.combined_tbl, public.date_multirange_table, public.documents, public.employees, public.employees2, public.employees3, public.ext_test, public.foo, public.inet_type, public.int_multirange_table, public.library_nested, public.london, public.mixed_data_types_table1, public.mixed_data_types_table2, public.numeric_multirange_table, public.orders, public.orders2, public.orders_lateral, public.ordersentry, public.parent_table, public.products, public.sales_region, public.session_log, public.session_log1, public.session_log2, public.sydney, public.test_exclude_basic, public.test_jsonb, public.test_xml_type, public.timestamp_multirange_table, public.timestamptz_multirange_table, public.ts_query_table, public.tt, public.with_example1, public.with_example2, schema2.\"Case_Sensitive_Columns\", schema2.\"Mixed_Case_Table_Name_Test\", schema2.\"Recipients\", schema2.\"WITH\", schema2.audit, schema2.bigint_multirange_table, schema2.boston, schema2.c, schema2.child_table, schema2.date_multirange_table, schema2.employees2, schema2.ext_test, schema2.foo, schema2.int_multirange_table, schema2.london, schema2.mixed_data_types_table1, schema2.mixed_data_types_table2, schema2.numeric_multirange_table, schema2.orders, schema2.orders2, schema2.parent_table, schema2.products, schema2.sales_region, schema2.session_log, schema2.session_log1, schema2.session_log2, schema2.sydney, schema2.test_xml_type, schema2.timestamp_multirange_table, schema2.timestamptz_multirange_table, schema2.tt, schema2.with_example1, schema2.with_example2, test_views.view_table1, test_views.view_table2"
},
{
Expand Down Expand Up @@ -634,6 +634,16 @@
],
"MinimumVersionsFixedIn": null
},
{
"FeatureName": "Foreign key constraint references partitioned table",
"Objects": [
{
"ObjectName": "public.test_jsonb, constraint: (test_jsonb_id_region_fkey)",
"SqlStatement": "ALTER TABLE ONLY public.test_jsonb\n ADD CONSTRAINT test_jsonb_id_region_fkey FOREIGN KEY (id, region) REFERENCES public.sales_region(id, region);"
}
],
"MinimumVersionsFixedIn": null
},
{
"FeatureName": "Regex Functions",
"Objects": [
Expand Down Expand Up @@ -1011,7 +1021,7 @@
"SchemaName": "public",
"ObjectName": "test_jsonb",
"RowCount": 0,
"ColumnCount": 3,
"ColumnCount": 4,
"Reads": 0,
"Writes": 0,
"ReadsPerSecond": 0,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,9 @@ WITH CHECK OPTION;
CREATE TABLE public.test_jsonb (
id integer,
data jsonb,
data2 text
data2 text,
region text,
FOREIGN KEY (id, region) REFERENCES sales_region(id, region)
);

CREATE TABLE public.inet_type (
Expand Down
1 change: 1 addition & 0 deletions yb-voyager/cmd/analyzeSchema.go
Original file line number Diff line number Diff line change
Expand Up @@ -654,6 +654,7 @@ func convertIssueInstanceToAnalyzeIssue(issueInstance queryissue.QueryIssue, fil
queryissue.EXCLUSION_CONSTRAINTS,
queryissue.DEFERRABLE_CONSTRAINTS,
queryissue.PK_UK_ON_COMPLEX_DATATYPE,
queryissue.FOREIGN_KEY_REFERENCES_PARTITIONED_TABLE,
}
/*
TODO:
Expand Down
2 changes: 1 addition & 1 deletion yb-voyager/cmd/assessMigrationCommand.go
Original file line number Diff line number Diff line change
Expand Up @@ -1028,7 +1028,7 @@ func fetchUnsupportedPGFeaturesFromSchemaReport(schemaAnalysisReport utils.Schem
unsupportedFeatures = append(unsupportedFeatures, getUnsupportedFeaturesFromSchemaAnalysisReport(queryissue.JSON_CONSTRUCTOR_FUNCTION_NAME, "", queryissue.JSON_CONSTRUCTOR_FUNCTION, schemaAnalysisReport, false, ""))
unsupportedFeatures = append(unsupportedFeatures, getUnsupportedFeaturesFromSchemaAnalysisReport(queryissue.AGGREGATION_FUNCTIONS_NAME, "", queryissue.AGGREGATE_FUNCTION, schemaAnalysisReport, false, ""))
unsupportedFeatures = append(unsupportedFeatures, getUnsupportedFeaturesFromSchemaAnalysisReport(queryissue.SECURITY_INVOKER_VIEWS_NAME, "", queryissue.SECURITY_INVOKER_VIEWS, schemaAnalysisReport, false, ""))

unsupportedFeatures = append(unsupportedFeatures, getUnsupportedFeaturesFromSchemaAnalysisReport(queryissue.FOREIGN_KEY_REFERENCES_PARTITIONED_TABLE_NAME, "", queryissue.FOREIGN_KEY_REFERENCES_PARTITIONED_TABLE, schemaAnalysisReport, false, ""))
return lo.Filter(unsupportedFeatures, func(f UnsupportedFeature, _ int) bool {
return len(f.Objects) > 0
}), nil
Expand Down
3 changes: 3 additions & 0 deletions yb-voyager/src/query/queryissue/constants.go
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,9 @@ const (
MULTI_RANGE_DATATYPE = "MULTI_RANGE_DATATYPE"
COPY_FROM_WHERE = "COPY FROM ... WHERE"
COPY_ON_ERROR = "COPY ... ON_ERROR"

FOREIGN_KEY_REFERENCES_PARTITIONED_TABLE = "FOREIGN_KEY_REFERENCED_PARTITIONED_TABLE"
FOREIGN_KEY_REFERENCES_PARTITIONED_TABLE_NAME = "Foreign key constraint references partitioned table"
)

// Object types
Expand Down
20 changes: 20 additions & 0 deletions yb-voyager/src/query/queryissue/detectors_ddl.go
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,15 @@ func (d *TableIssueDetector) DetectIssues(obj queryparser.DDLObject) ([]QueryIss
))
}

if c.ConstraintType == queryparser.FOREIGN_CONSTR_TYPE && d.partitionTablesMap[c.ReferencedTable] {
issues = append(issues, NewForeignKeyReferencesPartitionedTableIssue(
TABLE_OBJECT_TYPE,
table.GetObjectName(),
"",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lets add sqlstatement also here?
Moving forward(flattening of issues in assessment report), we will have this field for each issue and its good to have sql statement available in each case.

Copy link
Contributor Author

@priyanshi-yb priyanshi-yb Jan 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SQL statement is already added for these later in the GetDDLIssues

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm.. not sure what is the reason of adding it there.
But adding at the time of issue creation makes more sense to me.
is the sql statement getting modified somewhere in between.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No not changing sql statement anywhere, it's just that skipping the overhead of passing the query to these ddl_detectors, so adding it in one go there itself.

c.ConstraintName,
))
}

if c.IsPrimaryKeyORUniqueConstraint() {
for _, col := range c.Columns {
unsupportedColumnsForTable, ok := d.columnsWithUnsupportedIndexDatatypes[table.GetObjectName()]
Expand Down Expand Up @@ -444,6 +453,17 @@ func (aid *AlterTableIssueDetector) DetectIssues(obj queryparser.DDLObject) ([]Q
))
}

if alter.ConstraintType == queryparser.FOREIGN_CONSTR_TYPE &&
aid.partitionTablesMap[alter.ConstraintReferencedTable] {
//FK constraint references partitioned table
issues = append(issues, NewForeignKeyReferencesPartitionedTableIssue(
TABLE_OBJECT_TYPE,
alter.GetObjectName(),
"",
alter.ConstraintName,
))
}

if alter.ConstraintType == queryparser.PRIMARY_CONSTR_TYPE &&
aid.partitionTablesMap[alter.GetObjectName()] {
issues = append(issues, NewAlterTableAddPKOnPartiionIssue(
Expand Down
15 changes: 15 additions & 0 deletions yb-voyager/src/query/queryissue/issues_ddl.go
Original file line number Diff line number Diff line change
Expand Up @@ -465,3 +465,18 @@ var securityInvokerViewIssue = issue.Issue{
func NewSecurityInvokerViewIssue(objectType string, objectName string, SqlStatement string) QueryIssue {
return newQueryIssue(securityInvokerViewIssue, objectType, objectName, SqlStatement, map[string]interface{}{})
}

var foreignKeyReferencesPartitionedTableIssue = issue.Issue{
Type: FOREIGN_KEY_REFERENCES_PARTITIONED_TABLE,
TypeName: FOREIGN_KEY_REFERENCES_PARTITIONED_TABLE_NAME,
Suggestion: "No workaround available ",
GH: "", // TODO
DocsLink: "", // TODO
}

func NewForeignKeyReferencesPartitionedTableIssue(objectType string, objectName string, SqlStatement string, constraintName string) QueryIssue {
details := map[string]interface{}{
CONSTRAINT_NAME: constraintName,
}
return newQueryIssue(foreignKeyReferencesPartitionedTableIssue, objectType, objectName, SqlStatement, details)
}
18 changes: 18 additions & 0 deletions yb-voyager/src/query/queryissue/issues_ddl_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import (
"github.com/jackc/pgx/v5"
"github.com/stretchr/testify/assert"
"github.com/testcontainers/testcontainers-go/modules/yugabytedb"

"github.com/yugabyte/yb-voyager/yb-voyager/src/issue"
"github.com/yugabyte/yb-voyager/yb-voyager/src/ybversion"
testutils "github.com/yugabyte/yb-voyager/yb-voyager/test/utils"
Expand Down Expand Up @@ -273,6 +274,19 @@ func testSecurityInvokerView(t *testing.T) {
assertErrorCorrectlyThrownForIssueForYBVersion(t, err, "unrecognized parameter", securityInvokerViewIssue)
}

func testForeignKeyReferencesPartitionedTableIssue(t *testing.T) {
ctx := context.Background()
conn, err := getConn()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider using the new testcontainers package in this test file.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we do it separately for these issues integration tests as it will require changes to all these tests to not use the connection string to run the SQL commands directly and assert the error instead use the ExecuteSqls of the testContainer

assert.NoError(t, err)

defer conn.Close(context.Background())
_, err = conn.Exec(ctx, `
CREATE TABLE abc1(id int PRIMARY KEY, val text) PARTITION BY RANGE (id);
CREATE TABLE abc_fk(id int PRIMARY KEY, abc_id INT REFERENCES abc1(id), val text) ;`)

assertErrorCorrectlyThrownForIssueForYBVersion(t, err, `cannot reference partitioned table "abc1"`, foreignKeyReferencesPartitionedTableIssue)
}

func TestDDLIssuesInYBVersion(t *testing.T) {
var err error
ybVersion := os.Getenv("YB_VERSION")
Expand Down Expand Up @@ -328,6 +342,10 @@ func TestDDLIssuesInYBVersion(t *testing.T) {

success = t.Run(fmt.Sprintf("%s-%s", "multi range datatype", ybVersion), testMultiRangeDatatypeIssue)
assert.True(t, success)

success = t.Run(fmt.Sprintf("%s-%s", "security invoker view", ybVersion), testSecurityInvokerView)
assert.True(t, success)

success = t.Run(fmt.Sprintf("%s-%s", "foreign key referenced partitioned table", ybVersion), testForeignKeyReferencesPartitionedTableIssue)
assert.True(t, success)
}
55 changes: 54 additions & 1 deletion yb-voyager/src/query/queryissue/parser_issue_detector_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,9 @@ import (
"github.com/google/go-cmp/cmp"
"github.com/samber/lo"
"github.com/stretchr/testify/assert"
testutils "github.com/yugabyte/yb-voyager/yb-voyager/test/utils"

"github.com/yugabyte/yb-voyager/yb-voyager/src/ybversion"
testutils "github.com/yugabyte/yb-voyager/yb-voyager/test/utils"
)

const (
Expand Down Expand Up @@ -754,3 +754,56 @@ func TestCopyUnsupportedConstructIssuesDetected(t *testing.T) {
}
}
}

func TestForeignKeyReferencesPartitionedTableIssues(t *testing.T) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we remove any redundant test here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are no redundant cases here all of them have different ways of defining the constraint and qualified or unqualified table names etc..

requiredDDLs := []string{
`CREATE TABLE abc1(id int PRIMARY KEY, val text) PARTITION BY RANGE (id);`,
`CREATE TABLE schema1.abc(id int PRIMARY KEY, val text) PARTITION BY RANGE (id);`,
}
stmt1 := `CREATE TABLE abc_fk(id int PRIMARY KEY, abc_id INT REFERENCES abc1(id), val text) ;`
stmt2 := `ALTER TABLE schema1.abc_fk1
ADD CONSTRAINT fk FOREIGN KEY (abc1_id)
REFERENCES schema1.abc (id);
`
stmt3 := `CREATE TABLE abc_fk (
id INT PRIMARY KEY,
abc_id INT,
val TEXT,
CONSTRAINT fk_abc FOREIGN KEY (abc_id) REFERENCES abc1(id)
);
`

stmt4 := `CREATE TABLE schema1.abc_fk(id int PRIMARY KEY, abc_id INT, val text, FOREIGN KEY (abc_id) REFERENCES schema1.abc(id));`

ddlStmtsWithIssues := map[string][]QueryIssue{
stmt1: []QueryIssue{
NewForeignKeyReferencesPartitionedTableIssue(TABLE_OBJECT_TYPE, "abc_fk", stmt1, "abc_fk_abc_id_fkey"),
},
stmt2: []QueryIssue{
NewForeignKeyReferencesPartitionedTableIssue(TABLE_OBJECT_TYPE, "schema1.abc_fk1", stmt2, "fk"),
},
stmt3: []QueryIssue{
NewForeignKeyReferencesPartitionedTableIssue(TABLE_OBJECT_TYPE, "abc_fk", stmt3, "fk_abc"),
},
stmt4: []QueryIssue{
NewForeignKeyReferencesPartitionedTableIssue(TABLE_OBJECT_TYPE, "schema1.abc_fk", stmt4, "abc_fk_abc_id_fkey"),
},
}
parserIssueDetector := NewParserIssueDetector()
for _, stmt := range requiredDDLs {
err := parserIssueDetector.ParseRequiredDDLs(stmt)
assert.NoError(t, err, "Error parsing required ddl: %s", stmt)
}
for stmt, expectedIssues := range ddlStmtsWithIssues {
issues, err := parserIssueDetector.GetDDLIssues(stmt, ybversion.LatestStable)
assert.NoError(t, err, "Error detecting issues for statement: %s", stmt)

assert.Equal(t, len(expectedIssues), len(issues), "Mismatch in issue count for statement: %s", stmt)
for _, expectedIssue := range expectedIssues {
found := slices.ContainsFunc(issues, func(queryIssue QueryIssue) bool {
return cmp.Equal(expectedIssue, queryIssue)
})
assert.True(t, found, "Expected issue not found: %v in statement: %s", expectedIssue, stmt)
}
}
}
Loading