Conversation
A walsender reports server_version like any other backend, so a logical-replication connection does return a non-zero PQserverVersion(). Two comments claimed the opposite to justify using the SQL connection for version detection. The conclusion (use the SQL connection) is still right -- it is held open anyway for the slot-reclaim and progress queries -- but the stated reason was wrong. Fix the comments only; no behaviour change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A structure sync shells out to pg_dump/pg_restore, which get_pg_executable() pins to the subscriber's own major version. pg_dump cannot read a server of a newer major version than itself and aborts with "server version mismatch"; previously that surfaced only as an opaque pg_dump exit code, after spock had already created a replication slot and exported a snapshot. Add upstream_version_supports_structure_sync(), mirroring pg_dump's own _check_database_version() test, and enforce it in the apply worker before the slot is created. The mismatch is permanent, so a plain ERROR would make the worker crash-loop and reconnect to the publisher on every restart; instead disable the subscription -- as the nonrecoverable init steps already do -- and FATAL with an actionable hint. An unknown (zero) version is treated as unsupported, failing closed. Data-only syncs use COPY, which has no such restriction, and are left untouched. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Run the same version check in spock.create_subscription() when synchronize_structure is requested, so the user gets an immediate, clear error at DDL time instead of a background apply worker that disables itself moments later. Reuses upstream_version_supports_structure_sync() on the connection already opened to fetch the provider's node info, and reports the provider's version in the message. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR adds PostgreSQL version compatibility checks for structure synchronization in Spock. It introduces a helper function to validate upstream version support and deploys version checks at subscription creation and sync initialization. Minor inline comment updates clarify which SQL connection is used for version detection. ChangesStructure Synchronization Version Compatibility
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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_sync.csrc/spock_sync.c:13:10: fatal error: 'postgres.h' file not found ... [truncated 680 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
|
| 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: 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 `@src/spock_sync.c`:
- Around line 333-356: The function upstream_version_supports_structure_sync
currently only rejects remoteversion==0 and newer majors; change it to mirror
pg_dump's inclusive [minRemoteVersion, maxRemoteVersion] check by computing the
remote server version (using PQserverVersion(origin_conn)) and rejecting if
remoteversion == 0 or remoteversion < minRemoteVersion or remoteversion >
maxRemoteVersion (i.e. implement the same bounds logic used by
_check_database_version()/pg_dump), so update
upstream_version_supports_structure_sync to validate both the lower and upper
bounds before returning true.
🪄 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: 77c36fc3-e628-4b56-a8ba-60ae1c5218b5
📒 Files selected for processing (3)
include/spock_sync.hsrc/spock_functions.csrc/spock_sync.c
1 participant
Problem
When a subscription is created with
synchronize_structure = true, spock copies the provider's schema by shelling out topg_dump/pg_restore. Those binaries are pinned to the subscriber's own major version (viaget_pg_executable()), andpg_dumpcannot read a server of a newer major version than itself — it aborts with"aborting because of server version mismatch".Until now that incompatibility surfaced badly:
pg_dumpexit code wrapped in"could not execute pg_dump", and only after spock had already created a replication slot and exported a snapshot on the provider.ERROR, die, and be restarted by the launcher — reconnecting to the publisher and failing again on every cycle, indefinitely.What this PR does
Detects the version mismatch up front by mirroring
pg_dump's own_check_database_version()test against the live connection, and enforces it at two points:sub_create(foreground): rejects the operation immediately with a clearERRORnaming the provider's PostgreSQL version, before any slot/snapshot is created. This is the path most users hit.ERROR-ing into a restart loop) and exitsFATALwith an actionable hint. Disabling is what actually breaks the loop — the severity keyword alone would not.Both call sites share a single predicate,
upstream_version_supports_structure_sync(), so the "must trackpg_dump's rule" logic lives in one place. An unknown (zero) version — a dead connection or a non-PostgreSQL endpoint — is treated as unsupported (fail closed).Data-only syncs use
COPY, which has no such version restriction, and are deliberately left untouched: the check is scoped to structure-bearing sync kinds only.