changefeedccl: support altering filters on db-level feeds #156871

aerfrei · 2025-11-04T16:46:59Z

This commit allows users to change filters on existing
DB-level changefeeds with commands like ALTER CHANGEFEED
<job_id> SET INCLUDE TABLES (including EXCLUDE or UNSET).

This also implements errors when trying to set filters
on non-DB-level feeds, ADD/DROP targets on DB-level feeds,
setting both INCLUDE and EXCLUDE at the same time, and
switching between them without first doing an explicit
unset.

Note that, when setting an include filter on a feed that
already has a filter set, tables not explicitly
respecified in the ALTER CHANGEFEED command will no longer
be included (or excluded) by the filter.

When filter changes result in new tables being watched,
they will only emit changes from the time the filter was
altered.

Epic: CRDB-1421
Fixes: #156484
Fixes: #155986
Informs: #155708
Release note: none

cockroach-teamcity · 2025-11-04T16:47:17Z

This change is

blathers-crl · 2025-11-04T19:29:09Z

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.}

github-actions · 2025-11-04T22:00:30Z

Potential Bug(s) Detected

The three-stage Claude Code analysis has identified potential bug(s) in this PR that may warrant investigation.

Next Steps:
Please review the detailed findings in the workflow run.

Note: When viewing the workflow output, scroll to the bottom to find the Final Analysis Summary.

After you review the findings, please tag the issue as follows:

If the detected issue is real or was helpful in any way, please tag the issue with O-AI-Review-Real-Issue-Found
If the detected issue was not helpful in any way, please tag the issue with O-AI-Review-Not-Helpful

aerfrei · 2025-11-04T22:10:04Z

The AI review said that we should be defensive against targetSpec.FilterList == nil. This shouldn't really be possible for DB level feeds but their evidence that it could be possible is that we have an assertion failure against it in changefeed_stmt.go. I added that check because it doesn't hurt. It's not such a great find, but I'll mark it as "real".

aerfrei · 2025-11-04T22:19:21Z

Self review notes

github-actions · 2025-11-05T03:14:08Z

Potential Bug(s) Detected

The three-stage Claude Code analysis has identified potential bug(s) in this PR that may warrant investigation.

Next Steps:
Please review the detailed findings in the workflow run.

Note: When viewing the workflow output, scroll to the bottom to find the Final Analysis Summary.

After you review the findings, please tag the issue as follows:

If the detected issue is real or was helpful in any way, please tag the issue with O-AI-Review-Real-Issue-Found
If the detected issue was not helpful in any way, please tag the issue with O-AI-Review-Not-Helpful

pkg/ccl/changefeedccl/alter_changefeed_stmt.go

KeithCh · 2025-11-06T14:28:22Z

pkg/ccl/changefeedccl/alter_changefeed_stmt.go

