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

priyanshi-yb · 2024-12-10T13:16:29Z

Describe the changes in this pull request

Added three more features to Unsupported features in the reports Advisory locks, XML functions and System columns in the DDL.
Moved the MVIEW/VIEW issues from Unsupported PL/pgSQL objects to Unsupported Features.
Minor format change of constraint reporting for PK/Unique on complex datatypes issue
Added unit tests for GetDDLIssues() in the parser_issue_detector_test.go

Fixes #2025

Sample Assessment report -

Sample Analyze report -

Describe if there are any user-facing changes

Added new features section under Unsupported Features

How was this pull request tested?

Existing tests were enough.
Added unit tests

Does your PR have changes that can cause upgrade issues?

Component	Breaking changes?
MetaDB	No
Name registry json	No
Data File Descriptor Json	No
Export Snapshot Status Json	No
Import Data State	No
Export Status Json	No
Data .sql files of tables	No
Export and import data queue	No
Schema Dump	No
AssessmentDB	No
Sizing DB	No
Migration Assessment Report Json	No
Callhome Json	No
YugabyteD Tables	No
TargetDB Metadata Tables	No

… DDLs in the unsupported_features in analyze

makalaaneesh · 2024-12-13T05:27:07Z

yb-voyager/src/query/queryissue/detectors_ddl.go

