workload/schemachange: expect incomparable types

[annrpom]

This patch ensures that our schemachange workload will
allow for invalid columns of incomparable types to pass the
rowsSatisfyFkConstraint check -- as they are expected to
fail later on with a foreign key violation.

Epic: none

Fixes: #124719
Release note: None

[cockroach-teamcity]

This change is 

[fqazi]

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @DarrylWong, @rafiss, and @renatolabs)

[rafiss]

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @annrpom, @DarrylWong, and @renatolabs)

---

pkg/workload/schemachange/operation_generator.go line 3276 at r1 (raw file):

		       ) AS cons ON cons.conrelid = cols.tableid
		 WHERE table_name SIMILAR TO 'table_w[0-9]_+%'
     AND crdb_sql_type != 'REFCURSOR'

hm, well i think the idea with this function is that it occasionally will return an invalid parent column. so i believe we would want it to keep returning refcursor columns, even though they are invalid for foreign keys.

would it make sense to change rowsSatisfyFkConstraint instead?

diff --git a/pkg/workload/schemachange/error_screening.go b/pkg/workload/schemachange/error_screening.go
index 786ac08e36c..169348e9571 100644
--- a/pkg/workload/schemachange/error_screening.go
+++ b/pkg/workload/schemachange/error_screening.go
@@ -1029,6 +1029,10 @@ SELECT count(*) FROM %s
 
 	numJoinRows, err := og.scanInt(ctx, tx, q)
 	if err != nil {
+		// UndefinedFunction errors mean that the column type is not comparable.
+		if pgErr := new(pgconn.PgError); errors.As(err, &pgErr) && pgcode.MakeCode(pgErr.Code) == pgcode.UndefinedFunction {
+			return false, nil
+		}
 		return false, err
 	}
 	return numJoinRows == childRows, err

[rafiss]

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @annrpom, @DarrylWong, and @renatolabs)

---

pkg/workload/schemachange/operation_generator.go line 3276 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

hm, well i think the idea with this function is that it occasionally will return an invalid parent column. so i believe we would want it to keep returning refcursor columns, even though they are invalid for foreign keys.

would it make sense to change rowsSatisfyFkConstraint instead?

diff --git a/pkg/workload/schemachange/error_screening.go b/pkg/workload/schemachange/error_screening.go
index 786ac08e36c..169348e9571 100644
--- a/pkg/workload/schemachange/error_screening.go
+++ b/pkg/workload/schemachange/error_screening.go
@@ -1029,6 +1029,10 @@ SELECT count(*) FROM %s
 
 	numJoinRows, err := og.scanInt(ctx, tx, q)
 	if err != nil {
+		// UndefinedFunction errors mean that the column type is not comparable.
+		if pgErr := new(pgconn.PgError); errors.As(err, &pgErr) && pgcode.MakeCode(pgErr.Code) == pgcode.UndefinedFunction {
+			return false, nil
+		}
 		return false, err
 	}
 	return numJoinRows == childRows, err

the other reason to avoid the check here is so that we don't need to maintain a list of non-comparable types here. refcursor may not always be the only one.

[annrpom]

it occasionally will return an invalid parent column
ah you're right

that makes sense -- fixed

[annrpom]

TFTR! ('-')7

bors r+

[craig]

Build succeeded:

• local_roachtest_fips
• linux_amd64_fips_build
• windows_build
• examples_orms
• acceptance
• macos_amd64_build
• docker_image_amd64
• unit_tests
• check_generated_code
• macos_arm64_build
• lint
• linux_arm64_build
• linux_amd64_build
• local_roachtest

[rafiss]

blathers backport 24.1