Revert delta patch removal#454
Draft
mason-sharp wants to merge 9 commits into
Draft
Conversation
📝 WalkthroughWalkthroughThis PR replaces security-label-based delta configuration with per-attribute reloptions ( ChangesSecurity Label Infrastructure Removal
Attribute Options & Delta Apply Configuration
Heap Update & Replica Identity Integration
Delta Apply Computation & Build Flag
SQL Procedures & Test Cleanup
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)
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.
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
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)
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
Comment on lines
+99
to
+133
| desc = RelationGetDescr(entry->rel); | ||
| for (i = 0; i < entry->natts; i++) | ||
| { | ||
| ObjectAddress object; | ||
| char *seclabel; | ||
| TupleDesc desc; | ||
| AttributeOpts *aopt; | ||
|
|
||
| desc = RelationGetDescr(entry->rel); | ||
| entry->attmap[i] = tupdesc_get_att_by_name(desc, entry->attnames[i]); | ||
|
|
||
| if (entry->rel->rd_rel->relreplident != REPLICA_IDENTITY_FULL) | ||
| continue; | ||
|
|
||
| /* | ||
| * Read security labels for each attname. For each such an attribute | ||
| * choose corresponding delta function. | ||
| * | ||
| * XXX: What about non-existing columns on remote side? | ||
| * If we find attribute options for this column and the | ||
| * delta_apply_function is set, lookup the oid for it. | ||
| */ | ||
| ObjectAddressSubSet(object, RelationRelationId, | ||
| RelationGetRelid(entry->rel), | ||
| entry->attmap[i] + 1); | ||
| seclabel = GetSecurityLabel(&object, SPOCK_SECLABEL_PROVIDER); | ||
| if (seclabel != NULL) | ||
| aopt = get_attribute_options(entry->rel->rd_id, | ||
| entry->attmap[i] + 1); | ||
| if (aopt != NULL && aopt->delta_apply_function != 0) | ||
| { | ||
| Form_pg_attribute att; | ||
| Oid dfunc; | ||
| char *fname; | ||
| Form_pg_attribute att; | ||
| Oid dfunc; | ||
|
|
||
| att = TupleDescAttr(desc, entry->attmap[i]); | ||
| dfunc = spock_lookup_delta_function(seclabel, att->atttypid); | ||
| fname = pstrdup(GET_STRING_RELOPTION(aopt, | ||
| delta_apply_function)); | ||
| dfunc = spock_lookup_delta_function(fname, att->atttypid); | ||
| pfree(fname); | ||
|
|
||
| if (dfunc == InvalidOid) | ||
| elog(ERROR, "SPOCK: column %s.%s.%s is configured for " | ||
| "delta_apply function %s - function not found", | ||
| "delta_apply_function %s - function not found", | ||
| entry->nspname, entry->relname, | ||
| entry->attnames[i], seclabel); | ||
| entry->attnames[i], | ||
| GET_STRING_RELOPTION(aopt, delta_apply_function)); | ||
|
|
||
|
|
||
| entry->delta_apply_functions[entry->attmap[i]] = dfunc; | ||
| Assert(entry->delta_apply_functions[entry->attmap[i]] != InvalidOid); | ||
| entry->has_delta_columns = true; | ||
| } | ||
| else | ||
| { | ||
| /* Main case */ | ||
| entry->delta_apply_functions[entry->attmap[i]] = InvalidOid; | ||
| entry->delta_apply_functions[entry->attmap[i]] = dfunc; |
There was a problem hiding this comment.
Rebuild delta metadata from a clean, local-width buffer.
delta_apply_functions is allocated with the remote column count in the cache update paths, but this loop writes it using the local attribute index (entry->attmap[i]). That can go out of bounds as soon as the local table has dropped or extra columns. Also, on relcache invalidation we only reset reloid, so removed/changed column options can leave stale function Oids and has_delta_columns behind on the next open.
Please resize/zero this array from the local TupleDesc before the scan, then recompute has_delta_columns from scratch.
Also applies to: 211-211, 249-249
🧰 Tools
🪛 Cppcheck (2.20.0)
[error] 126-126: There is an unknown macro here somewhere. Configuration is required. If pg_attribute_printf is a macro then please configure it.
(unknownMacro)
🤖 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 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).
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.