Revert delta patch removal#454
Open
mason-sharp wants to merge 11 commits into
Open
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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 ignored due to path filters (3)
📒 Files selected for processing (22)
💤 Files with no reviewable changes (6)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (11)
📝 WalkthroughWalkthroughThis PR migrates Spock's delta-apply configuration from security-label-based setup to per-column attribute reloptions across PostgreSQL 15–18, removes the security-label provider hook, updates DDL validation to reject SEC LABEL statements, and removes obsolete tests. ChangesDelta-Apply Configuration and Provider Migration
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)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
|---|---|
| Complexity | 0 |
| 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/spock_apply_heap.c (1)
764-805:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winLinker error when
NO_LOG_OLD_VALUEis defined.The call to
build_delta_tupleat line 785 is unconditional, but the function definition (lines 575-645) is wrapped in#ifndef NO_LOG_OLD_VALUE. WhenNO_LOG_OLD_VALUEis defined, this will cause an undefined reference linker error.The
if (rel->has_delta_columns)check is a runtime guard and does not prevent compilation/linking failures.🐛 Proposed fix: wrap the delta_columns block with compile-time guard
+#ifndef NO_LOG_OLD_VALUE if (rel->has_delta_columns) { SpockTupleData deltatup; HeapTuple currenttuple; /* * Depending on previous conflict resolution our final NEW tuple will * be based on either the incoming remote tuple or the existing local * one and then the delta processing on top of that. */ if (apply) { currenttuple = ExecFetchSlotHeapTuple(remoteslot, true, &clear_remoteslot); } else { currenttuple = ExecFetchSlotHeapTuple(localslot, true, &clear_localslot); } oldctx = MemoryContextSwitchTo(GetPerTupleMemoryContext(estate)); build_delta_tuple(rel, oldtup, newtup, &deltatup, localslot); if (!apply) { /* * We are overriding apply=false because of delta apply. */ apply = true; is_delta_apply = true; /* Count the DCA event in stats */ handle_stats_counter(rel->rel, MyApplyWorker->subid, SPOCK_STATS_DCA_COUNT, 1); } applytuple = heap_modify_tuple(currenttuple, RelationGetDescr(rel->rel), deltatup.values, deltatup.nulls, deltatup.changed); MemoryContextSwitchTo(oldctx); slot_store_htup(remoteslot, rel, applytuple); } +#endif /* NO_LOG_OLD_VALUE */🤖 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_apply_heap.c` around lines 764 - 805, The call to build_delta_tuple inside the rel->has_delta_columns block will fail to link when NO_LOG_OLD_VALUE is defined because the function is conditionally compiled; wrap the entire delta-processing block (the if (rel->has_delta_columns) { ... } that contains build_delta_tuple, is_delta_apply, handle_stats_counter, heap_modify_tuple and slot_store_htup calls) with the same compile-time guard used for the function definition (`#ifndef` NO_LOG_OLD_VALUE / `#endif`) or alternatively conditionally omit only the build_delta_tuple and dependent logic when NO_LOG_OLD_VALUE is defined so the code compiles and links cleanly.
🧹 Nitpick comments (2)
patches/17/pg17-015-attoptions.diff (1)
178-181: 💤 Low valueNote: original
idattrsfromRelationGetIndexAttrBitmapis overwritten bybms_union.The bitmap returned by
RelationGetIndexAttrBitmap(INDEX_ATTR_BITMAP_IDENTITY_KEY)is leaked whenidattrs = bms_union(idattrs, logged_old_attrs);reassigns. This is the prior behavior of the restored patch and is short-lived (freed at end of statement memory context), so no functional defect — flagging only as awareness for the future delta rework alongside the major PG release.🤖 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 `@patches/17/pg17-015-attoptions.diff` around lines 178 - 181, RelationGetIndexAttrBitmap(INDEX_ATTR_BITMAP_IDENTITY_KEY) returns a bitmap that must not be leaked; instead of overwriting idattrs with bms_union(...) directly, compute the union into a temporary Bitmapset, free the original idattrs, then assign the temp to idattrs (i.e. use bms_union to produce a new set, call bms_free on the prior idattrs, then set idattrs = <union result>); reference symbols: RelationGetIndexAttrBitmap, INDEX_ATTR_BITMAP_IDENTITY_KEY, idattrs, logged_old_attrs, bms_union, and bms_free.src/spock_relcache.c (1)
110-134: 💤 Low valueConsider using
OidIsValidfor clarity.At line 112, the check
aopt->delta_apply_function != 0works correctly butOidIsValid(aopt->delta_apply_function)would be more idiomatic for PostgreSQL code.♻️ Suggested improvement
- if (aopt != NULL && aopt->delta_apply_function != 0) + if (aopt != NULL && OidIsValid(aopt->delta_apply_function))🤖 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_relcache.c` around lines 110 - 134, Replace the non-idiomatic numeric check "aopt->delta_apply_function != 0" with the PostgreSQL helper OidIsValid(aopt->delta_apply_function) in the block that retrieves attribute options (see get_attribute_options and the aopt->delta_apply_function usage), leaving the rest of the logic (spock_lookup_delta_function, error elog, setting entry->delta_apply_functions) unchanged so the presence of a valid Oid is tested in the canonical way.
🤖 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 `@src/spock_apply_heap.c`:
- Around line 764-805: The call to build_delta_tuple inside the
rel->has_delta_columns block will fail to link when NO_LOG_OLD_VALUE is defined
because the function is conditionally compiled; wrap the entire delta-processing
block (the if (rel->has_delta_columns) { ... } that contains build_delta_tuple,
is_delta_apply, handle_stats_counter, heap_modify_tuple and slot_store_htup
calls) with the same compile-time guard used for the function definition
(`#ifndef` NO_LOG_OLD_VALUE / `#endif`) or alternatively conditionally omit only the
build_delta_tuple and dependent logic when NO_LOG_OLD_VALUE is defined so the
code compiles and links cleanly.
---
Nitpick comments:
In `@patches/17/pg17-015-attoptions.diff`:
- Around line 178-181:
RelationGetIndexAttrBitmap(INDEX_ATTR_BITMAP_IDENTITY_KEY) returns a bitmap that
must not be leaked; instead of overwriting idattrs with bms_union(...) directly,
compute the union into a temporary Bitmapset, free the original idattrs, then
assign the temp to idattrs (i.e. use bms_union to produce a new set, call
bms_free on the prior idattrs, then set idattrs = <union result>); reference
symbols: RelationGetIndexAttrBitmap, INDEX_ATTR_BITMAP_IDENTITY_KEY, idattrs,
logged_old_attrs, bms_union, and bms_free.
In `@src/spock_relcache.c`:
- Around line 110-134: Replace the non-idiomatic numeric check
"aopt->delta_apply_function != 0" with the PostgreSQL helper
OidIsValid(aopt->delta_apply_function) in the block that retrieves attribute
options (see get_attribute_options and the aopt->delta_apply_function usage),
leaving the rest of the logic (spock_lookup_delta_function, error elog, setting
entry->delta_apply_functions) unchanged so the presence of a valid Oid is tested
in the canonical way.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 86d20554-645c-4a37-a1eb-0f38188fb106
⛔ Files ignored due to path filters (3)
tests/regress/expected/autoddl.outis excluded by!**/*.outtests/regress/expected/basic.outis excluded by!**/*.outtests/regress/expected/infofuncs.outis excluded by!**/*.out
📒 Files selected for processing (21)
Makefileinclude/spock.hinclude/spock_relcache.hpatches/15/pg15-015-attoptions.diffpatches/16/pg16-015-attoptions.diffpatches/17/pg17-015-attoptions.diffpatches/18/pg18-015-attoptions.diffsql/spock--5.0.6--6.0.0-devel.sqlsql/spock--6.0.0-devel.sqlsrc/spock.csrc/spock_apply.csrc/spock_apply_heap.csrc/spock_autoddl.csrc/spock_executor.csrc/spock_relcache.csrc/spock_repset.ctests/regress/sql/autoddl.sqltests/regress/sql/basic.sqltests/regress/sql/infofuncs.sqltests/tap/scheduletests/tap/t/012_delta_apply.pl
💤 Files with no reviewable changes (7)
- tests/regress/sql/infofuncs.sql
- include/spock.h
- tests/tap/t/012_delta_apply.pl
- tests/tap/schedule
- tests/regress/sql/basic.sql
- src/spock.c
- sql/spock--5.0.6--6.0.0-devel.sql
mason-sharp
force-pushed
the
revert-delta-patch-removal
branch
from
5e33c19 to
519ab81
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/spock_executor.c (1)
198-272:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftKeep a compatibility cleanup path for legacy Spock security labels.
Line 243 now returns immediately for Spock-owned drops, and this patch also removes the old post-alter cleanup path. If an upgraded cluster still has
SECURITY LABEL FOR spockentries from the previous delta implementation, this leaves no scrub path for that catalog state on drop/alter anymore. Please either retain the legacy cleanup until a catalog migration removes those labels, or add that migration in the same change; otherwise this revert is not upgrade-safe.🤖 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_executor.c` around lines 198 - 272, The early return for Spock-owned drops in spock_object_access (triggered by dropping_spock_obj) removes the legacy cleanup path for old "SECURITY LABEL FOR spock" entries; restore a compatibility cleanup by either calling a cleanup helper (e.g., implement/invoke spock_scrub_legacy_security_labels(&object) or similar) before returning when dropping_spock_obj is true, or add a catalog migration (e.g., spock_migrate_remove_security_labels) in the same change to remove those labels during upgrade; ensure the code path is reachable from spock_object_access so legacy labels are scrubbed on DROP/ALTER.
🤖 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_relcache.c`:
- Around line 99-133: The loop writes delta_apply_functions using local
attribute indexes (entry->attmap[i]) but the array was allocated for the remote
column count, so before scanning the local TupleDesc (RelationGetDescr(desc))
resize/allocate or memset-zero entry->delta_apply_functions to the local number
of attributes (TupleDesc->natts) and set entry->has_delta_columns = false; then
in the loop, when you find a valid aopt and resolve dfunc via
spock_lookup_delta_function(TupleDescAttr(...)->atttypid) store it into
entry->delta_apply_functions at the local column index (entry->attmap[i]) and
set entry->has_delta_columns = true only when a function is actually stored;
also ensure any leftover entries from previous cache state are cleared so stale
Oids cannot remain (i.e., zero the entire local-width array before recomputing).
---
Outside diff comments:
In `@src/spock_executor.c`:
- Around line 198-272: The early return for Spock-owned drops in
spock_object_access (triggered by dropping_spock_obj) removes the legacy cleanup
path for old "SECURITY LABEL FOR spock" entries; restore a compatibility cleanup
by either calling a cleanup helper (e.g., implement/invoke
spock_scrub_legacy_security_labels(&object) or similar) before returning when
dropping_spock_obj is true, or add a catalog migration (e.g.,
spock_migrate_remove_security_labels) in the same change to remove those labels
during upgrade; ensure the code path is reachable from spock_object_access so
legacy labels are scrubbed on DROP/ALTER.
🪄 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: 948e6c42-9137-4f31-b7c7-94847c9f3b92
⛔ Files ignored due to path filters (3)
tests/regress/expected/autoddl.outis excluded by!**/*.outtests/regress/expected/basic.outis excluded by!**/*.outtests/regress/expected/infofuncs.outis excluded by!**/*.out
📒 Files selected for processing (21)
Makefileinclude/spock.hinclude/spock_relcache.hpatches/15/pg15-015-attoptions.diffpatches/16/pg16-015-attoptions.diffpatches/17/pg17-015-attoptions.diffpatches/18/pg18-015-attoptions.diffsql/spock--5.0.6--6.0.0-devel.sqlsql/spock--6.0.0-devel.sqlsrc/spock.csrc/spock_apply.csrc/spock_apply_heap.csrc/spock_autoddl.csrc/spock_executor.csrc/spock_relcache.csrc/spock_repset.ctests/regress/sql/autoddl.sqltests/regress/sql/basic.sqltests/regress/sql/infofuncs.sqltests/tap/scheduletests/tap/t/012_delta_apply.pl
💤 Files with no reviewable changes (7)
- tests/tap/t/012_delta_apply.pl
- tests/tap/schedule
- include/spock.h
- tests/regress/sql/basic.sql
- src/spock.c
- sql/spock--5.0.6--6.0.0-devel.sql
- tests/regress/sql/infofuncs.sql
🚧 Files skipped from review as they are similar to previous changes (12)
- src/spock_apply.c
- include/spock_relcache.h
- Makefile
- tests/regress/sql/autoddl.sql
- src/spock_apply_heap.c
- src/spock_repset.c
- sql/spock--6.0.0-devel.sql
- src/spock_autoddl.c
- patches/18/pg18-015-attoptions.diff
- patches/15/pg15-015-attoptions.diff
- patches/17/pg17-015-attoptions.diff
- patches/16/pg16-015-attoptions.diff
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_relcache.c`:
- Around line 126-161: The code assumes the reloption delta_apply_function
exists in PostgreSQL; detect and warn when it is not registered so the user
knows to apply the Postgres-version patches: modify the loop in spock_relcache.c
(around tupdesc_get_att_by_name, get_attribute_options and the
delta_apply_function check) to explicitly log a clear WARNING or ERROR when aopt
is NULL or aopt->delta_apply_function == 0 (e.g., elog(WARNING, "SPOCK:
delta_apply_function reloption not registered; apply
spock/patches/Postgres-version/..."), mentioning the rel and column using
entry->nspname, entry->relname, entry->attnames[i]) so missing Postgres patches
are surfaced instead of silently skipping delta registration; keep existing
behavior for when delta_apply_function is present and continue to call
GET_STRING_RELOPTION, spock_lookup_delta_function and set
entry->delta_apply_functions and entry->has_delta_columns as before.
🪄 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: 92971dd9-39e0-4205-939b-7e521083ad37
📒 Files selected for processing (1)
src/spock_relcache.c
This reverts commit 1bea9e8.
This reverts commit 7d9337b.
…lass citizen." This reverts commit 730cac0.
This reverts commit 589b1af.
This reverts commit 43e3857.
This reverts commit 9bc41d7.
This reverts commit 31466f7.
Skip HeapDetermineLogOldColumns() for catalog relations to avoid assertion failure during initdb when updating catalog tables. The fix adds IsCatalogRelationOid check before calling HeapDetermineLogOldColumns(), same as already done for PG18. (cherry picked from commit 782d078)
Pointed out by Coderabbit
mason-sharp
force-pushed
the
revert-delta-patch-removal
branch
from
c6d6e44 to
c4e48f2
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Do later in conjuction with a major Postgres release instead.