-		newChangefeedStmt.TableTargets = newTargets
+
+		// Only table-level feeds should have table targets in their changefeed statement.
+		if newChangefeedStmt.Level == tree.ChangefeedLevelTable {


There are a bunch of checks for the level in this function (not to mention it's long). It could be worth seeing if you can split the logic into separate functions depending on the level to make it cleaner.

The reason I didn't do this is because so much of the logic (most importantly applying new options, creating the jobRecord and the payload) is in common between the two cases and I think we want to keep it that way. The differences seem relatively minor.

I don't think the solution is something like a separate function called setTableTargetsIfTableLevelFeed.

I agree that maybe there's something better to do here but the specifics haven't come to me yet. I'll think about it some more but if you have a specific idea, please share.

I don't have anything specific but maybe we could have helper functions as well to make the plan hook function shorter. You can also try prompting cursor as well.

Up to you to determine if this is already in a good state.

pkg/ccl/changefeedccl/alter_changefeed_stmt.go

aerfrei · 2025-11-06T16:19:45Z

The new issue from claude:

Bug Summary
Location: pkg/ccl/changefeedccl/alter_changefeed_stmt.go:313-319

Issue: Unconditional initial_scan override for table-level changefeed alterations

Impact: When users alter table-level changefeeds to modify unrelated options (e.g., SET diff), the system silently overrides their initial_scan='no' setting to initial_scan='yes', causing unexpected full table scans and potential performance degradation.

Root Cause: Line 318 sets initial_scan='' (meaning 'yes') for ALL table-level changefeed alterations, but the comment explicitly states this should only happen "for new targets". The isAlteringTargets variable exists (lines 185-189) but is never checked.

Recommended Actions
Immediate Fix: Add && isAlteringTargets condition to line 313:

if newChangefeedStmt.Level != tree.ChangefeedLevelDatabase && isAlteringTargets {
    newDetails.Opts[changefeedbase.OptInitialScan] = ``
}
Test Coverage: Add regression test verifying initial_scan='no' is preserved when altering options without changing targets

Investigation: Review existing production changefeeds to identify if users have been affected by this silent configuration change

Severity: High - Silently changes user configuration, causes unexpected performance impact

This additional check they're asking me to do would change the current behavior for non-db-level feeds. Additionally, I don't think there will actually be a performance impact since the highwater/progress of the feed should be maintained. The thing I needed from this that was relevant to DB level feeds was just that we not set initial scan for db-level feeds. I can file an issue for this if we think there's something real to this (I don't think I personally do), but I don't think it's relevant to this PR.

github-actions · 2025-11-19T23:01:27Z

Potential Bug(s) Detected

The three-stage Claude Code analysis has identified potential bug(s) in this PR that may warrant investigation.

Next Steps:
Please review the detailed findings in the workflow run.

Note: When viewing the workflow output, scroll to the bottom to find the Final Analysis Summary.

After you review the findings, please tag the issue as follows:

If the detected issue is real or was helpful in any way, please tag the issue with O-AI-Review-Real-Issue-Found
If the detected issue was not helpful in any way, please tag the issue with O-AI-Review-Not-Helpful

github-actions · 2025-11-19T23:02:07Z

Potential Bug(s) Detected

The three-stage Claude Code analysis has identified potential bug(s) in this PR that may warrant investigation.

Next Steps:
Please review the detailed findings in the workflow run.

Note: When viewing the workflow output, scroll to the bottom to find the Final Analysis Summary.

After you review the findings, please tag the issue as follows:

If the detected issue is real or was helpful in any way, please tag the issue with O-AI-Review-Real-Issue-Found
If the detected issue was not helpful in any way, please tag the issue with O-AI-Review-Not-Helpful

github-actions · 2025-11-19T23:17:45Z

Potential Bug(s) Detected

The three-stage Claude Code analysis has identified potential bug(s) in this PR that may warrant investigation.

Next Steps:
Please review the detailed findings in the workflow run.

Note: When viewing the workflow output, scroll to the bottom to find the Final Analysis Summary.

After you review the findings, please tag the issue as follows:

If the detected issue is real or was helpful in any way, please tag the issue with O-AI-Review-Real-Issue-Found
If the detected issue was not helpful in any way, please tag the issue with O-AI-Review-Not-Helpful

aerfrei · 2025-11-20T03:36:44Z

Self review

github-actions · 2025-11-21T04:12:33Z

Potential Bug(s) Detected

The three-stage Claude Code analysis has identified potential bug(s) in this PR that may warrant investigation.

Next Steps:
Please review the detailed findings in the workflow run.

Note: When viewing the workflow output, scroll to the bottom to find the Final Analysis Summary.

After you review the findings, please tag the issue as follows:

If the detected issue is real or was helpful in any way, please tag the issue with O-AI-Review-Real-Issue-Found
If the detected issue was not helpful in any way, please tag the issue with O-AI-Review-Not-Helpful

github-actions · 2025-11-21T16:53:58Z

Potential Bug(s) Detected

The three-stage Claude Code analysis has identified potential bug(s) in this PR that may warrant investigation.

Next Steps:
Please review the detailed findings in the workflow run.

Note: When viewing the workflow output, scroll to the bottom to find the Final Analysis Summary.

After you review the findings, please tag the issue as follows:

If the detected issue is real or was helpful in any way, please tag the issue with O-AI-Review-Real-Issue-Found
If the detected issue was not helpful in any way, please tag the issue with O-AI-Review-Not-Helpful

github-actions · 2025-11-21T17:37:36Z

Potential Bug(s) Detected

The three-stage Claude Code analysis has identified potential bug(s) in this PR that may warrant investigation.

Next Steps:
Please review the detailed findings in the workflow run.

Note: When viewing the workflow output, scroll to the bottom to find the Final Analysis Summary.

After you review the findings, please tag the issue as follows:

If the detected issue is real or was helpful in any way, please tag the issue with O-AI-Review-Real-Issue-Found
If the detected issue was not helpful in any way, please tag the issue with O-AI-Review-Not-Helpful

aerfrei · 2025-11-21T18:47:57Z

pkg/ccl/changefeedccl/alter_changefeed_stmt.go

+// we resume the changefeed, new tables being watched by the database level feed
+// (due to ALTER CHANGEFEED changing the filter options) are watched only from
+// the time of the ALTER CHANGEFEED statement.
+func storeNewTablesJobFrontier(


For completeness I'm putting this comment here from the CI's AI review:

Issue: When multiple ALTER CHANGEFEED statements modify filter options on a database-level changefeed, the function incorrectly preserves spans from previous ALTER operations that should no longer be tracked. Root Cause: Lines 1082-1086 correctly compute newTrackedSpans (spans added in current ALTER) Lines 1095-1103 retrieve ALL resolved spans from the previous alter_changefeed frontier and add them back without filtering This causes spans that were removed or are no longer "new" to persist incorrectly in the frontier Impact: The frontier tracks spans that should not be monitored, potentially causing: Incorrect resolved timestamp tracking Resource waste monitoring unnecessary tables Wrong behavior when tables are removed from the changefeed filter

I don't think this issue is right. Please see the test cases I added for this. It's funny because before I made this change to persist old versions of the frontier (while I was working on it) claude told me the opposite, that I was incorrectly overwriting previous frontiers.

asg0451 · 2025-11-26T17:34:05Z

pkg/ccl/changefeedccl/alter_changefeed_stmt.go

+	// For table-level feeds, this should be a no-op since there are no
+	// filter commands as asserted above. Otherwise, apply each filter in order.


nit: dont think this comment is necessary as we bail out at 346 if we're not a db level feed

asg0451 · 2025-11-26T17:35:17Z

pkg/sql/sem/tree/changefeed.go

+// DefaultChangefeedFilterOption returns the default filter that excludes nothing.
+// This is used when unsetting filters or when no explicit filter is specified.
+func DefaultChangefeedFilterOption() ChangefeedFilterOption {
+	return ChangefeedFilterOption{
+		FilterType: ExcludeFilter,
+		Tables:     TableNames{},
+	}
+}


isnt this just the zero value? why have a function returning one?

asg0451 · 2025-11-26T17:37:10Z

pkg/ccl/changefeedccl/alter_changefeed_stmt.go

+	return tree.ChangefeedTableTargets{}, &prevProgress, prevDetails.StatementTime,
+		make(map[tree.ChangefeedTableTarget]jobspb.ChangefeedTargetSpecification), nil


nit: why make a new map here vs using nil?

asg0451 · 2025-11-26T17:37:23Z

pkg/ccl/changefeedccl/alter_changefeed_stmt.go

 	return newProgress, prevStatementTime, nil
 }

+// storeAlterChangefeedFrontier uses the persistentjob frontier to ensure that when


asg0451 · 2025-11-26T17:49:18Z

pkg/ccl/changefeedccl/alter_changefeed_stmt.go

+		return spans, nil
+	}
+
+	// We compute the difference between the spans we would have been tracking


