sql: remove enforce_home_region_follower_reads_enabled#148314
sql: remove enforce_home_region_follower_reads_enabled#148314craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
|
It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR? 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
|
This change is |
|
We can do it as a follow-up if you'd prefer, but I think there is also some code we can remove over in |
|
Your pull request contains more than 1000 changes. It is strongly encouraged to split big PRs into smaller chunks. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
|
@michae2 I volunteer as a reviewer if you think this is reasonably close. |
|
(Let me rebase it at least before you look.) |
DrewKimball
left a comment
There was a problem hiding this comment.
We can do it as a follow-up if you'd prefer, but I think there is also some code we can remove over in
txn_coord_sender_savepoints.goandtxn_interceptor_span_refresher.gothat we can remove as a result of this change.
I think we can now flip KeepRefreshSpansOnSavepointRollback on by default, but can't remove the extra savepoint logic until that setting is removed, is that right?
@DrewKimball reviewed 3 of 21 files at r1, 2 of 13 files at r2, 21 of 21 files at r3, all commit messages.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @michae2)
pkg/ccl/logictestccl/testdata/logic_test/multi_region_remote_access_error line 1203 at r3 (raw file):
# Prepared statement accessing a row in a remote region errors out. retry statement error pq:
nit: should we have error messages here? Or do you think it's uninteresting?
In cockroachdb#97827 we added a new ability to dynamically determine the home region of a row when a query failed due to `enforce_home_region`. Unfortunately, the strategy of switching to follower reads after a savepoint rollback is no longer permitted by the KV layer. This commit removes the dynamic home region lookup ability. Fixes: cockroachdb#113765 Release note (sql change): The functionality provided by session variable `enforce_home_region_follower_reads_enabled` was deprecated in v24.2.4 and is now removed. (The variable itself remains for backward compatibility but has no effect.) (Note that related session variable `enforce_home_region` is _not_ deprecated and still functions normally.)
michae2
left a comment
There was a problem hiding this comment.
TFTR!
We can do it as a follow-up if you'd prefer, but I think there is also some code we can remove over in
txn_coord_sender_savepoints.goandtxn_interceptor_span_refresher.gothat we can remove as a result of this change.I think we can now flip
KeepRefreshSpansOnSavepointRollbackon by default, but can't remove the extra savepoint logic until that setting is removed, is that right?
Good point. Let's do this in a follow-up PR.
bors r=DrewKimball
Reviewable status:
pkg/ccl/logictestccl/testdata/logic_test/multi_region_remote_access_error line 1203 at r3 (raw file):
Previously, DrewKimball (Drew Kimball) wrote…
nit: should we have error messages here? Or do you think it's uninteresting?
Oh, good catch! I just missed a couple spots.
|
Build succeeded: |
In #97827 we added a new ability to dynamically determine the home region of a row when a query failed due to
enforce_home_region. Unfortunately, the strategy of switching to follower reads after a savepoint rollback is no longer permitted by the KV layer. This commit removes the dynamic home region lookup ability.Fixes: #113765
Release note (sql change): The functionality provided by session variable
enforce_home_region_follower_reads_enabledwas deprecated in v24.2.4 and is now removed. (The variable itself remains for backward compatibility but has no effect.) (Note that related session variableenforce_home_regionis not deprecated and still functions normally.)