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.

Sign up for GitHub

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 the Advisory locks, XML Functions and System columns on the DDLs in the unsupported features in assess/analyze #2061

Merged
merged 13 commits into from
Dec 13, 2024
Merged

Reporting the Advisory locks, XML Functions and System columns on the DDLs in the unsupported features in assess/analyze #2061

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
20e5089
Reporting the Advisory locks, XML Functions and System columsn on the…
priyanshi-yb Dec 9, 2024
c127704
added reporting in assess-migration
priyanshi-yb Dec 9, 2024
8afaaea
Moved MVIEW/View select generic issues to the unsupported features
priyanshi-yb Dec 10, 2024
0baf78c
fix tests and add case for TABLE in analyze-schema
priyanshi-yb Dec 10, 2024
c528346
minor change
priyanshi-yb Dec 10, 2024
0a11460
Merge branch 'main' into priyanshi/report-xml-advisory-issues-on-ddl
priyanshi-yb Dec 10, 2024
cc9018f
Merge branch 'main' into priyanshi/report-xml-advisory-issues-on-ddl
priyanshi-yb Dec 11, 2024
9e0c265
update expected assessment report for pg complex schemas
priyanshi-yb Dec 11, 2024
1d73003
minor fix in the format of reporting constraint for a couple of cases
priyanshi-yb Dec 11, 2024
43f1853
added unit tests for GetDDLIssues()
priyanshi-yb Dec 11, 2024
ae5ba19
Merge branch 'main' into priyanshi/report-xml-advisory-issues-on-ddl
priyanshi-yb Dec 13, 2024
cea6410
review comments
priyanshi-yb Dec 13, 2024
1cd099d
review comment
priyanshi-yb Dec 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 23 additions & 7 deletions yb-voyager/src/query/queryissue/parser_issue_detector.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ func (p *ParserIssueDetector) getAllIssues(query string) ([]QueryIssue, error) {
if err != nil {
return nil, fmt.Errorf("error getting plpgsql issues: %v", err)
}
dmlIssues, err := p.genericIssues(query)
dmlIssues, err := p.getDMLIssues(query)
if err != nil {
return nil, fmt.Errorf("error getting generic issues: %v", err)
}
Expand Down Expand Up @@ -246,6 +246,13 @@ func (p *ParserIssueDetector) getDDLIssues(query string) ([]QueryIssue, error) {
if err != nil {
return nil, fmt.Errorf("error parsing a query: %v", err)
}
isDDL, err := queryparser.IsDDL(parseTree)
if err != nil {
return nil, fmt.Errorf("error checking if query is ddl: %w", err)
}
if !isDDL {
return nil, nil
}
// Parse the query into a DDL object
ddlObj, err := queryparser.ProcessDDL(parseTree)
if err != nil {
Expand All @@ -270,10 +277,11 @@ func (p *ParserIssueDetector) getDDLIssues(query string) ([]QueryIssue, error) {
}
}

if _, ok := ddlObj.(*queryparser.Object); ok { // In case the DDL doesn't have any processor skip checking generic issues
return issues, nil
}

/*
For detecting these generic issues (Advisory locks, XML functions and System columns as of now) on DDL example -
CREATE INDEX idx_invoices on invoices (xpath('/invoice/customer/text()', data));
We need to call it on DDLs as well
*/
genericIssues, err := p.genericIssues(query)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add a comment here explaining why we're making this call for DDLs. Give an example.

if err != nil {
return nil, fmt.Errorf("error getting generic issues: %w", err)
Expand Down Expand Up @@ -319,6 +327,15 @@ func (p *ParserIssueDetector) GetPercentTypeSyntaxIssues(query string) ([]QueryI
}

func (p *ParserIssueDetector) GetDMLIssues(query string, targetDbVersion *ybversion.YBVersion) ([]QueryIssue, error) {
issues, err := p.getDMLIssues(query)
if err != nil {
return issues, err
}

return p.getIssuesNotFixedInTargetDbVersion(issues, targetDbVersion)
}

func (p *ParserIssueDetector) getDMLIssues(query string) ([]QueryIssue, error) {
parseTree, err := queryparser.Parse(query)
if err != nil {
return nil, fmt.Errorf("error parsing query: %w", err)
Expand All @@ -335,8 +352,7 @@ func (p *ParserIssueDetector) GetDMLIssues(query string, targetDbVersion *ybvers
if err != nil {
return issues, err
}

return p.getIssuesNotFixedInTargetDbVersion(issues, targetDbVersion)
return issues, err
}

func (p *ParserIssueDetector) genericIssues(query string) ([]QueryIssue, error) {
Expand Down
50 changes: 28 additions & 22 deletions yb-voyager/src/query/queryissue/parser_issue_detector_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@ import (
"slices"
"testing"

"github.com/google/go-cmp/cmp"
"github.com/samber/lo"
"github.com/stretchr/testify/assert"

"github.com/yugabyte/yb-voyager/yb-voyager/src/ybversion"
Expand Down Expand Up @@ -154,6 +156,13 @@ CHECK (xpath_exists('/invoice/customer', data));`
stmt18 = `CREATE INDEX idx_invoices on invoices (xpath('/invoice/customer/text()', data));`
)

func modifyiedIssuesforPLPGSQL(issues []QueryIssue, objType string, objName string) []QueryIssue {
return lo.Map(issues, func(i QueryIssue, _ int) QueryIssue {
i.ObjectType = objType
i.ObjectName = objName
return i
})
}
func TestAllIssues(t *testing.T) {
requiredDDLs := []string{stmt12}
parserIssueDetector := NewParserIssueDetector()
Expand All @@ -162,19 +171,19 @@ func TestAllIssues(t *testing.T) {
NewPercentTypeSyntaxIssue("FUNCTION", "list_high_earners", "public.emp1.salary%TYPE"),
NewPercentTypeSyntaxIssue("FUNCTION", "list_high_earners", "employees.name%TYPE"),
NewPercentTypeSyntaxIssue("FUNCTION", "list_high_earners", "employees.salary%TYPE"),
NewClusterONIssue("FUNCTION", "list_high_earners", "ALTER TABLE employees CLUSTER ON idx;"),
NewAdvisoryLocksIssue("FUNCTION", "list_high_earners", "SELECT pg_advisory_unlock(sender_id);"),
NewAdvisoryLocksIssue("FUNCTION", "list_high_earners", "SELECT pg_advisory_unlock(receiver_id);"),
NewXmlFunctionsIssue("FUNCTION", "list_high_earners", "SELECT id, xpath('/person/name/text()', data) AS name FROM test_xml_type;"),
NewSystemColumnsIssue("FUNCTION", "list_high_earners", "SELECT * FROM employees e WHERE e.xmax = (SELECT MAX(xmax) FROM employees WHERE department = e.department);"),
NewClusterONIssue("TABLE", "employees", "ALTER TABLE employees CLUSTER ON idx;"),
NewAdvisoryLocksIssue("DML_QUERY", "", "SELECT pg_advisory_unlock(sender_id);"),
NewAdvisoryLocksIssue("DML_QUERY", "", "SELECT pg_advisory_unlock(receiver_id);"),
NewXmlFunctionsIssue("DML_QUERY", "", "SELECT id, xpath('/person/name/text()', data) AS name FROM test_xml_type;"),
NewSystemColumnsIssue("DML_QUERY", "", "SELECT * FROM employees e WHERE e.xmax = (SELECT MAX(xmax) FROM employees WHERE department = e.department);"),
},
stmt2: []QueryIssue{
NewPercentTypeSyntaxIssue("FUNCTION", "process_order", "orders.id%TYPE"),
NewStorageParameterIssue("FUNCTION", "process_order", "ALTER TABLE ONLY public.example ADD CONSTRAINT example_email_key UNIQUE (email) WITH (fillfactor=70);"),
NewUnloggedTableIssue("FUNCTION", "process_order", "CREATE UNLOGGED TABLE tbl_unlog (id int, val text);"),
NewMultiColumnGinIndexIssue("FUNCTION", "process_order", "CREATE INDEX idx_example ON example_table USING gin(name, name1);"),
NewUnsupportedIndexMethodIssue("FUNCTION", "process_order", "CREATE INDEX idx_example ON schema1.example_table USING gist(name);", "gist"),
NewAdvisoryLocksIssue("FUNCTION", "process_order", "SELECT pg_advisory_unlock(orderid);"),
NewStorageParameterIssue("TABLE", "public.example", "ALTER TABLE ONLY public.example ADD CONSTRAINT example_email_key UNIQUE (email) WITH (fillfactor=70);"),
NewUnloggedTableIssue("TABLE", "tbl_unlog", "CREATE UNLOGGED TABLE tbl_unlog (id int, val text);"),
NewMultiColumnGinIndexIssue("INDEX", "idx_example ON example_table", "CREATE INDEX idx_example ON example_table USING gin(name, name1);"),
NewUnsupportedIndexMethodIssue("INDEX", "idx_example ON schema1.example_table", "CREATE INDEX idx_example ON schema1.example_table USING gist(name);", "gist"),
NewAdvisoryLocksIssue("DML_QUERY", "", "SELECT pg_advisory_unlock(orderid);"),
},
stmt3: []QueryIssue{
NewStorageParameterIssue("INDEX", "abc ON public.example", stmt3),
Expand Down Expand Up @@ -210,6 +219,11 @@ func TestAllIssues(t *testing.T) {
},
}

//Should modify it in similar way we do it actual code as the particular DDL issue in plpgsql can have different Details map on the basis of objectType
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm confused. What was wrong with the previous approach where you define issues directly with "FUNCTION", "list_high_earners" ?

Copy link
Contributor Author

@priyanshi-yb priyanshi-yb Dec 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

example for this cluster on -

func NewClusterONIssue(objectType string, objectName string, sqlStatement string) QueryIssue {
	details := map[string]interface{}{}
	//for ALTER AND INDEX both  same struct now how to differentiate which one to not
	if objectType == "TABLE" {
		details["INCREASE_INVALID_COUNT"] = false
	}
	return newQueryIssue(clusterOnIssue, objectType, objectName, sqlStatement, details)
}

earlier I was using the New function for getting queryissue -

NewClusterONIssue("FUNCTION", "list_high_earners", "ALTER TABLE employees CLUSTER ON idx;")

giving this QueryIssue, with details as empty as -

{{CLUSTER_ON ALTER TABLE CLUSTER not supported yet.  Remove it from the exported schema. https://github.com/YugaByte/yugabyte-db/issues/1124 https://docs.yugabyte.com/preview/yugabyte-voyager/known-issues/postgresql/#unsupported-alter-table-ddl-variants-in-source-schema map[]} FUNCTION list_high_earners ALTER TABLE employees CLUSTER ON idx; map[]}

But the actual query issue will look like this where details will be populated based on TABLE object type-

{{CLUSTER_ON ALTER TABLE CLUSTER not supported yet.  Remove it from the exported schema. https://github.com/YugaByte/yugabyte-db/issues/1124 https://docs.yugabyte.com/preview/yugabyte-voyager/known-issues/postgresql/#unsupported-alter-table-ddl-variants-in-source-schema map[]} FUNCTION list_high_earners ALTER TABLE employees CLUSTER ON idx; map[INCREASE_INVALID_COUNT:false]}

In the code to detect these issues in PLPGSQL, we modify the issues later (in getAllPLPGSQLIssues()) to change the objectType and objectName to the actual object name and type of the PLPGSQL object.
So now with cmp.Equal, it also compares details map, I had to change the way I generate the expected issues to be able to check properly.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Orthogonal point:
INCREASE_INVALID_COUNT should not be in queryissue layer.
It is an analyze-schema detail, we should have some logic of figuring it out at that layer.

Copy link
Contributor Author

@priyanshi-yb priyanshi-yb Dec 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah, I agree. This was just a hack while refactoring. I already removed it in this PR with handling all cases for invalid count #2073

Copy link
Collaborator

Choose a reason for hiding this comment

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

oh nice, will check!

stmtsWithExpectedIssues[stmt1] = modifyiedIssuesforPLPGSQL(stmtsWithExpectedIssues[stmt1], "FUNCTION", "list_high_earners")

stmtsWithExpectedIssues[stmt2] = modifyiedIssuesforPLPGSQL(stmtsWithExpectedIssues[stmt2], "FUNCTION", "process_order")

for _, stmt := range requiredDDLs {
err := parserIssueDetector.ParseRequiredDDLs(stmt)
assert.NoError(t, err, "Error parsing required ddl: %s", stmt)
Expand All @@ -220,12 +234,8 @@ func TestAllIssues(t *testing.T) {

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 {
typeNameMatches := QueryIssue.TypeName == expectedIssue.TypeName
queryMatches := QueryIssue.SqlStatement == expectedIssue.SqlStatement
objectNameMatches := QueryIssue.ObjectName == expectedIssue.ObjectName
objectTypeMatches := QueryIssue.ObjectType == expectedIssue.ObjectType
return typeNameMatches && queryMatches && objectNameMatches && objectTypeMatches
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)
}
Expand Down Expand Up @@ -272,12 +282,8 @@ func TestDDLIssues(t *testing.T) {

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 {
typeNameMatches := QueryIssue.TypeName == expectedIssue.TypeName
queryMatches := QueryIssue.SqlStatement == expectedIssue.SqlStatement
objectNameMatches := QueryIssue.ObjectName == expectedIssue.ObjectName
objectTypeMatches := QueryIssue.ObjectType == expectedIssue.ObjectType
return typeNameMatches && queryMatches && objectNameMatches && objectTypeMatches
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)
}
Expand Down