Allow large object tables to join a replication set.#517
Conversation
Large objects stored by lolor were blocked from replication sets, so a DROP EXTENSION migrated them only on the node where it ran and left the other nodes with unreachable data. Split the skip list so lolor stays excluded from the structure-sync dump but may now be replicated.
📝 WalkthroughWalkthroughspock_node.c now separates replication-set eligibility checks from structure-sync dump exclusions, and a new TAP test verifies that lolor large-object tables can be added to a replication set and replicated. The test is also added to the scheduled TAP suite. ChangesReplication Set Ignore List Refactor
Sequence Diagram(s)Not applicable; the change set is a validation refactor plus a TAP test addition. Related PRs: None identified from provided context. Suggested labels: bug, replication, tests Suggested reviewers: None specified. 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
|
| 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.
🧹 Nitpick comments (1)
src/spock_node.c (1)
134-149: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate entries across skip_/norepset_ lists risk future drift.
norepset_schema/norepset_extensionre-declare"snowflake"and"spock"that already exist in the (unchanged)skip_schema/skip_extensiondump-exclusion lists. With four parallel static lists now needing manual sync, a future addition to one list (e.g. a new internal schema that must be excluded from both dumps and repsets) is easy to add to only one array and forget the other.Consider deriving
norepset_schema/norepset_extensionfromskip_schema/skip_extensionminus the lolor entry (or vice versa), so there's a single source of truth for "internal, replication-unfriendly by default" objects, with lolor explicitly opted back in for repset membership.♻️ Example approach
/* * Build norepset_* from skip_* at init time, excluding lolor, instead of * maintaining two independently-hand-written literal lists. */🤖 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 `@src/spock_node.c` around lines 134 - 149, The replication-set exclusion lists are duplicated across separate skip and norepset arrays, which can drift over time. Update the logic around `norepset_schema` and `norepset_extension` in `spock_node.c` so they are derived from the existing `skip_schema` and `skip_extension` lists, with `lolor` explicitly excluded from the derived replication-set bans. Keep a single source of truth for internal objects and ensure both dump exclusion and repset exclusion stay in sync through the same shared definition.
🤖 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.
Nitpick comments:
In `@src/spock_node.c`:
- Around line 134-149: The replication-set exclusion lists are duplicated across
separate skip and norepset arrays, which can drift over time. Update the logic
around `norepset_schema` and `norepset_extension` in `spock_node.c` so they are
derived from the existing `skip_schema` and `skip_extension` lists, with `lolor`
explicitly excluded from the derived replication-set bans. Keep a single source
of truth for internal objects and ensure both dump exclusion and repset
exclusion stay in sync through the same shared definition.
Verify lolor tables can join a replication set and replicate, that other protected schemas stay blocked, and that add-node structure sync still excludes lolor.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/tap/t/032_lolor_largeobject_repset.pl (2)
34-47: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNo timeout on external clone/build; could hang CI indefinitely.
system("git clone ... $LOLOR_URL ...")and the subsequentmakecall have no timeout. If the network stalls ormakehangs, the entire test run blocks rather than failing fast or falling through toskip_all.🔧 Suggested fix using `timeout`
- my $rc = system("git clone --depth 1 $LOLOR_URL $build >> '$log' 2>&1"); + my $rc = system("timeout 60 git clone --depth 1 $LOLOR_URL $build >> '$log' 2>&1"); return 0 if $rc != 0; - $rc = system("make -C $build USE_PGXS=1 PG_CONFIG='$PG/pg_config' install >> '$log' 2>&1"); + $rc = system("timeout 120 make -C $build USE_PGXS=1 PG_CONFIG='$PG/pg_config' install >> '$log' 2>&1");🤖 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/032_lolor_largeobject_repset.pl` around lines 34 - 47, The ensure_lolor helper can hang CI because the git clone and make steps run without any timeout handling. Update ensure_lolor to wrap the external clone/build commands with a timeout-aware mechanism so they fail fast if the network stalls or the build blocks, and then return 0 so the test can skip or fall through cleanly. Keep the fix localized to ensure_lolor and its existing system() calls, and preserve the current success checks for the lolor.control file.
39-39: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winHardcoded build directory can collide across parallel test runs.
/tmp/spock_lolor_buildis a fixed path; if multiple CI jobs or repeated local runs execute this test concurrently on the same host, they will race on the same directory (therm -rfin one run could delete another's in-progress clone).Consider using a PID- or
File::Temp-based unique path.🤖 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/032_lolor_largeobject_repset.pl` at line 39, The test setup in 032_lolor_largeobject_repset.pl uses a fixed build directory path, which can collide when runs execute in parallel. Update the build path logic around the $build variable to generate a unique per-run directory, such as one based on the process ID or File::Temp, and keep the rest of the test using that dynamic path so concurrent runs do not share or delete the same clone.
🤖 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.
Nitpick comments:
In `@tests/tap/t/032_lolor_largeobject_repset.pl`:
- Around line 34-47: The ensure_lolor helper can hang CI because the git clone
and make steps run without any timeout handling. Update ensure_lolor to wrap the
external clone/build commands with a timeout-aware mechanism so they fail fast
if the network stalls or the build blocks, and then return 0 so the test can
skip or fall through cleanly. Keep the fix localized to ensure_lolor and its
existing system() calls, and preserve the current success checks for the
lolor.control file.
- Line 39: The test setup in 032_lolor_largeobject_repset.pl uses a fixed build
directory path, which can collide when runs execute in parallel. Update the
build path logic around the $build variable to generate a unique per-run
directory, such as one based on the process ID or File::Temp, and keep the rest
of the test using that dynamic path so concurrent runs do not share or delete
the same clone.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 899d14b9-0190-4022-8782-4fec1de70655
📒 Files selected for processing (2)
tests/tap/scheduletests/tap/t/032_lolor_largeobject_repset.pl
✅ Files skipped from review due to trivial changes (1)
- tests/tap/schedule
1 participant
Large objects stored by lolor were blocked from replication sets, so a DROP EXTENSION migrated them only on the node where it ran and left the other nodes with unreachable data. Split the skip list so lolor stays excluded from the structure-sync dump but may now be replicated.