autoddl: keep ALTER TABLE from yanking tables out of custom repsets#490
autoddl: keep ALTER TABLE from yanking tables out of custom repsets#490rasifr wants to merge 4 commits into
Conversation
rasifr
commented
Jun 3, 2026
rasifr
commented
Split the monolithic add_ddl_to_repset() into three pieces with a clear
single responsibility each:
- add_ddl_to_repset() is now a thin orchestrator: GUC guard, extract
target, open relation, expand partitions, apply policy per inheritor.
- extract_ddl_target() owns parse-tree dispatch (ALTER TABLE / OBJECT_INDEX
ATTACH PARTITION / CREATE / CREATE TABLE AS / CREATE SCHEMA / EXPLAIN)
including the recursive cases.
- apply_repset_policy_for_reloid() owns the per-relation policy:
UNLOGGED skip and default vs default_insert_only routing.
Pure structural change; behavior is preserved across all DDL shapes.
📝 WalkthroughWalkthroughThis PR refactors Spock's autoDDL repset routing into a pipeline: extract_ddl_target() identifies table targets and stickiness intent, classify_alter_pkri_change() and classify_repset_membership() detect PK/RI transitions and current membership, and apply_repset_policy_for_reloid() routes tables with stickiness and safety-net fallbacks. ChangesPK/RI-based repset stickiness for autoDDL
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Infer (1.2.0)src/spock_autoddl.csrc/spock_autoddl.c:12:10: fatal error: 'postgres.h' file not found ... [truncated 689 characters] ... opt/infer-linux-x86_64-v1.2.0/lib/infer/facebook-clang-plugins/clang/install/lib/clang/18/include" Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Up to standards ✅🟢 Issues
|
| Category | Results |
|---|---|
| Complexity | 6 medium |
🟢 Metrics 0 duplication
Metric Results Duplication 0
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/spock_autoddl.c`:
- Around line 391-416: classify_repset_membership currently collapses "in
default" and "in any custom" into only_in_default, which breaks mixed-membership
handling; change the membership detection to produce two distinct booleans
(e.g., in_default and in_any_custom) from classify_repset_membership (or call it
differently) and update the logic around pkchange: replace the top-level if
(!only_in_default) check with logic that only returns early (sticky custom
behavior) when the table is not in_default and is in_any_custom (i.e., if
(!in_default && in_any_custom) { /* existing sticky path: if pkchange ==
PKRI_DROPPED && in_upd_del_custom ... else notice and return */ }), otherwise
allow the default-managed reroute behavior to proceed (handling PKRI_DROPPED to
move to DEFAULT_INSONLY_REPSET_NAME and PKRI_ADDED notices with
DEFAULT_REPSET_NAME), keeping existing references to pkchange,
in_upd_del_custom, DEFAULT_INSONLY_REPSET_NAME, DEFAULT_REPSET_NAME,
table_close, and targetrel.
In `@tests/tap/t/030_autoddl_repset_stickiness.pl`:
- Line 3: The TAP plan at the top uses a hardcoded count ("use Test::More tests
=> 20;") but the script actually emits 16 assertions; update the test plan to
match the real count or switch to a dynamic plan: replace the fixed "use
Test::More tests => 20;" declaration with either the correct numeric count (16)
or call done_testing() at the end of the script so the harness no longer
over-counts; locate and update the "use Test::More" line in the test file
(tests/tap/t/030_autoddl_repset_stickiness.pl) accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 31820899-a103-43e7-a20a-ee753fcd6e7a
📒 Files selected for processing (3)
src/spock_autoddl.ctests/tap/scheduletests/tap/t/030_autoddl_repset_stickiness.pl
| @@ -0,0 +1,135 @@ | |||
| use strict; | |||
| use warnings; | |||
| use Test::More tests => 20; | |||
There was a problem hiding this comment.
The TAP plan is over-counted and will fail this test file.
This script runs 16 assertions, not 20, so the harness will report a bad plan even when the behavior is correct. Please fix the count or switch to done_testing().
Suggested fix
-use Test::More tests => 20;
+use Test::More tests => 16;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| use Test::More tests => 20; | |
| use Test::More tests => 16; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/tap/t/030_autoddl_repset_stickiness.pl` at line 3, The TAP plan at the
top uses a hardcoded count ("use Test::More tests => 20;") but the script
actually emits 16 assertions; update the test plan to match the real count or
switch to a dynamic plan: replace the fixed "use Test::More tests => 20;"
declaration with either the correct numeric count (16) or call done_testing() at
the end of the script so the harness no longer over-counts; locate and update
the "use Test::More" line in the test file
(tests/tap/t/030_autoddl_repset_stickiness.pl) accordingly.
With autoDDL and spock.include_ddl_repset both enabled, any ALTER TABLE
on a table that lived in a user-defined replication set was being
forcibly re-routed back into 'default' (or 'default_insert_only'),
silently undoing the operator's repset placement.
Apply ALTER stickiness rules in apply_repset_policy_for_reloid():
- If the ALTER did not add or drop a PK / replica identity, leave
repset membership entirely alone.
- If the ALTER added a PK on a table in a custom repset, emit a
NOTICE and leave membership alone. The operator can move it
manually if they want full UPDATE/DELETE replication.
- If the ALTER dropped the PK on a table in a custom repset that
replicates UPDATE/DELETE, emit a WARNING and move it to
default_insert_only. Without a replica identity, that custom
repset would otherwise produce broken downstream replication.
- Tables in the default repsets continue to be auto-re-routed as
before.
Single-node TAP test that exercises the seven scenarios autoDDL repset
routing has to get right when spock.include_ddl_repset is enabled:
T1: unrelated ALTER on a table in a custom repset is sticky
T2: unrelated ALTER on a default-repset table does not churn
T3: dropping PK on a default-repset table moves it to insert-only
T4: dropping PK in a custom UPDATE/DELETE repset emits WARNING and
falls back to default_insert_only
T5: dropping PK in a custom insert-only repset is sticky
T6: adding PK to a table in a custom repset emits NOTICE and leaves
membership unchanged
T7: CREATE / unrelated ALTER on no-PK tables still land in
default_insert_only
T4 and T6 also assert on the exact NOTICE / WARNING text to lock the
operator-facing messages in place.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
classify_alter_pkri_change() walks atstmt->cmds looking for
AT_AddConstraint / AT_AddIndexConstraint to identify a PK add. But
"ALTER TABLE ... ADD COLUMN x int PRIMARY KEY" arrives as a single
AT_AddColumn command with the PRIMARY KEY embedded in the ColumnDef's
inline constraints list, not as a separate AT_AddConstraint. We were
missing that form: post-state had a PK, has_add_pkri_cmd stayed false,
and the classifier returned PKRI_UNCHANGED — so the stickiness check
short-circuited and the table never moved from default_insert_only
into default after the inline PK arrived.
Add an AT_AddColumn case that inspects ColumnDef->constraints for an
inline CONSTR_PRIMARY. Verified against the reproducer:
CREATE TABLE t1(a int primary key, b int);
SELECT spock.repset_add_table('mt', 't1');
ALTER TABLE t1 DROP COLUMN a; -- safety net: -> default_insert_only
ALTER TABLE t1 ADD COLUMN a int PRIMARY KEY; -- now correctly -> default
Add TAP scenario T8 to lock the fix in place: a no-PK table in
default_insert_only is promoted to default after ADD COLUMN ... PRIMARY
KEY. This test fails without the AT_AddColumn case.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
rasifr
force-pushed
the
task/SPOC-539/autoddl-repset-stickiness
branch
from
6c770bf to
4e56d36
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/tap/t/030_autoddl_repset_stickiness.pl`:
- Line 3: The TAP plan in the test file is wrong: update the Test::More plan
declaration (the line using "use Test::More tests => 27;") to match the actual
number of assertions (change 27 to 23) or replace it with "use Test::More tests
=> 23;" (alternatively use "use Test::More 'no_plan';" if you prefer not to fix
a numeric plan); ensure the plan value matches the total assertions across
T1–T10 so the TAP harness doesn't report a bad plan.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8a036fd8-52b1-4bed-9934-c499a1f5386c
📒 Files selected for processing (3)
src/spock_autoddl.ctests/tap/scheduletests/tap/t/030_autoddl_repset_stickiness.pl
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/tap/schedule
- src/spock_autoddl.c
| @@ -0,0 +1,180 @@ | |||
| use strict; | |||
| use warnings; | |||
| use Test::More tests => 27; | |||
There was a problem hiding this comment.
The TAP plan count is incorrect and will cause test failure.
The plan declares 27 tests, but the file contains only 23 assertions (2+2+2+3+2+3+2+3+2+2 across T1–T10). The TAP harness will report a bad plan even when all assertions pass.
🔧 Proposed fix
-use Test::More tests => 27;
+use Test::More tests => 23;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/tap/t/030_autoddl_repset_stickiness.pl` at line 3, The TAP plan in the
test file is wrong: update the Test::More plan declaration (the line using "use
Test::More tests => 27;") to match the actual number of assertions (change 27 to
23) or replace it with "use Test::More tests => 23;" (alternatively use "use
Test::More 'no_plan';" if you prefer not to fix a numeric plan); ensure the plan
value matches the total assertions across T1–T10 so the TAP harness doesn't
report a bad plan.
1 participant