i'm not so sure this is right. cant we do something like identify new spans, load the old frontier, add those spans to it at statementtime, and save it back?

I think you're saying that maybe the order should be to 1) load the old frontier then 2) add the new spans instead of what I'm doing (1- creating a new frontier with the new spans and then 2- adding back in the old frontier by iterating over resolved spans). I'm going to try to make that change.

But are you saying something should be different about how I'm identifying the new spans or what timestamp I add the spans to the frontier with? I think that logic is right and I have tests that verify that, but it was kind of tricky. We spoke about this briefly last week. Any specific concerns?

This commit allows users to change filters on existing DB-level changefeeds with commands like ALTER CHANGEFEED <job_id> SET INCLUDE TABLES (including EXCLUDE or UNSET). This also implements errors when trying to set filters on non-DB-level feeds, ADD/DROP targets on DB-level feeds, setting both INCLUDE and EXCLUDE at the same time, and switching between them without first doing an explicit unset. Note that, when setting an include filter on a feed that already has a filter set, tables not explicitly respecified in the ALTER CHANGEFEED command will no longer be included (or excluded) by the filter. When filter changes result in new tables being watched, they will only emit changes from the time the filter was altered. Epic: CRDB-1421 Fixes: cockroachdb#156484 Fixes: cockroachdb#155986 Informs: cockroachdb#155708 Release note: none

aerfrei force-pushed the alter-db-changefeed branch 4 times, most recently from f23ad71 to c127b6c Compare November 4, 2025 19:29

aerfrei force-pushed the alter-db-changefeed branch 4 times, most recently from ce8b6e9 to 6f53c23 Compare November 4, 2025 21:51

