Mirror the source node's replication sets onto the node being added. - #559
Mirror the source node's replication sets onto the node being added.#559ibrarahmad wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe Z0DAN SQL now discovers and mirrors provider replication sets during node addition. Subscription creation uses complete provider-specific replication-set lists. The workflow synchronizes sets after initial synchronization and before subscription enablement. ChangesReplication-set synchronization
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
samples/Z0DAN/zodan.sql (2)
1237-1254: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winReuse one source set list instead of a second dblink query.
Line 1211 already reads the source set list. This loop reads it again over dblink. Two reads can return different results if a user changes sets on the source between the two queries. The membership drift check in step 5 does not cover set definitions, so a set created on the source between the two reads is created locally and then dropped again.
Collect the source set names into a local
text[]once, then use it for both the create/alter loop and the drop loop.🤖 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 `@samples/Z0DAN/zodan.sql` around lines 1237 - 1254, Reuse the source replication-set list already fetched near line 1211 instead of issuing a second dblink query in the drop loop. Store that result in a local text[] and use it for both the create/alter processing and the NOT IN membership check before spock.repset_drop, preserving consistent source-set membership across the operation.
2166-2166: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winTwo procedures initialize
repsetswith a dblink call insideDECLARE. A variable initializer runs before the block body, so anEXCEPTIONhandler in the procedure cannot catch a remote-connection failure, and no phase notice is emitted before the error. The shared fix is to assignrepsetsas the first statement afterBEGIN.
samples/Z0DAN/zodan.sql#L2166-L2166: increate_new_to_source_subscription, declarerepsets text;without an initializer and assignrepsets := spock.source_repset_list(src_dsn);afterBEGIN.samples/Z0DAN/zodan.sql#L2201-L2201: increate_source_to_new_subscription, apply the same change.🤖 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 `@samples/Z0DAN/zodan.sql` at line 2166, Move the dblink initializer out of the DECLARE sections of create_new_to_source_subscription and create_source_to_new_subscription: declare repsets as text, then assign spock.source_repset_list(src_dsn) as the first statement after BEGIN so the existing exception handlers and phase notices can handle connection failures. Apply this change at samples/Z0DAN/zodan.sql lines 2166-2166 and 2201-2201.
🤖 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 `@samples/Z0DAN/zodan.sql`:
- Around line 1327-1361: Replace the row-count comparisons in the add_node
membership recheck with ordered digests of the complete replication-set
definitions and memberships, including set attributes such as set_att_list and
set_row_filter. Apply the same digest comparison to table and sequence
memberships, declare src_digest and local_digest as text in the DECLARE block,
and retain the existing failure notice/exception when either digest differs.
- Around line 2787-2814: Update the hardcoded RAISE NOTICE phase labels in the
post-Phase 8 procedures to match add_node’s current ordering: enabling disabled
subscriptions is Phase 9, creating the subscription to the source is Phase 10,
and each subsequent procedure advances accordingly. Apply this consistently to
the procedures invoked after sync_repsets_from_source, without changing their
execution order.
- Around line 1262-1287: The row-filter handling in the dblink query must
preserve the catalog’s pg_node_tree value instead of deparsing set_row_filter
with pg_get_expr and passing text to spock.repset_add_table. Update the SELECT
and corresponding dblink record type to copy or restore the row filter through
the API’s representation-preserving mechanism, while keeping the existing table
and replication-set processing unchanged.
- Line 1897: Ensure every source-derived replication set in repsets is owned by
each provider referenced by rec.dsn before subscriptions are created. Validate
provider membership and add or mirror missing sets for other providers,
preserving all source replication sets so tables in them are included in
replication.
---
Nitpick comments:
In `@samples/Z0DAN/zodan.sql`:
- Around line 1237-1254: Reuse the source replication-set list already fetched
near line 1211 instead of issuing a second dblink query in the drop loop. Store
that result in a local text[] and use it for both the create/alter processing
and the NOT IN membership check before spock.repset_drop, preserving consistent
source-set membership across the operation.
- Line 2166: Move the dblink initializer out of the DECLARE sections of
create_new_to_source_subscription and create_source_to_new_subscription: declare
repsets as text, then assign spock.source_repset_list(src_dsn) as the first
statement after BEGIN so the existing exception handlers and phase notices can
handle connection failures. Apply this change at samples/Z0DAN/zodan.sql lines
2166-2166 and 2201-2201.
🪄 Autofix
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 Plus
Run ID: 646b67b0-afe4-442c-b1d3-705a93840996
📒 Files selected for processing (1)
samples/Z0DAN/zodan.sql
0bea1e8 to
149b251
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
samples/Z0DAN/zodan.sql (1)
2844-2848: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPhase 8 progress notices never reach the user.
sync_repsets_from_sourceraises notices at lines 1189, 1219, 1282, 1350 and 1401. A notice raised inside a dblink session is not forwarded to the calling session, so all of them are discarded and only line 2847 is printed. Phase 8 is then the one phase that reports no counters, while theRAISE EXCEPTIONpaths still surface as dblink errors.Return the counters from the remote call and print them locally, or accept the gap and drop the unreachable notices from the procedure.
Also refine the comment at lines 1164-1165. A single
CALLoverdblink_execcommits as one remote transaction, so the locks are released when that CALL returns, not after each statement. The deadlock-avoidance reason stays valid.🤖 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 `@samples/Z0DAN/zodan.sql` around lines 2844 - 2848, Update Phase 8 around sync_repsets_from_source and its dblink_exec call so the remote procedure returns its counters and the caller prints them locally, preserving surfaced exceptions. Revise the nearby lock comment to state that locks are released when the single remote CALL transaction completes, while retaining the deadlock-avoidance rationale.
🤖 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 `@samples/Z0DAN/zodan.sql`:
- Line 2209: Confirm the add_node call order and update the Phase notice in
create_new_to_source_subscription so it uses the correct unique phase number,
avoiding the duplicate Phase 10 announcement from
create_sub_on_new_node_to_src_node. Keep the surrounding notice text and
procedure behavior unchanged.
---
Nitpick comments:
In `@samples/Z0DAN/zodan.sql`:
- Around line 2844-2848: Update Phase 8 around sync_repsets_from_source and its
dblink_exec call so the remote procedure returns its counters and the caller
prints them locally, preserving surfaced exceptions. Revise the nearby lock
comment to state that locks are released when the single remote CALL transaction
completes, while retaining the deadlock-avoidance rationale.
🪄 Autofix
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 Plus
Run ID: 06dfa929-eea3-4583-a1bc-157be3da362f
📒 Files selected for processing (1)
samples/Z0DAN/zodan.sql
149b251 to
3548c38
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
Subscriptions named the three built-in sets literally, and nothing copied the source's sets to the new node, so tables in a user-created set were silently neither sent nor received. Membership was re-derived by AutoDDL during the structure restore rather than copied, losing column lists, row filters, and any table the user had removed from a set. Read each subscription's sets from its provider, and copy the definitions and membership once the data sync completes.
3548c38 to
8638338
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
danolivo
left a comment
danolivo
left a comment
There was a problem hiding this comment.
Basically, it looks good and might serve as a quick intermediate fix. But before merging it, I'd like to see the reasoning:
- Why base machinery not inside the Spock. spock.repset_export(node_id) / spock.repset_import(jsonb) might serve better and be used in different situations. At least - no dblink needed, less round trips, locks, etc. spock.repset_diff() also might be a general tool and let users be sure if replication sets are symmetrical.
- AutoDDL on a next table will send it to default/default_insert_only repset even if user deliberately put it into a custom repset, isn't it?
- It seems to me that COPY machinery shouldn't rely on AutoDDL and we need to add one more stage after the SYNC_STATUS_CONSTRAINTS - SYNC_STATUS_REPSETS. It will explicitly guarantee that we mirror replication sets (it may be designed optional) and remove (potentially) the whole phase 8 from zodan.
3 participants
Subscriptions named the three built-in sets literally, and nothing copied the source's sets to the new node, so tables in a user-created set were silently neither sent nor received. Membership was re-derived by AutoDDL during the structure restore rather than copied, losing column lists, row filters, and any table the user had removed from a set. Read the sets from the source instead, and copy the definitions and membership once the data sync has completed.