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

priyanshi-yb · 2025-01-02T13:10:35Z

Describe the changes in this pull request

Fixes #1989

Describe if there are any user-facing changes

Added an Unsupported feature in assessment - Foreign key references partitioned table

How was this pull request tested?

Added End-to-End tests, 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

…onstraint name as well

sanyamsinghal · 2025-01-04T19:09:03Z

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

+				issues = append(issues, NewForeignKeyReferencesPartitionedTableIssue(
+					TABLE_OBJECT_TYPE,
+					table.GetObjectName(),
+					"",


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.

SQL statement is already added for these later in the GetDDLIssues

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.

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.

sanyamsinghal · 2025-01-04T19:10:12Z

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

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


Consider using the new testcontainers package in this test file.

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

sanyamsinghal · 2025-01-04T19:12:43Z

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

@@ -754,3 +754,56 @@ func TestCopyUnsupportedConstructIssuesDetected(t *testing.T) {
 		}
 	}
 }
+
+func TestForeignKeyReferencesPartitionedTableIssues(t *testing.T) {


can we remove any redundant test here.

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

sanyamsinghal · 2025-01-04T19:13:45Z

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

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


consider using protomsg approach here

This is the old code, so I haven't changed it. I am just using the already-written code in this case.

sanyamsinghal · 2025-01-04T19:15:29Z

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

@@ -354,6 +375,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)


nit: i think this operation of fetching qualified name is used at multiple places.
Consider writing a helper/util function for this.

sanyamsinghal · 2025-01-04T19:16:38Z

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

@@ -354,6 +375,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 {


why doing this in case of foreign key only. Can this be a unconditional step?

reference table is only present in case of foreign keys so using the field only in that case as it will be nil in other cases

sanyamsinghal · 2025-01-04T19:20:05Z

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

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


Few summary comments about table processor related code will help here.

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

sanyamsinghal

lgtm

sanyamsinghal · 2025-01-06T10:22:58Z

yb-voyager/src/utils/utils.go

@@ -740,3 +740,7 @@ func CheckTools(tools ...string) []string {

 	return missingTools
 }
+
+func GetObjectName(schemaName, objName string) string {


nit: BuildObjectName

priyanshi-yb added 7 commits January 2, 2025 18:34

Reporting Foreign key references partitioned table issue

b066843

added end-to-end tests in analyze and assessment

3c03d6c

fix undefineds

ef67431

fiz suggestion field

4c8267d

changed approach to use the DDL processor for this case as need the c…

7050196

…onstraint name as well

clean up

17c6c0a

comments

c69c102

priyanshi-yb requested review from makalaaneesh, sanyamsinghal and ShivanshGahlot and removed request for makalaaneesh and sanyamsinghal January 3, 2025 05:57

priyanshi-yb marked this pull request as ready for review January 3, 2025 05:57

sanyamsinghal reviewed Jan 4, 2025

View reviewed changes

review comments

c178884

priyanshi-yb requested a review from sanyamsinghal January 6, 2025 09:29

sanyamsinghal approved these changes Jan 6, 2025

View reviewed changes

priyanshi-yb added 2 commits January 6, 2025 16:47

nit

c6b0f80

Merge branch 'main' into priyanshi/fk-referenced-part-table

1c28fe1

priyanshi-yb merged commit 5a50ec5 into main Jan 6, 2025
67 checks passed

priyanshi-yb deleted the priyanshi/fk-referenced-part-table branch January 6, 2025 14:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

priyanshi-yb commented Jan 2, 2025 •

edited

Loading

sanyamsinghal Jan 4, 2025

priyanshi-yb Jan 6, 2025 •

edited

Loading

sanyamsinghal Jan 6, 2025

priyanshi-yb Jan 6, 2025

sanyamsinghal Jan 4, 2025

priyanshi-yb Jan 6, 2025

sanyamsinghal Jan 4, 2025

priyanshi-yb Jan 6, 2025

sanyamsinghal Jan 4, 2025

priyanshi-yb Jan 6, 2025

sanyamsinghal Jan 4, 2025

sanyamsinghal Jan 4, 2025

priyanshi-yb Jan 6, 2025

sanyamsinghal Jan 4, 2025

sanyamsinghal left a comment

sanyamsinghal Jan 6, 2025

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

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

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

Conversation

priyanshi-yb commented Jan 2, 2025 • 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

priyanshi-yb Jan 6, 2025 • 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanyamsinghal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

priyanshi-yb commented Jan 2, 2025 •

edited

Loading

priyanshi-yb Jan 6, 2025 •

edited

Loading