Create NOT VALID constraints only in post-snapshot-import mode with i…

…mport-schema (#2036)

Fixing an issue where the presence of NOT VALID constraints in the schema can constraint violation errors in import-data. https://yugabyte.atlassian.net/browse/DB-13881
Fix: Creating the NOT VALID constraints during post-snapshot-import mode when the data is already loaded which will not lead to any constraint violation errors.
Added tests in PG and Oracle's constraints test.

migtests/tests/oracle/constraints/constraints_schema_data.sql

@@ -41,17 +41,18 @@ CREATE TABLE check_test (
     ID int GENERATED BY DEFAULT AS IDENTITY,
     first_name varchar(255) NOT NULL,
     last_name varchar(255),
+    middle_name varchar(255) NOT NULL,
     Age int,
     CHECK (Age>=18)
 );
-insert into check_test (first_name, last_name, age) values ('Modestine', 'MacMeeking', 20);
-insert into check_test (first_name, last_name, age) values ('Genna', 'Kaysor', 50);
-insert into check_test (first_name, last_name, age) values ('Tess', 'Wesker', 56);
-insert into check_test (first_name, last_name, age) values ('Magnum', 'Danzelman', 89);
-insert into check_test (first_name, last_name, age) values ('Mitzi', 'Pidwell', 34);
-insert into check_test (first_name, last_name, age) values ('Milzie', 'Rohlfing', 70);
-
-
+insert into check_test (first_name, middle_name, last_name, age) values ('Modestine', 'null', 'MacMeeking', 20);
+insert into check_test (first_name, middle_name, last_name, age) values ('Genna', 'null', 'Kaysor', 50);
+insert into check_test (first_name, middle_name, last_name, age) values ('Tess', 'null', 'Wesker', 56);
+insert into check_test (first_name, middle_name, last_name, age) values ('Magnum', 'null', 'Danzelman', 89);
+insert into check_test (first_name, middle_name, last_name, age) values ('Mitzi', 'null', 'Pidwell', 34);
+insert into check_test (first_name, middle_name, last_name, age) values ('Milzie', 'null', 'Rohlfing', 70);
+
+ALTER TABLE check_test ADD CONSTRAINT novalid_con CHECK(middle_name<>'null') NOVALIDATE;
 
 drop table default_test;
 

migtests/tests/oracle/constraints/validate

@@ -30,7 +30,11 @@ QUERIES_CHECK = {
 		'code': "23505"
 	},
 	'CHECK_CONDITION': {
-		'query': "insert into public.check_test (id, first_name, last_name, age) values (7, 'Tom', 'Stewan', 15);",
+		'query': "insert into public.check_test (id, first_name, middle_name, last_name, age) values (7, 'Tom', 'gfh', 'Stewan', 15);",
+		'code': "23514"
+	},
+	'CHECK_CONDITION_NOT_VALID': {
+		'query': "insert into public.check_test (id, first_name, middle_name, last_name, age) values (7, 'Tom', 'null', 'Stewan', 25);",
 		'code': "23514"
 	},
 	'FORIEGN_CHECK': {

migtests/tests/pg/constraints/pg_constraints_automation.sql

@@ -39,17 +39,19 @@ drop table if exists check_test;
 CREATE TABLE check_test (
     ID serial primary key,
     first_name varchar(255) NOT NULL,
+    middle_name varchar(255) not null,
     last_name varchar(255),
     Age int,
     CHECK (Age>=18)
 );
-insert into check_test (first_name, last_name, age) values ('Modestine', 'MacMeeking', 20);
-insert into check_test (first_name, last_name, age) values ('Genna', 'Kaysor', 50);
-insert into check_test (first_name, last_name, age) values ('Tess', 'Wesker', 56);
-insert into check_test (first_name, last_name, age) values ('Magnum', 'Danzelman', 89);
-insert into check_test (first_name, last_name, age) values ('Mitzi', 'Pidwell', 34);
-insert into check_test (first_name, last_name, age) values ('Milzie', 'Rohlfing', 70);
-
+insert into check_test (first_name, middle_name, last_name, age) values ('Modestine', '', 'MacMeeking', 20);
+insert into check_test (first_name, middle_name, last_name, age) values ('Genna', '', 'Kaysor', 50);
+insert into check_test (first_name, middle_name, last_name, age) values ('Tess', '', 'Wesker', 56);
+insert into check_test (first_name, middle_name, last_name, age) values ('Magnum', '', 'Danzelman', 89);
+insert into check_test (first_name, middle_name, last_name, age) values ('Mitzi', '', 'Pidwell', 34);
+insert into check_test (first_name, middle_name, last_name, age) values ('Milzie', '', 'Rohlfing', 70);
+
+ALTER TABLE check_test ADD CONSTRAINT not_valid_cons CHECK(middle_name<>'') NOT VALID;
 
 
 drop table if exists default_test;

migtests/tests/pg/constraints/validate

@@ -29,7 +29,11 @@ QUERIES_CHECK = {
 		'code': "23505"
 	},
 	'CHECK_CONDITION': {
-		'query': "insert into check_test (id, first_name, last_name, age) values (7, 'Tom', 'Stewan', 15);",
+		'query': "insert into check_test (id, first_name, middle_name, last_name, age) values (7, 'Tom', 'abc', 'Stewan', 15);",
+		'code': "23514"
+	},
+	'CHECK_CONDITION_NOT_VALID': {
+		'query': "insert into check_test (id, first_name, middle_name, last_name, age) values (7, 'Tom', '', 'Stewan', 52);",
 		'code': "23514"
 	},
 	'FOREIGN_CHECK': {

yb-voyager/cmd/importSchema.go

@@ -225,8 +225,14 @@ func importSchema() error {
 		dumpStatements(finalFailedSqlStmts, filepath.Join(exportDir, "schema", "failed.sql"))
 	}
 
-	if flagPostSnapshotImport && flagRefreshMViews {
-		refreshMViews(conn)
+	if flagPostSnapshotImport {
+		err = importSchemaInternal(exportDir, []string{"TABLE"}, nil)
+		if err != nil {
+			return err
+		}
+		if flagRefreshMViews {
+			refreshMViews(conn)
+		}
 	} else {
 		utils.PrintAndLog("\nNOTE: Materialized Views are not populated by default. To populate them, pass --refresh-mviews while executing `import schema --post-snapshot-import`.")
 	}

yb-voyager/cmd/importSchemaYugabyteDB.go

@@ -27,6 +27,7 @@ import (
 	log "github.com/sirupsen/logrus"
 	"golang.org/x/exp/slices"
 
+	"github.com/yugabyte/yb-voyager/yb-voyager/src/queryparser"
 	"github.com/yugabyte/yb-voyager/yb-voyager/src/utils"
 )
 
@@ -49,6 +50,25 @@ func importSchemaInternal(exportDir string, importObjectList []string,
 	return nil
 }
 
+func isNotValidConstraint(stmt string) (bool, error) {
+	parseTree, err := queryparser.Parse(stmt)
+	if err != nil {
+		return false, fmt.Errorf("error parsing the ddl[%s]: %v", stmt, err)
+	}
+	ddlObj, err := queryparser.ProcessDDL(parseTree)
+	if err != nil {
+		return false, fmt.Errorf("error in process DDL[%s]:%v", stmt, err)
+	}
+	alter, ok := ddlObj.(*queryparser.AlterTable)
+	if !ok {
+		return false, nil
+	}
+	if alter.IsAddConstraintType() && alter.ConstraintNotValid {
+		return true, nil
+	}
+	return false, nil
+}
+
 func executeSqlFile(file string, objType string, skipFn func(string, string) bool) error {
 	log.Infof("Execute SQL file %q on target %q", file, tconf.Host)
 	conn := newTargetConn()
@@ -74,10 +94,13 @@ func executeSqlFile(file string, objType string, skipFn func(string, string) boo
 
 		if objType == "TABLE" {
 			stmt := strings.ToUpper(sqlInfo.stmt)
-			skip := strings.Contains(stmt, "ALTER TABLE") && strings.Contains(stmt, "REPLICA IDENTITY")
+			// Check if the statement should be skipped
+			skip, err := shouldSkipDDL(stmt)
+			if err != nil {
+				return fmt.Errorf("error checking whether to skip DDL: %v", err)
+			}
 			if skip {
-				//skipping DDLS like ALTER TABLE ... REPLICA IDENTITY .. as this is not supported in YB
-				log.Infof("Skipping DDL: %s", sqlInfo.stmt)
+				log.Infof("Skipping DDL: %s", stmt)
 				continue
 			}
 		}
@@ -90,6 +113,24 @@ func executeSqlFile(file string, objType string, skipFn func(string, string) boo
 	return nil
 }
 
+func shouldSkipDDL(stmt string) (bool, error) {
+	skipReplicaIdentity := strings.Contains(stmt, "ALTER TABLE") && strings.Contains(stmt, "REPLICA IDENTITY")
+	if skipReplicaIdentity {
+		return true, nil
+	}
+	isNotValid, err := isNotValidConstraint(stmt)
+	if err != nil {
+		return false, fmt.Errorf("error checking whether stmt is to add not valid constraint: %v", err)
+	}
+	skipNotValidWithoutPostImport := isNotValid && !bool(flagPostSnapshotImport)
+	skipOtherDDLsWithPostImport := (bool(flagPostSnapshotImport) && !isNotValid)
+	if skipNotValidWithoutPostImport || // Skipping NOT VALID CONSTRAINT in import schema without post-snapshot-mode
+		skipOtherDDLsWithPostImport { // Skipping other TABLE DDLs than the NOT VALID in post-snapshot-import mode
+		return true, nil
+	}
+	return false, nil
+}
+
 func executeSqlStmtWithRetries(conn **pgx.Conn, sqlInfo sqlInfo, objType string) error {
 	var err error
 	log.Infof("On %s run query:\n%s\n", tconf.Host, sqlInfo.formattedStmt)

yb-voyager/src/queryparser/ddl_processor.go

@@ -555,11 +555,17 @@ func (atProcessor *AlterTableProcessor) Process(parseTree *pg_query.ParseResult)
 
 			similar to CREATE table 2nd case where constraint is at the end of column definitions mentioning the constraint only
 			so here as well while adding constraint checking the type of constraint and the deferrable field of it.
+
+			ALTER TABLE test ADD CONSTRAINT chk check (id<>'') NOT VALID;
+			stmts:{stmt:...subtype:AT_AddConstraint def:{constraint:{contype:CONSTR_CHECK conname:"chk" location:22
+			raw_expr:{a_expr:{kind:AEXPR_OP name:{string:{sval:"<>"}} lexpr:{column_ref:{fields:{string:{sval:"id"}} location:43}} rexpr:{a_const:{sval:{}
+			location:47}} location:45}} skip_validation:true}} behavior:DROP_RESTRICT}} objtype:OBJECT_TABLE}} stmt_len:60}
 		*/
 		constraint := cmd.GetDef().GetConstraint()
 		alter.ConstraintType = constraint.Contype
 		alter.ConstraintName = constraint.Conname
 		alter.IsDeferrable = constraint.Deferrable
+		alter.ConstraintNotValid = constraint.SkipValidation // this is set for the NOT VALID clause
 		alter.ConstraintColumns = parseColumnsFromKeys(constraint.GetKeys())
 
 	case pg_query.AlterTableType_AT_DisableRule:
@@ -590,10 +596,11 @@ type AlterTable struct {
 	NumSetAttributes  int
 	NumStorageOptions int
 	//In case AlterType - ADD_CONSTRAINT
-	ConstraintType    pg_query.ConstrType
-	ConstraintName    string
-	IsDeferrable      bool
-	ConstraintColumns []string
+	ConstraintType     pg_query.ConstrType
+	ConstraintName     string
+	ConstraintNotValid bool
+	IsDeferrable       bool
+	ConstraintColumns  []string
 }
 
 func (a *AlterTable) GetObjectName() string {
@@ -606,6 +613,10 @@ func (a *AlterTable) AddPrimaryKeyOrUniqueCons() bool {
 	return a.ConstraintType == PRIMARY_CONSTR_TYPE || a.ConstraintType == UNIQUE_CONSTR_TYPE
 }
 
+func (a *AlterTable) IsAddConstraintType() bool {
+	return a.AlterType == pg_query.AlterTableType_AT_AddConstraint
+}
+
 //===========POLICY PROCESSOR ================================
 
 // PolicyProcessor handles parsing CREATE POLICY statements