github-actions bot added the o-AI-Review-Potential-Issue-Detected AI reviewer found potential issue. Never assign manually—auto-applied by GH action only. label Nov 4, 2025

aerfrei force-pushed the alter-db-changefeed branch from 6f53c23 to 41b5c4f Compare November 4, 2025 22:08

aerfrei added the O-AI-Review-Real-Issue-Found AI reviewer found real issue label Nov 4, 2025

aerfrei force-pushed the alter-db-changefeed branch 2 times, most recently from d5c46a0 to 239f707 Compare November 5, 2025 03:08

aerfrei force-pushed the alter-db-changefeed branch 5 times, most recently from f628147 to 4ff2981 Compare November 5, 2025 16:11

aerfrei requested a review from KeithCh November 5, 2025 18:53

aerfrei marked this pull request as ready for review November 5, 2025 18:53

aerfrei requested a review from a team as a code owner November 5, 2025 18:53

KeithCh reviewed Nov 6, 2025

View reviewed changes

aerfrei requested a review from KeithCh November 6, 2025 16:20

aerfrei force-pushed the alter-db-changefeed branch 2 times, most recently from 25cb92d to 77f8a4d Compare November 19, 2025 22:59

aerfrei force-pushed the alter-db-changefeed branch 2 times, most recently from e734666 to f1ad028 Compare November 19, 2025 23:14

aerfrei force-pushed the alter-db-changefeed branch from f1ad028 to db34da4 Compare November 20, 2025 03:29

aerfrei force-pushed the alter-db-changefeed branch 2 times, most recently from 0526ff2 to 4c02529 Compare November 20, 2025 04:25

aerfrei requested review from KeithCh and asg0451 November 20, 2025 13:47

aerfrei force-pushed the alter-db-changefeed branch 2 times, most recently from f83b7f1 to 316d69d Compare November 21, 2025 04:06

aerfrei force-pushed the alter-db-changefeed branch from 316d69d to c1bbee1 Compare November 21, 2025 16:45

aerfrei force-pushed the alter-db-changefeed branch 2 times, most recently from 17d0c30 to 744d919 Compare November 21, 2025 17:27

aerfrei commented Nov 21, 2025

View reviewed changes

aerfrei force-pushed the alter-db-changefeed branch 2 times, most recently from f26259e to 40ef5d9 Compare November 23, 2025 17:47

asg0451 requested changes Nov 26, 2025

View reviewed changes

aerfrei force-pushed the alter-db-changefeed branch 2 times, most recently from 641299b to e307642 Compare November 26, 2025 22:12

aerfrei requested a review from asg0451 December 1, 2025 19:15

aerfrei force-pushed the alter-db-changefeed branch from e307642 to 109639c Compare December 3, 2025 20:21

		// For table-level feeds, this should be a no-op since there are no
		// filter commands as asserted above. Otherwise, apply each filter in order.

		return tree.ChangefeedTableTargets{}, &prevProgress, prevDetails.StatementTime,
		make(map[tree.ChangefeedTableTarget]jobspb.ChangefeedTargetSpecification), nil

changefeedccl: support altering filters on db-level feeds #156871

Are you sure you want to change the base?

changefeedccl: support altering filters on db-level feeds #156871

Uh oh!

Conversation

aerfrei commented Nov 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cockroach-teamcity commented Nov 4, 2025

Uh oh!

blathers-crl bot commented Nov 4, 2025

Uh oh!

github-actions bot commented Nov 4, 2025

Potential Bug(s) Detected

Uh oh!

aerfrei commented Nov 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aerfrei commented Nov 4, 2025

Uh oh!

github-actions bot commented Nov 5, 2025

Potential Bug(s) Detected

Uh oh!

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!

Uh oh!

aerfrei commented Nov 6, 2025

Uh oh!

github-actions bot commented Nov 19, 2025

Potential Bug(s) Detected

Uh oh!

github-actions bot commented Nov 19, 2025

Potential Bug(s) Detected

Uh oh!

github-actions bot commented Nov 19, 2025

Potential Bug(s) Detected

Uh oh!

aerfrei commented Nov 20, 2025

Uh oh!

github-actions bot commented Nov 21, 2025

Potential Bug(s) Detected

Uh oh!

github-actions bot commented Nov 21, 2025

Potential Bug(s) Detected

Uh oh!

github-actions bot commented Nov 21, 2025

Potential Bug(s) Detected

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!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

aerfrei commented Nov 4, 2025 •

edited

Loading

aerfrei commented Nov 4, 2025 •

edited

Loading