SPOC-489: Add bidirectional large object migration between native and lolor storage#34
Conversation
…rage Add migrate_from_native() and migrate_to_native() SQL functions that move large objects between PostgreSQL's native pg_catalog storage and lolor's replication-compatible tables, preserving OIDs, owners, ACLs, and data. The DROP EXTENSION event trigger now calls migrate_to_native() automatically before removing the extension, preventing data loss that previously occurred when lolor tables were dropped with the schema. Key details: - Forward migration uses bulk INSERT for performance - Reverse migration uses the native LO API (lo_create_orig/lowrite_orig) since direct INSERT into pg_catalog.pg_largeobject is not allowed - LOBLKSIZE derived at runtime from block_size GUC for non-default builds - Uses lo_lseek64_orig for large objects exceeding 2 GB - Both functions require superuser and verify lolor is enabled - Event trigger guards migrate_to_native() with pg_proc existence check for backward compatibility with versions < 1.2.3 - Emits NOTICE during installation if streaming replicas are detected - Migration is safe only in master-replica configurations for now Add regression tests for forward migration, reverse migration via DROP EXTENSION, manual migrate_to_native, empty-database edge cases, and the 1.2.2 to 1.2.3 upgrade path.
|
Warning Review limit reached
More reviews will be available in 6 minutes and 50 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR adds v1.3 of the lolor extension with bidirectional migration between native PostgreSQL large objects and lolor storage. The feature includes two migration functions, automatic reverse migration on DROP EXTENSION, comprehensive test coverage including OID conflict scenarios, and documentation. ChangesLarge Object Migration v1.3
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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
|
| Category | Results |
|---|---|
| Compatibility | 26 high (26 false positives) |
🟢 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.
mason-sharp
left a comment
mason-sharp
left a comment
There was a problem hiding this comment.
Important feature. I changed the version number to be 1.3.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
sql/lolor.sql (1)
109-145: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd owner/ACL assertions to the migration tests.
These cases validate OID and payload round-tripping, but they never assert that
lomownerandlomaclsurvive either migration direction. Sincemigrate_to_native()manually patchespg_largeobject_metadata, that part of the feature contract can regress without failing this suite. Please add a role/grant setup and verify owner/ACL preservation before and after both paths.Also applies to: 156-174
🤖 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 `@sql/lolor.sql` around lines 109 - 145, Add assertions and setup to verify lomowner and lomacl survive both directions of migration: create a test role, set ownership and specific ACLs on the native large objects (referencing native_oid1, native_oid2) and on the lolor-created object (lolor_direct_oid) before calling lolor.migrate_from_native() and before DROP EXTENSION, then after each migration step query pg_catalog.pg_largeobject_metadata and lolor.pg_largeobject_metadata for lomowner and lomacl and assert they match the original role and ACL entries; ensure these checks run both immediately after migrate_from_native() (confirm native objects removed and lolor rows have correct lomowner/lomacl) and after DROP EXTENSION/reverse migration (confirm pg_largeobject_metadata rows restored with the same lomowner/lomacl).
♻️ Duplicate comments (1)
lolor--1.2.2--1.3.sql (1)
156-181:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReplace the per-page LO API loop with bulk catalog inserts.
This is the same unresolved issue from the earlier review: the function already requires superuser, so the comment about not being able to
INSERTintopg_catalog.pg_largeobjectdoes not hold here. The row-by-rowlo_create_orig/lo_open_orig/lowrite_origpath is much slower thanmigrate_from_native(), can expand sparse objects, and leaves the descriptor open on mid-write failures. A bulkINSERT ... SELECTinto the native large-object catalogs would be both faster and safer.🤖 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 `@lolor--1.2.2--1.3.sql` around lines 156 - 181, Replace the slow per-page native-LO write loop (the FOR r_meta ... PERFORM pg_catalog.lo_create_orig(r_meta.oid); fd := pg_catalog.lo_open_orig(...); FOR r_data ... PERFORM pg_catalog.lowrite_orig(...); PERFORM pg_catalog.lo_close_orig(fd);) with a single bulk catalog insertion: create or ensure the pg_largeobject_metadata row for r_meta (lomowner/lomacl preserved) and then perform an INSERT INTO pg_catalog.pg_largeobject ... SELECT pageno, data FROM lolor.pg_largeobject WHERE loid = r_meta.oid to copy all pages in one statement; remove use of lo_open_orig/lo_lseek64_orig/lowrite_orig/lo_close_orig and the inner FOR r_data loop, run under the function's superuser rights, and ensure transactional safety so partial failures do not leave an open descriptor or half-migrated object.
🧹 Nitpick comments (1)
lolor--1.2.2--1.3.sql (1)
88-92: ⚡ Quick winMake the unlink input explicitly buffered for determinism (not because it’s guaranteed to “skip” OIDs)
lolor--1.2.2--1.3.sql:88-92wraps the scan asFROM (SELECT ...), which doesn’t strictly guarantee planner-level buffering; however, PostgreSQL MVCC/self-visibility generally avoids “missing” tuples purely due to later self-modifications. Still, using an explicitly materialized intermediate set (e.g., temp table /WITH ... AS MATERIALIZED) would make the unlink input evaluation order deterministic and remove any execution-order surprises.🤖 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 `@lolor--1.2.2--1.3.sql` around lines 88 - 92, The current PERFORM pg_catalog.lo_unlink_orig(oid) FROM (SELECT oid FROM pg_catalog.pg_largeobject_metadata) relies on an unmaterialized subquery; make the unlink input explicitly buffered for deterministic evaluation by first materializing the OID set (e.g., create a temporary table or use a WITH ... AS MATERIALIZED subquery) and then iterate (or SELECT FROM) that materialized set when calling lo_unlink_orig; ensure you reference the same pg_largeobject_metadata OIDs and keep the call to lo_unlink_orig(oid) but change its input source to the explicitly materialized relation.
🤖 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.
Outside diff comments:
In `@sql/lolor.sql`:
- Around line 109-145: Add assertions and setup to verify lomowner and lomacl
survive both directions of migration: create a test role, set ownership and
specific ACLs on the native large objects (referencing native_oid1, native_oid2)
and on the lolor-created object (lolor_direct_oid) before calling
lolor.migrate_from_native() and before DROP EXTENSION, then after each migration
step query pg_catalog.pg_largeobject_metadata and lolor.pg_largeobject_metadata
for lomowner and lomacl and assert they match the original role and ACL entries;
ensure these checks run both immediately after migrate_from_native() (confirm
native objects removed and lolor rows have correct lomowner/lomacl) and after
DROP EXTENSION/reverse migration (confirm pg_largeobject_metadata rows restored
with the same lomowner/lomacl).
---
Duplicate comments:
In `@lolor--1.2.2--1.3.sql`:
- Around line 156-181: Replace the slow per-page native-LO write loop (the FOR
r_meta ... PERFORM pg_catalog.lo_create_orig(r_meta.oid); fd :=
pg_catalog.lo_open_orig(...); FOR r_data ... PERFORM
pg_catalog.lowrite_orig(...); PERFORM pg_catalog.lo_close_orig(fd);) with a
single bulk catalog insertion: create or ensure the pg_largeobject_metadata row
for r_meta (lomowner/lomacl preserved) and then perform an INSERT INTO
pg_catalog.pg_largeobject ... SELECT pageno, data FROM lolor.pg_largeobject
WHERE loid = r_meta.oid to copy all pages in one statement; remove use of
lo_open_orig/lo_lseek64_orig/lowrite_orig/lo_close_orig and the inner FOR r_data
loop, run under the function's superuser rights, and ensure transactional safety
so partial failures do not leave an open descriptor or half-migrated object.
---
Nitpick comments:
In `@lolor--1.2.2--1.3.sql`:
- Around line 88-92: The current PERFORM pg_catalog.lo_unlink_orig(oid) FROM
(SELECT oid FROM pg_catalog.pg_largeobject_metadata) relies on an unmaterialized
subquery; make the unlink input explicitly buffered for deterministic evaluation
by first materializing the OID set (e.g., create a temporary table or use a WITH
... AS MATERIALIZED subquery) and then iterate (or SELECT FROM) that
materialized set when calling lo_unlink_orig; ensure you reference the same
pg_largeobject_metadata OIDs and keep the call to lo_unlink_orig(oid) but change
its input source to the explicitly materialized relation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f82a1d95-2fd2-4452-9075-80522d01a78c
⛔ Files ignored due to path filters (1)
expected/lolor.outis excluded by!**/*.out
📒 Files selected for processing (5)
Makefilelolor--1.2.2--1.3.sqllolor.controlsql/lolor.sqlsrc/lolor.c
✅ Files skipped from review due to trivial changes (2)
- lolor.control
- Makefile
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lolor.c
The platform's pg16 is built --with-llvm expecting LLVM 20, but the rockylinux:9 base image now ships LLVM 21, so the JIT bitcode step failed (llvm-lto not found) and the node containers exited. Build with with_llvm=no and out of the bind-mounted tree, and dump container logs on CI failure.
3 participants
Adds
lolor.migrate_from_native()andlolor.migrate_to_native()SQL functionsthat move large objects between PostgreSQL's native
pg_catalogstorage andlolor's replication-compatible tables, preserving OIDs, owners, ACLs, and data.
Design decisions
migrate_from_native()explicitly afterCREATE EXTENSION lolor. This requires superuser and should be done during a maintenance window.DROP EXTENSION lolortriggersmigrate_to_native()via the event trigger, ensuring no large objects are lost on extension removal. If migration fails (e.g. OID conflict), the DROP is rejected so the user can resolve the issue and retry.Implementation details
INSERTfor performance; reverse migration uses the native LO API (lo_create_orig/lo_lseek64_orig/lowrite_orig) since directINSERTintopg_catalog.pg_largeobjectis not allowed from SQL.LOBLKSIZEis derived at runtime fromcurrent_setting('block_size')to support non-default block sizes.lo_lseek64_origwithbigintoffsets to handle large objects exceeding 2 GB.migrate_to_native()with apg_procexistence check for backward compatibility with lolor versions < 1.2.3.lo_unlink_origto avoid scanning the catalog while modifying it.NOTICEduring installation if active streaming replicas are detected.Tests
DROP EXTENSION: all objects (including ones created directly in lolor) restored to native storage with data verification.migrate_to_native()without dropping the extension.migrate_from_nativeandmigrate_to_native.DROP EXTENSIONrejected on OID conflict, user resolves and retries successfully.