@@ -50,7 +50,7 @@ func (d *TableIssueDetector) DetectIssues(obj queryparser.DDLObject) ([]QueryIss
 	// Check for generated columns
 	if len(table.GeneratedColumns) > 0 {
 		issues = append(issues, NewGeneratedColumnsIssue(
-			TABLE_OBJECT_TYPE,
+			obj.GetObjectType(),


a TableIssueDetector will always return issues for Table object types, right?
Is this just a change to not use a constant and use the function, or is there a certain bug/case because of which you had to do this?

Is this just a change to not use a constant and use the function,

Yes right, that way I was able to directly use ddlObj.GetObjectType() to get the respective type.

makalaaneesh · 2024-12-13T05:28:22Z

yb-voyager/src/query/queryissue/parser_issue_detector.go

+		return issues, nil
+	}
+
+	genericIssues, err := p.genericIssues(query)


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

makalaaneesh · 2024-12-13T05:30:27Z

yb-voyager/src/query/queryissue/parser_issue_detector.go

@@ -289,6 +269,22 @@ func (p *ParserIssueDetector) getDDLIssues(query string) ([]QueryIssue, error) {
 			issues[i].SqlStatement = query
 		}
 	}
+
+	if _, ok := ddlObj.(*queryparser.Object); ok { // In case the DDL doesn't have any processor skip checking generic issues


a ddlobject is an interface that already adheres to GetObjectName and GetSchemaName. So why do we need to additionally check if you can type cast it to a queryparser.Object. As long as ddlObject is of type DDLObject interface, you should be good, no?

So there were reasons to add this check here-

To handle the scenario where the stmt coming to getDDLIssues is DML stmt which can be in case of PLPGQueries where we call internal getAllIssues(), so with that generic function is getting called for the select stmt twice giving the duplicate issues. - This can be solved by just unique issues by some Key or anything

To handle the scenario where the ObjectType is not known in this code path (the case where DDLObject is not implemented for some DDL types yet) so it might return. the objectType - OBJECT (NoOpProcessor). For such case, the analyze code to convertIssue toAnalyzeIssue() where we set invalidCount of objectname based on ObjectTYpe will fail with nil pointer as it doesn't know about it. - This can also be solved by handling it properly in that function.

As discussed, changed this condition with isDDL in the starting of getDDLIssues()

makalaaneesh · 2024-12-13T05:32:49Z

yb-voyager/src/query/queryissue/parser_issue_detector.go

-}
-
-// TODO: in future when we will DDL issues detection here we need `GetDDLIssues`
-func (p *ParserIssueDetector) getDMLIssues(query string) ([]QueryIssue, error) {


would recommend still keeping the getDMLIssues and GetDMLIssues both. The idea was to not touch the GetDMLIssues often and keep the getIssuesNotFixedInTargetDbVersion in it so that it does not get missed.

If we have just one function, there is a high possibility that we accidentally just write some return from the middle of that function which will lead to issues not being filtered out..

makalaaneesh · 2024-12-13T05:33:17Z

yb-voyager/src/query/queryissue/parser_issue_detector_test.go

+
+		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 {


As discussed, see if we can use cmp.Equal for this.

makalaaneesh · 2024-12-13T05:34:37Z

yb-voyager/src/query/queryparser/helpers_struct.go

@@ -58,22 +39,6 @@ func IsMviewObject(parseTree *pg_query.ParseResult) bool {
 	return isCreateAsStmt && createAsNode.CreateTableAsStmt.Objtype == pg_query.ObjectType_OBJECT_MATVIEW
 }

-func GetSelectStmtQueryFromViewOrMView(parseTree *pg_query.ParseResult) (string, error) {


Ah, so we don't really need these anymore because parser can directly parse MVIEW/VIEW, correct?

makalaaneesh · 2024-12-13T06:27:03Z

@priyanshi-yb It would be good to also include details for those issues in the report.

advisory locks: which functions were used?
XML functions - which functions?
system columns - which system columns.

I believe these details are not available right now, am i right?

priyanshi-yb · 2024-12-13T07:21:25Z

I believe these details are not available right now, am i right?

Yes, currently it is not available we might have to enhance the traversal logic to return all the variety of unsupported functions/columns.. we found in the statement

makalaaneesh · 2024-12-13T09:18:24Z

yb-voyager/src/query/queryissue/parser_issue_detector_test.go

@@ -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


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

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.

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.

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

oh nice, will check!

makalaaneesh

LGTM

priyanshi-yb added 7 commits December 9, 2024 22:10

Reporting the Advisory locks, XML Functions and System columsn on the…

20e5089

… DDLs in the unsupported_features in analyze

added reporting in assess-migration

c127704

Moved MVIEW/View select generic issues to the unsupported features

8afaaea

fix tests and add case for TABLE in analyze-schema

0baf78c

minor change

c528346

Merge branch 'main' into priyanshi/report-xml-advisory-issues-on-ddl

0a11460

Merge branch 'main' into priyanshi/report-xml-advisory-issues-on-ddl

cc9018f

priyanshi-yb requested a review from makalaaneesh December 11, 2024 10:54

priyanshi-yb marked this pull request as ready for review December 11, 2024 10:54

priyanshi-yb added 3 commits December 11, 2024 16:48

update expected assessment report for pg complex schemas

9e0c265

minor fix in the format of reporting constraint for a couple of cases

1d73003

added unit tests for GetDDLIssues()

43f1853

priyanshi-yb force-pushed the priyanshi/report-xml-advisory-issues-on-ddl branch from 87dbbac to 43f1853 Compare December 11, 2024 12:19

makalaaneesh reviewed Dec 13, 2024

View reviewed changes

Merge branch 'main' into priyanshi/report-xml-advisory-issues-on-ddl

ae5ba19

priyanshi-yb added 2 commits December 13, 2024 13:44

review comments

cea6410

review comment

1cd099d

priyanshi-yb requested a review from makalaaneesh December 13, 2024 09:07

makalaaneesh reviewed Dec 13, 2024

View reviewed changes

makalaaneesh approved these changes Dec 13, 2024

View reviewed changes

priyanshi-yb merged commit 13a678b into main Dec 13, 2024
42 checks passed

priyanshi-yb deleted the priyanshi/report-xml-advisory-issues-on-ddl branch December 13, 2024 16:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

priyanshi-yb commented Dec 10, 2024 •

edited

Loading

makalaaneesh Dec 13, 2024

priyanshi-yb Dec 13, 2024

makalaaneesh Dec 13, 2024

makalaaneesh Dec 13, 2024

priyanshi-yb Dec 13, 2024 •

edited

Loading

priyanshi-yb Dec 13, 2024 •

edited

Loading

makalaaneesh Dec 13, 2024

makalaaneesh Dec 13, 2024

makalaaneesh Dec 13, 2024

priyanshi-yb Dec 13, 2024

makalaaneesh commented Dec 13, 2024

priyanshi-yb commented Dec 13, 2024 •

edited

Loading

makalaaneesh Dec 13, 2024

priyanshi-yb Dec 13, 2024 •

edited

Loading

makalaaneesh Dec 13, 2024

priyanshi-yb Dec 13, 2024 •

edited

Loading

makalaaneesh Dec 13, 2024

makalaaneesh left a comment

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

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

Conversation

priyanshi-yb commented Dec 10, 2024 • edited Loading

Describe the changes in this pull request

Describe if there are any user-facing changes

How was this pull request tested?

Does your PR have changes that can cause upgrade issues?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

priyanshi-yb Dec 13, 2024 • edited Loading

Choose a reason for hiding this comment

priyanshi-yb Dec 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

makalaaneesh commented Dec 13, 2024

priyanshi-yb commented Dec 13, 2024 • edited Loading

Choose a reason for hiding this comment

priyanshi-yb Dec 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

priyanshi-yb Dec 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

makalaaneesh left a comment

Choose a reason for hiding this comment

priyanshi-yb commented Dec 10, 2024 •

edited

Loading

priyanshi-yb Dec 13, 2024 •

edited

Loading

priyanshi-yb Dec 13, 2024 •

edited

Loading

priyanshi-yb commented Dec 13, 2024 •

edited

Loading

priyanshi-yb Dec 13, 2024 •

edited

Loading

priyanshi-yb Dec 13, 2024 •

edited

Loading