sql: treat memory monitor errors as retriable #120806
Conversation
|
This change is |
fqazi
force-pushed
the
memory-monitor-errors
branch
2 times, most recently
from
6ed0b14
to
8dc92b1
Compare
pkg/sql/schema_changer.go
Outdated
| @@ -254,7 +254,7 @@ func IsPermanentSchemaChangeError(err error) bool { | |||
| } | |||
|
|
|||
| switch pgerror.GetPGCode(err) { | |||
| case pgcode.SerializationFailure, pgcode.InternalConnectionFailure: | |||
| case pgcode.SerializationFailure, pgcode.InternalConnectionFailure, pgcode.OutOfMemory, pgcode.DiskFull: | |||
There was a problem hiding this comment.
hm for DiskFull does retrying also make sense? i'm wondering if retrying could make the issue worse.
pkg/sql/schema_changer_test.go
Outdated
| // Run across both nodes to make sure that the error makes it across distsql | ||
| // boundaries. | ||
| testutils.RunTrueAndFalse(t, "local", func(t *testing.T, local bool) { | ||
| var shouldFail int64 |
There was a problem hiding this comment.
nit: make this a atomic.Int64 for slightly better readability
| tdb.Exec(t, "INSERT INTO foo VALUES (1)") | ||
| tdb.Exec(t, `ALTER TABLE foo EXPERIMENTAL_RELOCATE SELECT ARRAY[$1], 1`, | ||
| tc.Server(dataNode).GetFirstStoreID()) | ||
| tdb.Exec(t, `ALTER TABLE foo ADD COLUMN j INT NOT NULL DEFAULT 42`) |
There was a problem hiding this comment.
nit: could you add one more assertion here to verify that shouldFail is > 2? that way we are sure the error was injected.
fqazi
force-pushed
the
memory-monitor-errors
branch
from
8dc92b1
to
2b87b92
Compare
There was a problem hiding this comment.
@rafiss TFTR!
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @rafiss)
pkg/sql/schema_changer.go line 257 at r1 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
hm for DiskFull does retrying also make sense? i'm wondering if retrying could make the issue worse.
Done.
Good point, and I reverted this part since that is riskier.
|
bors r+ |
|
Build failed: |
Release note (bug fix): Memory exhaustion errors which can occur during schema changes are no longer considered to be permanent failures. These schema changes will now be retried rather than reverted. Fixes: cockroachdb#120807
fqazi
force-pushed
the
memory-monitor-errors
branch
from
2b87b92
to
f280d53
Compare
|
bors r+ |
Release note (bug fix): Memory exhaustion errors which can occur during schema changes are no longer considered to be permanent failures. These schema changes will now be retried rather than reverted.
This is just rebase of a PR by @ajwerner: #71816, with a minor change to improve test reliability.
Fixes: #120807