release-25.4: changefeedccl: force-disable per-table PTS #154478
Conversation
![blathers[crl]](https://avatars.githubusercontent.com/in/59700?s=40&u=95b789b1d594ac6b74f377aa7555d8d98d63240d&v=4){kind=avatar}
|
Thanks for opening a backport. Before merging, please confirm that the change does not break backwards compatibility and otherwise complies with the backport policy. Include a brief release justification in the PR description explaining why the backport is appropriate. All backports must be reviewed by the TL for the owning area. While the stricter LTS policy does not yet apply, please exercise judgment and consider gating non-critical changes behind a disabled-by-default feature flag when appropriate. |
blathers-crl
bot
added
the
backport
|
This change is |
aerfrei
force-pushed
the
remove-per-table-pts-25.4
branch
from
0cc36f7 to
90bda24
Compare
| var progressConfig *execinfrapb.ChangefeedProgressConfig | ||
| if execCtx.ExecCfg().Settings.Version.IsActive(ctx, clusterversion.V25_4) { | ||
| perTableTrackingEnabled := changefeedbase.TrackPerTableProgress.Get(sv) | ||
| perTableProtectedTimestampsEnabled := changefeedbase.PerTableProtectedTimestamps.Get(sv) |
There was a problem hiding this comment.
Maybe we should remove this cluster setting altogether since there are no longer any production usages?
There was a problem hiding this comment.
Pros:
It means that if this builds it's very clear that we didn't miss any cases where the flag could potentially do something. (I think this makes it worthwhile for me, so I'll make the change.)
We could theoretically rename this flag and not have to worry about backwards compatibility to 25.4.
It prevents people from setting this flag which could have implications if they upgrade.
Potential Cons:
I think this would mean we have to touch existing tests that specify this flag (to set it to false) and remove those references as well, that would increase the footprint of this pr by another two tests and mean if we wanted to make any changes to those tests and backport them, there could be more merge conflicts. But we very likely won't backport things that interact with this PTS code.
There was a problem hiding this comment.
We could theoretically rename this flag and not have to worry about backwards compatibility to 25.4.
It prevents people from setting this flag which could have implications if they upgrade.
Yeah these are the two main benefits I was thinking of. Also, similar to the first benefit, if we decide to not add this flag, we won't have to add it to the list of retired settings.
aerfrei
force-pushed
the
remove-per-table-pts-25.4
branch
from
90bda24 to
2845ec6
Compare
|
|
||
| // PerTableProtectedTimestamps enables per-table protected timestamp records | ||
| // instead of a single record for all tables in a changefeed. | ||
| var PerTableProtectedTimestamps = settings.RegisterBoolSetting( |
There was a problem hiding this comment.
I think we'll have to update/skip the per-table PTS benchmark roachtests too
There was a problem hiding this comment.
Good call out. Since they those roachtests are generally for multi-table feeds, I'll keep them in but only with per-table pts set to false and remove the actual query that sets the setting.
There was a problem hiding this comment.
That run failed with VM preemption for the 50000 table test. It failed while initializing the workload.
Started a new run here. I hope it will also be helped by the fact that the backport removing the latency assertion has now been merged.
There was a problem hiding this comment.
Looks like we missed the frontier persistence benchmarks, opened #154832
aerfrei
force-pushed
the
remove-per-table-pts-25.4
branch
2 times, most recently
from
35699ac to
1aa054b
Compare
Out of an abundance of caution, per-table protected timestamp records are now fully disabled, even if the corresponding cluster setting is enabled. This ensures that no per-table PTS records are created under any circumstances to prevent users from entering an irrecoverable state by accident. Fixes: cockroachdb#154479 Epic: CRDB-1421 Release note: None
aerfrei
force-pushed
the
remove-per-table-pts-25.4
branch
from
1aa054b to
9da8496
Compare
andyyang890
changed the title
Out of an abundance of caution, per-table protected timestamp
records are now fully disabled, even if the corresponding cluster
setting is enabled. This ensures that no per-table PTS records
are created under any circumstances to prevent users from entering
an irrecoverable state by accident.
Fixes: #154479
Epic: CRDB-1421
Release note: None
Release justification: As mentioned above, this is to prevent potential problems for users that would arise if they used the per-table pts feature which isn't yet ready to be used in production as of 25.4