Skip to content

Commit

Permalink
changed approach to use the DDL processor for this case as need the c…
Browse files Browse the repository at this point in the history
…onstraint name as well
  • Loading branch information
priyanshi-yb committed Jan 2, 2025
1 parent 4c8267d commit 7050196
Show file tree
Hide file tree
Showing 10 changed files with 138 additions and 93 deletions.
2 changes: 1 addition & 1 deletion migtests/tests/analyze-schema/expected_issues.json
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@
{
"IssueType": "unsupported_features",
"ObjectType": "TABLE",
"ObjectName": "test_1",
"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 ",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -638,7 +638,7 @@
"FeatureName": "Foreign key constraint references partitioned table",
"Objects": [
{
"ObjectName": "public.test_jsonb",
"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);"
}
],
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
89 changes: 44 additions & 45 deletions yb-voyager/src/query/queryissue/detectors.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ import (
"slices"

mapset "github.com/deckarep/golang-set/v2"
"github.com/samber/lo"
log "github.com/sirupsen/logrus"
"google.golang.org/protobuf/reflect/protoreflect"

Expand Down Expand Up @@ -392,47 +391,47 @@ func (d *JsonQueryFunctionDetector) GetIssues() []QueryIssue {
return issues
}

type ConstraintIssuesDetector struct {
query string
partitionTablesMap map[string]bool
isForeignKeyReferencesPartitionTableDetected bool
}

func NewConstraintIssuesDetector(query string, partitionTablesMap map[string]bool) *ConstraintIssuesDetector {
return &ConstraintIssuesDetector{
query: query,
partitionTablesMap: partitionTablesMap,
}
}

func (c *ConstraintIssuesDetector) Detect(msg protoreflect.Message) error {
if queryparser.GetMsgFullName(msg) != queryparser.PG_QUERY_CONSTRAINT_NODE {
return nil
}

constraintNode, err := queryparser.GetConstraintNode(msg)
if err != nil {
return err
}

switch constraintNode.Contype {
case queryparser.FOREIGN_CONSTR_TYPE:
referencedTable := constraintNode.Pktable
//constraints:{constraint:{contype:CONSTR_FOREIGN initially_valid:true pktable:{relname:"abc1" inh:true relpersistence:"p" location:121}
referencedTableObjName := lo.Ternary(referencedTable.Schemaname != "", referencedTable.Schemaname+"."+referencedTable.Relname, referencedTable.Relname)
if c.partitionTablesMap[referencedTableObjName] {
//If reference table is Partitioned table
c.isForeignKeyReferencesPartitionTableDetected = true
}
}

return nil
}

func (c *ConstraintIssuesDetector) GetIssues() []QueryIssue {
issues := make([]QueryIssue, 0)
if c.isForeignKeyReferencesPartitionTableDetected {
issues = append(issues, NewForeignKeyReferencesPartitionedTableIssue(DML_QUERY_OBJECT_TYPE, "", c.query))
}
return issues
}
// type ConstraintIssuesDetector struct {
// query string
// partitionTablesMap map[string]bool
// isForeignKeyReferencesPartitionTableDetected bool
// }

// func NewConstraintIssuesDetector(query string, partitionTablesMap map[string]bool) *ConstraintIssuesDetector {
// return &ConstraintIssuesDetector{
// query: query,
// partitionTablesMap: partitionTablesMap,
// }
// }

// func (c *ConstraintIssuesDetector) Detect(msg protoreflect.Message) error {
// if queryparser.GetMsgFullName(msg) != queryparser.PG_QUERY_CONSTRAINT_NODE {
// return nil
// }

// constraintNode, err := queryparser.GetConstraintNode(msg)
// if err != nil {
// return err
// }

// switch constraintNode.Contype {
// case queryparser.FOREIGN_CONSTR_TYPE:
// referencedTable := constraintNode.Pktable
// //constraints:{constraint:{contype:CONSTR_FOREIGN initially_valid:true pktable:{relname:"abc1" inh:true relpersistence:"p" location:121}
// referencedTableObjName := lo.Ternary(referencedTable.Schemaname != "", referencedTable.Schemaname+"."+referencedTable.Relname, referencedTable.Relname)
// if c.partitionTablesMap[referencedTableObjName] {
// //If reference table is Partitioned table
// c.isForeignKeyReferencesPartitionTableDetected = true
// }
// }

// return nil
// }

// func (c *ConstraintIssuesDetector) GetIssues() []QueryIssue {
// issues := make([]QueryIssue, 0)
// if c.isForeignKeyReferencesPartitionTableDetected {
// // issues = append(issues, NewForeignKeyReferencesPartitionedTableIssue(DML_QUERY_OBJECT_TYPE, "", c.query))
// }
// return issues
// }
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(),
"",
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
18 changes: 9 additions & 9 deletions yb-voyager/src/query/queryissue/detectors_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -711,12 +711,12 @@ WHERE JSON_EXISTS(details, '$.price ? (@ > $price)' PASSING 30 AS price);`

}

func TestConstraintIssuesDetector(t *testing.T) {
sql := `CREATE TABLE abc_fk(id int PRIMARY KEY, abc_id INT REFERENCES public.abc(id), val text) ;`

issues := getDetectorIssues(t, NewConstraintIssuesDetector(sql, map[string]bool{
"public.abc": true,
}), sql)
assert.Equal(t, 1, len(issues), "Expected 1 issue for SQL: %s", sql)
assert.Equal(t, FOREIGN_KEY_REFERENCES_PARTITIONED_TABLE, issues[0].Type, "Expected Advisory Locks issue for SQL: %s", sql)
}
// func TestConstraintIssuesDetector(t *testing.T) {
// sql := `CREATE TABLE abc_fk(id int PRIMARY KEY, abc_id INT REFERENCES public.abc(id), val text) ;`

// issues := getDetectorIssues(t, NewConstraintIssuesDetector(sql, map[string]bool{
// "public.abc": true,
// }), sql)
// assert.Equal(t, 1, len(issues), "Expected 1 issue for SQL: %s", sql)
// assert.Equal(t, FOREIGN_KEY_REFERENCES_PARTITIONED_TABLE, issues[0].Type, "Expected Advisory Locks issue for SQL: %s", sql)
// }
7 changes: 5 additions & 2 deletions yb-voyager/src/query/queryissue/issues_ddl.go
Original file line number Diff line number Diff line change
Expand Up @@ -474,6 +474,9 @@ var foreignKeyReferencesPartitionedTableIssue = issue.Issue{
DocsLink: "", // TODO
}

func NewForeignKeyReferencesPartitionedTableIssue(objectType string, objectName string, SqlStatement string) QueryIssue {
return newQueryIssue(foreignKeyReferencesPartitionedTableIssue, objectType, objectName, SqlStatement, map[string]interface{}{})
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)
}
2 changes: 1 addition & 1 deletion yb-voyager/src/query/queryissue/parser_issue_detector.go
Original file line number Diff line number Diff line change
Expand Up @@ -379,7 +379,7 @@ func (p *ParserIssueDetector) genericIssues(query string) ([]QueryIssue, error)
NewCopyCommandUnsupportedConstructsDetector(query),
NewJsonConstructorFuncDetector(query),
NewJsonQueryFunctionDetector(query),
NewConstraintIssuesDetector(query, p.partitionTablesMap),
// NewConstraintIssuesDetector(query, p.partitionTablesMap),
}

processor := func(msg protoreflect.Message) error {
Expand Down
9 changes: 5 additions & 4 deletions yb-voyager/src/query/queryissue/parser_issue_detector_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -777,16 +777,16 @@ REFERENCES schema1.abc (id);

ddlStmtsWithIssues := map[string][]QueryIssue{
stmt1: []QueryIssue{
NewForeignKeyReferencesPartitionedTableIssue(TABLE_OBJECT_TYPE, "abc_fk", stmt1),
NewForeignKeyReferencesPartitionedTableIssue(TABLE_OBJECT_TYPE, "abc_fk", stmt1, "abc_fk_abc_id_fkey"),
},
stmt2: []QueryIssue{
NewForeignKeyReferencesPartitionedTableIssue(TABLE_OBJECT_TYPE, "schema1.abc_fk1", stmt2),
NewForeignKeyReferencesPartitionedTableIssue(TABLE_OBJECT_TYPE, "schema1.abc_fk1", stmt2, "fk"),
},
stmt3: []QueryIssue{
NewForeignKeyReferencesPartitionedTableIssue(TABLE_OBJECT_TYPE, "abc_fk", stmt3),
NewForeignKeyReferencesPartitionedTableIssue(TABLE_OBJECT_TYPE, "abc_fk", stmt3, "fk_abc"),
},
stmt4: []QueryIssue{
NewForeignKeyReferencesPartitionedTableIssue(TABLE_OBJECT_TYPE, "schema1.abc_fk", stmt4),
NewForeignKeyReferencesPartitionedTableIssue(TABLE_OBJECT_TYPE, "schema1.abc_fk", stmt4, "abc_fk_abc_id_fkey"),
},
}
parserIssueDetector := NewParserIssueDetector()
Expand All @@ -796,6 +796,7 @@ REFERENCES schema1.abc (id);
}
for stmt, expectedIssues := range ddlStmtsWithIssues {
issues, err := parserIssueDetector.GetDDLIssues(stmt, ybversion.LatestStable)
fmt.Printf("%v", issues)
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)
Expand Down
81 changes: 51 additions & 30 deletions yb-voyager/src/query/queryparser/ddl_processor.go
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,28 @@ func (tableProcessor *TableProcessor) Process(parseTree *pg_query.ParseResult) (
}

// Parse columns and their properties
for _, element := range createTableNode.CreateStmt.TableElts {
tableProcessor.parseTableElts(createTableNode.CreateStmt.TableElts, table)

if table.IsPartitioned {

partitionElements := createTableNode.CreateStmt.GetPartspec().GetPartParams()
table.PartitionStrategy = createTableNode.CreateStmt.GetPartspec().GetStrategy()

for _, partElem := range partitionElements {
if partElem.GetPartitionElem().GetExpr() != nil {
table.IsExpressionPartition = true
} else {
table.PartitionColumns = append(table.PartitionColumns, partElem.GetPartitionElem().GetName())
}
}
}

return table, nil
}

func (tableProcessor *TableProcessor) parseTableElts(tableElts []*pg_query.Node, table *Table) {

for _, element := range tableElts {
if element.GetColumnDef() != nil {
if tableProcessor.isGeneratedColumn(element.GetColumnDef()) {
table.GeneratedColumns = append(table.GeneratedColumns, element.GetColumnDef().Colname)
Expand Down Expand Up @@ -172,7 +193,7 @@ func (tableProcessor *TableProcessor) Process(parseTree *pg_query.ParseResult) (
table.Constraints[len(table.Constraints)-1] = lastConstraint
}
} else {
table.addConstraint(constraint.Contype, []string{colName}, constraint.Conname, false)
table.addConstraint(constraint.Contype, []string{colName}, constraint.Conname, false, constraint.Pktable)
}
}
}
Expand All @@ -190,33 +211,25 @@ func (tableProcessor *TableProcessor) Process(parseTree *pg_query.ParseResult) (
constraint := element.GetConstraint()
conType := element.GetConstraint().Contype
columns := parseColumnsFromKeys(constraint.GetKeys())
if conType == EXCLUSION_CONSTR_TYPE {
switch conType {
case EXCLUSION_CONSTR_TYPE:
//In case CREATE DDL has EXCLUDE USING gist(room_id '=', time_range WITH &&) - it will be included in columns but won't have columnDef as its a constraint
exclusions := constraint.GetExclusions()
//exclusions:{list:{items:{index_elem:{name:"room_id" ordering:SORTBY_DEFAULT nulls_ordering:SORTBY_NULLS_DEFAULT}}
//items:{list:{items:{string:{sval:"="}}}}}}
columns = tableProcessor.parseColumnsFromExclusions(exclusions)
case FOREIGN_CONSTR_TYPE:
// In case of Foreign key constraint if it is present at the end of table column definition
fkAttrs := constraint.FkAttrs
//CREATE TABLE schema1.abc_fk(id int, abc_id INT, val text, PRIMARY KEY(id), FOREIGN KEY (abc_id) REFERENCES schema1.abc(id));
//table_elts:{constraint:{contype:CONSTR_FOREIGN initially_valid:true pktable:{schemaname:"schema1" relname:"abc" inh:true relpersistence:"p" location:109}
//fk_attrs:{string:{sval:"abc_id"}} pk_attrs:{string:{sval:"id"}}
columns = parseColumnsFromKeys(fkAttrs)
}
table.addConstraint(conType, columns, constraint.Conname, constraint.Deferrable)
table.addConstraint(conType, columns, constraint.Conname, constraint.Deferrable, constraint.Pktable)

}
}

if table.IsPartitioned {

partitionElements := createTableNode.CreateStmt.GetPartspec().GetPartParams()
table.PartitionStrategy = createTableNode.CreateStmt.GetPartspec().GetStrategy()

for _, partElem := range partitionElements {
if partElem.GetPartitionElem().GetExpr() != nil {
table.IsExpressionPartition = true
} else {
table.PartitionColumns = append(table.PartitionColumns, partElem.GetPartitionElem().GetName())
}
}
}

return table, nil
}

func (tableProcessor *TableProcessor) checkInheritance(createTableNode *pg_query.Node_CreateStmt) bool {
Expand Down Expand Up @@ -290,10 +303,11 @@ func (tc *TableColumn) GetFullTypeName() string {
}

type TableConstraint struct {
ConstraintType pg_query.ConstrType
ConstraintName string
IsDeferrable bool
Columns []string
ConstraintType pg_query.ConstrType
ConstraintName string
IsDeferrable bool
ReferencedTable string
Columns []string
}

func (c *TableConstraint) IsPrimaryKeyORUniqueConstraint() bool {
Expand Down Expand Up @@ -345,7 +359,7 @@ func (t *Table) UniqueKeyColumns() []string {
return uniqueCols
}

func (t *Table) addConstraint(conType pg_query.ConstrType, columns []string, specifiedConName string, deferrable bool) {
func (t *Table) addConstraint(conType pg_query.ConstrType, columns []string, specifiedConName string, deferrable bool, referencedTable *pg_query.RangeVar) {
tc := TableConstraint{
ConstraintType: conType,
Columns: columns,
Expand All @@ -354,6 +368,9 @@ func (t *Table) addConstraint(conType pg_query.ConstrType, columns []string, spe
generatedConName := tc.generateConstraintName(t.TableName)
conName := lo.Ternary(specifiedConName == "", generatedConName, specifiedConName)
tc.ConstraintName = conName
if conType == FOREIGN_CONSTR_TYPE {
tc.ReferencedTable = lo.Ternary(referencedTable.Schemaname != "", referencedTable.Schemaname+"."+referencedTable.Relname, referencedTable.Relname)
}
t.Constraints = append(t.Constraints, tc)
}

Expand Down Expand Up @@ -575,6 +592,9 @@ func (atProcessor *AlterTableProcessor) Process(parseTree *pg_query.ParseResult)
alter.IsDeferrable = constraint.Deferrable
alter.ConstraintNotValid = constraint.SkipValidation // this is set for the NOT VALID clause
alter.ConstraintColumns = parseColumnsFromKeys(constraint.GetKeys())
if alter.ConstraintType == FOREIGN_CONSTR_TYPE {
alter.ConstraintReferencedTable = lo.Ternary(constraint.Pktable.Schemaname != "", constraint.Pktable.Schemaname+"."+constraint.Pktable.Relname, constraint.Pktable.Relname)
}

case pg_query.AlterTableType_AT_DisableRule:
/*
Expand Down Expand Up @@ -604,11 +624,12 @@ type AlterTable struct {
NumSetAttributes int
NumStorageOptions int
//In case AlterType - ADD_CONSTRAINT
ConstraintType pg_query.ConstrType
ConstraintName string
ConstraintNotValid bool
IsDeferrable bool
ConstraintColumns []string
ConstraintType pg_query.ConstrType
ConstraintName string
ConstraintNotValid bool
ConstraintReferencedTable string
IsDeferrable bool
ConstraintColumns []string
}

func (a *AlterTable) GetObjectName() string {
Expand Down

0 comments on commit 7050196

Please sign in to comment.