roachtest: retry transient errors in cdc/multiple-schema-changes by rharding6373 · Pull Request #166338 · cockroachdb/cockroach

rharding6373 · 2026-03-20T22:50:41Z

The cdc/multiple-schema-changes roachtest used ExecMultiple to run ALTER TABLE DROP col statements, which fatals on any error with no retry. A transient "connection refused" error on any ALTER causes the test to never reach the highwater check, leading to a confusing timeout.

Replace ExecMultiple with individual SucceedsSoon retry loops for each ALTER statement. Also handle the UndefinedColumn error case where a previous attempt succeeded at the KV level but the connection dropped before the response was returned.

Fixes: #155830
Fixes: #163154

Co-Authored-By: roachdev-claude roachdev-claude-bot@cockroachlabs.com

Release note: None

trunk-io · 2026-03-20T22:50:44Z

😎 Merged successfully - details.

cockroach-teamcity · 2026-03-20T22:50:53Z

This change is

jeffswenson · 2026-03-23T15:26:27Z

-	alterStmts := []string{"SET sql_safe_updates = false"}
 	for _, tableName := range tableNames {
-		alterStmts = append(alterStmts, fmt.Sprintf(`ALTER TABLE %s DROP col`, tableName))
+		alterStmt := fmt.Sprintf(`ALTER TABLE %s DROP col`, tableName)


nit: if we are going to retry the alter, we can use an idempotent sql statement which avoids the need to examine the error.

ALTER TABLE %s DROP COLUMN IF EXISTS col;

jeffswenson · 2026-03-23T15:28:14Z

-	alterStmts := []string{"SET sql_safe_updates = false"}
 	for _, tableName := range tableNames {
-		alterStmts = append(alterStmts, fmt.Sprintf(`ALTER TABLE %s DROP col`, tableName))
+		alterStmt := fmt.Sprintf(`ALTER TABLE %s DROP col`, tableName)


The artifacts have gc'd so I can't dig into this, but I'm skeptical of fixing this with a retry. Most of the sql statements we make to the test cluster are not wrapped in retry loops. So even if this is the right fix, its not sufficient to fix the issue.

Furthermore, I don't think 'CONNECTION REFUSED' is a transient error in the context of a roachprod test. I would expect it in the context of rolling restarts, but not in a stable cluster. If we see this in a roachtest it suggests either:

We failed to wait for the cluster to be ready.

One of the nodes crashed.

I think you're right. From the little info we have on the issue at this time, it looks like 7 ALTERs succeeded, and the test hung while the 8th ALTER was trying to connect. This suggests something happened to the cluster.

One benefit of this implementation is that if it gets stuck while the ALTERs are executing, it won't wait 1h for the test to time out, but it's possible it could get stuck after and it would still take 1h to time out, so that benefit seems marginal.

Happy to close this PR if you don't think it's useful and just close the issues as not enough info, and wait to see if it happens again to reinvestigate.

aerfrei · 2026-03-23T17:08:12Z

 	for _, tableName := range tableNames {
-		alterStmts = append(alterStmts, fmt.Sprintf(`ALTER TABLE %s DROP col`, tableName))
+		alterStmt := fmt.Sprintf(`ALTER TABLE %s DROP col`, tableName)
+		testutils.SucceedsSoon(t, func() error {


To me the weirdest thing about the failures we were seeing is that we don't see the errors we (presumably) got while executing these statements in ExecMultiple, instead we just see the test timeout eventually. SucceedsSoon fatals so I would think this shouldn't be an issue, but so does ExecMultiple. Any thoughts on why we saw this as a timeout?

In the event that this retry loop still fails, I'd like to be sure we'd see a more descriptive error.

This is a good observation and relates to Jeff's point, too. I think the error you found in #153695 (comment) might have been a red herring. The exec hung because it was trying to connect, then failed when the test timed out, returning "connection refused". So the real issue is what was happening that caused the connection to hang.

rharding6373

LMK if you'd prefer to close this PR (and the related issues) as not enough info.

rharding6373 · 2026-03-26T03:37:09Z

-	alterStmts := []string{"SET sql_safe_updates = false"}
 	for _, tableName := range tableNames {
-		alterStmts = append(alterStmts, fmt.Sprintf(`ALTER TABLE %s DROP col`, tableName))
+		alterStmt := fmt.Sprintf(`ALTER TABLE %s DROP col`, tableName)


rharding6373 · 2026-03-26T03:52:37Z

 	for _, tableName := range tableNames {
-		alterStmts = append(alterStmts, fmt.Sprintf(`ALTER TABLE %s DROP col`, tableName))
+		alterStmt := fmt.Sprintf(`ALTER TABLE %s DROP col`, tableName)
+		testutils.SucceedsSoon(t, func() error {


This is a good observation and relates to Jeff's point, too. I think the error you found in #153695 (comment) might have been a red herring. The exec hung because it was trying to connect, then failed when the test timed out, returning "connection refused". So the real issue is what was happening that caused the connection to hang.

rharding6373 · 2026-03-26T03:52:39Z

-	alterStmts := []string{"SET sql_safe_updates = false"}
 	for _, tableName := range tableNames {
-		alterStmts = append(alterStmts, fmt.Sprintf(`ALTER TABLE %s DROP col`, tableName))
+		alterStmt := fmt.Sprintf(`ALTER TABLE %s DROP col`, tableName)


I think you're right. From the little info we have on the issue at this time, it looks like 7 ALTERs succeeded, and the test hung while the 8th ALTER was trying to connect. This suggests something happened to the cluster.

One benefit of this implementation is that if it gets stuck while the ALTERs are executing, it won't wait 1h for the test to time out, but it's possible it could get stuck after and it would still take 1h to time out, so that benefit seems marginal.

Happy to close this PR if you don't think it's useful and just close the issues as not enough info, and wait to see if it happens again to reinvestigate.

jeffswenson · 2026-03-26T17:18:47Z

I think splitting up the statements is an improvement, but I think we should remove the retry loop until we understand why the statements are failing.

…nges The cdc/multiple-schema-changes roachtest used ExecMultiple to run ALTER TABLE DROP col statements. ExecMultiple uses context.Background(), so if a query hangs (e.g. due to a temporarily unavailable node), it blocks indefinitely and cannot be cancelled. The test's 1-hour timeout fires on the runner goroutine while the test goroutine is still stuck, so the failure is reported as a timeout with no useful error. The actual error (e.g. "connection refused") only surfaces later as a suppressed failure_2 log entry. Replace ExecMultiple with individual sqlDB.Exec calls so each ALTER TABLE statement is executed separately. Use DROP COLUMN IF EXISTS for idempotency. Fixes: cockroachdb#155830 Fixes: cockroachdb#163154 Co-Authored-By: roachdev-claude <roachdev-claude-bot@cockroachlabs.com>

jeffswenson

LGTM

rharding6373 · 2026-03-26T22:40:14Z

TFTR!

rharding6373 marked this pull request as ready for review March 20, 2026 23:17

rharding6373 requested a review from a team as a code owner March 20, 2026 23:17

rharding6373 requested review from aerfrei and removed request for a team March 20, 2026 23:17

jeffswenson reviewed Mar 23, 2026

View reviewed changes

aerfrei reviewed Mar 23, 2026

View reviewed changes

rharding6373 force-pushed the 20260320-multiple-schema-changes-failure branch from e6b54e2 to 233291d Compare March 26, 2026 03:46

rharding6373 commented Mar 26, 2026

View reviewed changes

rharding6373 requested a review from jeffswenson March 26, 2026 16:42

rharding6373 force-pushed the 20260320-multiple-schema-changes-failure branch from 233291d to 01d3ce6 Compare March 26, 2026 17:27

rharding6373 force-pushed the 20260320-multiple-schema-changes-failure branch from 01d3ce6 to 00ebc41 Compare March 26, 2026 21:01

jeffswenson approved these changes Mar 26, 2026

View reviewed changes

trunk-io bot merged commit 5155d48 into cockroachdb:master Mar 27, 2026
26 checks passed

celeste-cockroachdb bot added the target-release-26.3.0 label Mar 27, 2026

Conversation

rharding6373 commented Mar 20, 2026

Uh oh!

trunk-io bot commented Mar 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cockroach-teamcity commented Mar 20, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rharding6373 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jeffswenson commented Mar 26, 2026

Uh oh!

jeffswenson left a comment

Choose a reason for hiding this comment

Uh oh!

rharding6373 commented Mar 26, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

trunk-io bot commented Mar 20, 2026 •

edited

Loading