Skip to content

Re-add delta apply via security label commits#489

Open
mason-sharp wants to merge 10 commits into
mainfrom
re-add-delta-apply-via-sec-label
Open

Re-add delta apply via security label commits#489
mason-sharp wants to merge 10 commits into
mainfrom
re-add-delta-apply-via-sec-label

Conversation

@mason-sharp

Copy link
Copy Markdown
Member

These were previously removed, now added back, however the patches to PG 15 - 18 will remain. That way, Spock 5.x and 6.x will continue to work against the same single version of Postgres; the existing patched delta apply code is simply unused if using Spock 6.

…ches)

Revert of e38e87b. Re-applies the security-label code changes from the
original "Remove the core attribute options" commit, but intentionally
RETAINS the four pg*-015-attoptions.diff patches so old and new Spock can
build against the same patched PostgreSQL. The core patch is left applied
but unused by the security-label path (dormant).

The relcache-invalidation reset (post-#454 fix 3886058) is preserved here,
ported onto the security-label delta_functions / has_delta_apply fields
(renamed to delta_apply_functions / has_delta_columns by a later commit).
@mason-sharp mason-sharp requested a review from danolivo June 3, 2026 04:29
@codacy-production

codacy-production Bot commented Jun 3, 2026

Copy link
Copy Markdown

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 0 duplication

Metric Results
Duplication 0

View in Codacy

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

coderabbitai Bot commented Jun 3, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 9538db66-326c-484e-aedd-06ff74a190dc

📥 Commits

Reviewing files that changed from the base of the PR and between ada61d6 and 79d46a4.

📒 Files selected for processing (7)
  • sql/spock--5.0.8--6.0.0.sql
  • sql/spock--6.0.0.sql
  • src/spock.c
  • src/spock_apply_heap.c
  • src/spock_autoddl.c
  • src/spock_relcache.c
  • src/spock_repset.c
🚧 Files skipped from review as they are similar to previous changes (6)
  • src/spock_repset.c
  • src/spock_apply_heap.c
  • src/spock_autoddl.c
  • src/spock.c
  • sql/spock--5.0.8--6.0.0.sql
  • src/spock_relcache.c

📝 Walkthrough

Walkthrough

Replaces attribute-option delta markers with PostgreSQL security labels: provider hook, spock.delta_apply SQL, relcache label-based detection, apply/autoddl/rep set and executor integration, unconditional delta-tuple build with non-NULL enforcement, and tests.

Changes

Delta-Apply via Security Labels

Layer / File(s) Summary
Core security label provider infrastructure
include/spock.h, src/spock.c
Defines SPOCK_SECLABEL_PROVIDER, adds seclabel includes, implements spock_object_relabel label-provider hook that validates label targets, and registers the provider during _PG_init.
Delta-apply relcache and replication identity lookup
include/spock_relcache.h, src/spock_relcache.c, src/spock_repset.c
Reorders SpockRelation cached fields, switches delta-apply detection from attribute options to per-column GetSecurityLabel (SPOCK provider), sizes delta_apply_functions by entry->natts, and adds get_replication_identity() used by repset checks.
Delta-apply SQL function implementations
sql/spock--5.0.8--6.0.0.sql, sql/spock--6.0.0.sql
Adds spock.delta_apply(rel, att_name, to_drop boolean DEFAULT false) PL/pgSQL function and reformats numeric C-function declarations; the PL/pgSQL overload validates replica identity and column properties, applies/removes SECURITY LABEL FOR spock ON COLUMN ..., warns about DDL replication config, and toggles replica identity to refresh relcache.
Delta tuple building with NULL validation
src/spock_apply_heap.c, Makefile
Removes NO_LOG_OLD_VALUE compile guard so build_delta_tuple() is always compiled; changes delta logic to error when remote old or remote new or the local base are NULL and compute deltas only when inputs are non-NULL.
Executor, AutoDDL and rep-set integration
src/spock_apply.c, src/spock_autoddl.c, src/spock_executor.c, src/spock_repset.c
Allows SecLabelStmt in replicate_ddl() gate; AutoDDL permits SecLabelStmt and uses get_replication_identity() when gating add-to-repset; executor adds DeleteSecurityLabels() and OAT_POST_ALTER handling to remove spock labels on alter/drop; repset uses get_replication_identity() for UPDATE/DELETE validation.
Regression and TAP test coverage
tests/regress/sql/basic.sql, tests/regress/sql/autoddl.sql, tests/regress/sql/infofuncs.sql, tests/tap/schedule, tests/tap/t/012_delta_apply.pl, tests/docker/Dockerfile-step-1.el9
Adds SQL regression blocks and a TAP test that exercise spock.delta_apply label application/removal, replica identity interactions, nullable-column restrictions, extension-drop cleanup, a 3-node concurrent-update scenario, and updates the Spockbench clone branch used in the Dockerfile step.

Poem

🐰 I nibbled labels in the sod,
Marked delta fields with gentle nod,
Relcache wakes, the hooks applaud,
Tests hop by — no NULLs allowed,
Hooray — the garden's tidy, plod by plod.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 63.16% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely summarizes the main change: reintroducing delta apply functionality via security labels.
Description check ✅ Passed The description is related to the changeset, explaining that delta_apply via security labels is being reintroduced while existing PG patches remain.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch re-add-delta-apply-via-sec-label

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_apply_heap.c

src/spock_apply_heap.c:15:10: fatal error: 'postgres.h' file not found
15 | #include "postgres.h"
| ^~~~~~~~~~~~
1 error generated.
Error: the following clang command did not run successfully:
/opt/infer-linux-x86_64-v1.2.0/lib/infer/facebook-clang-plugins/clang/install/bin/clang-18
@/tmp/coderabbit-infer/79d46a4cdde09c255f4bba60f4e7e36f5c946641-a81cdd017527b9d1/tmp/clang_command_.tmp.0f1bbe.txt
++Contents of '/tmp/coderabbit-infer/79d46a4cdde09c255f4bba60f4e7e36f5c946641-a81cdd017527b9d1/tmp/clang_command_.tmp.0f1bbe.txt':
"-cc1" "-load"
"/opt/infer-linux-x86_64-v1.2.0/lib/infer/infer/bin/../../facebook-clang-plugins/libtooling/build/FacebookClangPlugin.dylib"
"-add-plugin" "BiniouASTExporter" "-plugin-arg-BiniouASTExporter" "-"
"-plugin-arg-BiniouASTExporter" "PREPEND_CURRENT_DIR=1"
"-plugin-arg-BiniouASTExporter" "MAX_STRING_SIZE=65535" "-cc1" "-triple"
"x86_64-unknown-linux-gnu" "-emit-obj" "-mrelax-all" "-disable-free"
"-clea

... [truncated 698 characters] ...

/infer-linux-x86_64-v1.2.0/lib/infer/facebook-clang-plugins/clang/install/lib/clang/18/include"
"-internal-isystem" "/usr/local/include" "-internal-isystem"
"/usr/lib/gcc/x86_64-linux-gnu/12/../../../../x86_64-linux-gnu/include"
"-internal-externc-isystem" "/usr/include/x86_64-linux-gnu"
"-internal-externc-isystem" "/include" "-internal-externc-isystem"
"/usr/include" "-Wno-ignored-optimization-argument" "-Wno-everything"
"-ferror-limit" "19" "-fgnuc-version=4.2.1" "-fskip-odr-check-in-gmf"
"-D__GCC_HAVE_DWARF2_CFI_ASM=1" "-o"
"/tmp/coderabbit-infer/a81cdd017527b9d1/file.o" "-x" "c"
"src/spock_apply_heap.c" "-O0" "-fno-builtin" "-include"
"/opt/infer-linux-x86_64-v1.2.0/lib/infer/infer/bin/../lib/clang_wrappers/global_defines.h"
"-Wno-everything"

src/spock.c

src/spock.c:12:10: fatal error: 'postgres.h' file not found
12 | #include "postgres.h"
| ^~~~~~~~~~~~
1 error generated.
Error: the following clang command did not run successfully:
/opt/infer-linux-x86_64-v1.2.0/lib/infer/facebook-clang-plugins/clang/install/bin/clang-18
@/tmp/coderabbit-infer/79d46a4cdde09c255f4bba60f4e7e36f5c946641-08e66d61c01647e3/tmp/clang_command_.tmp.a2a77f.txt
++Contents of '/tmp/coderabbit-infer/79d46a4cdde09c255f4bba60f4e7e36f5c946641-08e66d61c01647e3/tmp/clang_command_.tmp.a2a77f.txt':
"-cc1" "-load"
"/opt/infer-linux-x86_64-v1.2.0/lib/infer/infer/bin/../../facebook-clang-plugins/libtooling/build/FacebookClangPlugin.dylib"
"-add-plugin" "BiniouASTExporter" "-plugin-arg-BiniouASTExporter" "-"
"-plugin-arg-BiniouASTExporter" "PREPEND_CURRENT_DIR=1"
"-plugin-arg-BiniouASTExporter" "MAX_STRING_SIZE=65535" "-cc1" "-triple"
"x86_64-unknown-linux-gnu" "-emit-obj" "-mrelax-all" "-disable-free"
"-clear-ast-befor

... [truncated 661 characters] ...

"
"/opt/infer-linux-x86_64-v1.2.0/lib/infer/facebook-clang-plugins/clang/install/lib/clang/18/include"
"-internal-isystem" "/usr/local/include" "-internal-isystem"
"/usr/lib/gcc/x86_64-linux-gnu/12/../../../../x86_64-linux-gnu/include"
"-internal-externc-isystem" "/usr/include/x86_64-linux-gnu"
"-internal-externc-isystem" "/include" "-internal-externc-isystem"
"/usr/include" "-Wno-ignored-optimization-argument" "-Wno-everything"
"-ferror-limit" "19" "-fgnuc-version=4.2.1" "-fskip-odr-check-in-gmf"
"-D__GCC_HAVE_DWARF2_CFI_ASM=1" "-o"
"/tmp/coderabbit-infer/08e66d61c01647e3/file.o" "-x" "c" "src/spock.c"
"-O0" "-fno-builtin" "-include"
"/opt/infer-linux-x86_64-v1.2.0/lib/infer/infer/bin/../lib/clang_wrappers/global_defines.h"
"-Wno-everything"

src/spock_autoddl.c

src/spock_autoddl.c:12:10: fatal error: 'postgres.h' file not found
12 | #include "postgres.h"
| ^~~~~~~~~~~~
1 error generated.
Error: the following clang command did not run successfully:
/opt/infer-linux-x86_64-v1.2.0/lib/infer/facebook-clang-plugins/clang/install/bin/clang-18
@/tmp/coderabbit-infer/79d46a4cdde09c255f4bba60f4e7e36f5c946641-a44352798d8c4c01/tmp/clang_command_.tmp.6b96b6.txt
++Contents of '/tmp/coderabbit-infer/79d46a4cdde09c255f4bba60f4e7e36f5c946641-a44352798d8c4c01/tmp/clang_command_.tmp.6b96b6.txt':
"-cc1" "-load"
"/opt/infer-linux-x86_64-v1.2.0/lib/infer/infer/bin/../../facebook-clang-plugins/libtooling/build/FacebookClangPlugin.dylib"
"-add-plugin" "BiniouASTExporter" "-plugin-arg-BiniouASTExporter" "-"
"-plugin-arg-BiniouASTExporter" "PREPEND_CURRENT_DIR=1"
"-plugin-arg-BiniouASTExporter" "MAX_STRING_SIZE=65535" "-cc1" "-triple"
"x86_64-unknown-linux-gnu" "-emit-obj" "-mrelax-all" "-disable-free"
"-clear-a

... [truncated 689 characters] ...

opt/infer-linux-x86_64-v1.2.0/lib/infer/facebook-clang-plugins/clang/install/lib/clang/18/include"
"-internal-isystem" "/usr/local/include" "-internal-isystem"
"/usr/lib/gcc/x86_64-linux-gnu/12/../../../../x86_64-linux-gnu/include"
"-internal-externc-isystem" "/usr/include/x86_64-linux-gnu"
"-internal-externc-isystem" "/include" "-internal-externc-isystem"
"/usr/include" "-Wno-ignored-optimization-argument" "-Wno-everything"
"-ferror-limit" "19" "-fgnuc-version=4.2.1" "-fskip-odr-check-in-gmf"
"-D__GCC_HAVE_DWARF2_CFI_ASM=1" "-o"
"/tmp/coderabbit-infer/a44352798d8c4c01/file.o" "-x" "c"
"src/spock_autoddl.c" "-O0" "-fno-builtin" "-include"
"/opt/infer-linux-x86_64-v1.2.0/lib/infer/infer/bin/../lib/clang_wrappers/global_defines.h"
"-Wno-everything"

  • 2 others

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 10

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
src/spock_apply_heap.c (1)

531-543: ⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

Initialize deltatup->changed for the full local descriptor.

heap_modify_tuple() consumes desc->natts change flags, but this function only assigns changed[] inside the rel->natts loop. When the subscriber has extra local columns, the remaining entries stay as stack garbage and can make random local-only columns look updated.

Suggested fix
 	memset(deltatup->values, 0, tupdesc->natts * sizeof(Datum));
 	memset(deltatup->nulls, 1, tupdesc->natts * sizeof(bool));
+	memset(deltatup->changed, 0, tupdesc->natts * sizeof(bool));
Based on learnings: the "local has more columns than remote" case is intentionally supported by sizing delta metadata to `desc->natts`, so all `desc->natts` flags must be initialized.
🤖 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 531 - 543, The deltatup->changed array
is not initialized for the full local tuple descriptor (tupdesc->natts), causing
garbage flags for local-only columns; before the attidx loop (near the existing
memset calls for deltatup->values and deltatup->nulls) initialize all change
flags to false for the entire descriptor (e.g., memset(deltatup->changed, 0,
tupdesc->natts * sizeof(bool))) so heap_modify_tuple() sees deterministic change
flags.
src/spock_executor.c (1)

257-284: ⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

Don't purge all Spock security labels on ordinary drops inside schema spock.

dropping_spock_obj becomes true for any relation under schema spock, and this branch then deletes every provider='spock' row from pg_seclabel before returning. The upgrade script in sql/spock--5.0.8--6.0.0.sql drops spock.lag_tracker and spock.progress, so upgrading will silently strip delta_apply labels from user tables. Limit the global cleanup to dropping the extension itself, or delete labels scoped to the dropped object.

🤖 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 257 - 284, The code currently treats any
object in the spock namespace as dropping_spock_obj and calls
DeleteSecurityLabels(SPOCK_SECLABEL_PROVIDER), which removes all spock-scoped
labels globally; change this so DeleteSecurityLabels is only invoked when the
extension itself is being dropped (detect via classId == ExtensionRelationId and
objectId == get_extension_oid(EXTENSION_NAME, true)), and for ordinary objects
in the spock schema (detected via classId == RelationRelationId and namespace
comparison using get_namespace_oid and get_rel_namespace) instead remove only
labels for that specific object by calling the appropriate per-object label
removal API (e.g. DeleteSecurityLabel or the API that accepts classId/objectId
and SPOCK_SECLABEL_PROVIDER) rather than DeleteSecurityLabels.
🧹 Nitpick comments (1)
tests/tap/t/012_delta_apply.pl (1)

63-71: ⚡ Quick win

Use $pg_bin for the backgrounded psql instead of relying on PATH.

$pg_bin is captured (Line 21) and used elsewhere via the helpers, but this IPC::Run invocation calls a bare psql. On a host with multiple PostgreSQL installs this can resolve to a different binary/version than the cluster under test, causing spurious failures.

♻️ Proposed change
 my $phandle1 = IPC::Run::start(
 	[
-		'psql',
+		"$pg_bin/psql",
 		'-c', "CALL counter_change('t1', 'x', -1, 10000)",
 		'-h', $host, '-p', $node_ports->[1], '-U', $db_user, $dbname
 	],
 	'>' => \$pgbench_stdout1,
 	'2>' => \$pgbench_stderr1);

Confirm psql_or_bail/scalar_query in SpockTest.pm build their command from $pg_bin so this aligns with the rest of the suite.

🤖 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/012_delta_apply.pl` around lines 63 - 71, The backgrounded
IPC::Run::start invocation uses a bare 'psql' which can pick the wrong
PostgreSQL binary; change the command array to use the captured $pg_bin path
(the same way psql_or_bail/scalar_query in SpockTest.pm do) by replacing the
literal 'psql' with the $pg_bin variable (e.g., "$pg_bin/psql" or $pg_bin .
'/psql') so IPC::Run::start invokes the exact psql binary for the cluster under
test.
🤖 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 `@sql/spock--5.0.8--6.0.0.sql`:
- Around line 239-248: The current check in the IF block only permits DEFAULT
('d') or FULL ('f') replica identities and then raises if not, which allows
pre-existing FULL tables to be accepted and later overwritten by
spock.delta_apply(..., true); modify the logic around relreplident checks (and
the related validation code at the other occurrence around lines referenced) to
reject tables with pre-existing FULL identity unless they already have a Spock
label, or alternatively persist the prior state and restore it when removing the
label; specifically, inspect the table's labels/metadata where Spock marks
tables (the same mechanism used by spock.delta_apply) and only allow
relreplident = 'f' when that Spock label is present, otherwise raise the
exception and/or record the original identity to be restored on label removal.
- Around line 289-295: The relation name must be properly quoted when building
the SECURITY LABEL statement: stop using %I with the regclass value `rel` (it
produces a single quoted token like "schema.table"); instead build a
schema-qualified, safely quoted name from `rel::text` by splitting into schema
and table and quoting each identifier (e.g. quote_ident(split_part(rel::text,
'.', 1)) || '.' || quote_ident(split_part(rel::text, '.', 2))) and use that
string in the formatted SQL assigned to `sqlstring` for the SECURITY LABEL
branch (the same change must be applied to the similar block referenced at lines
316-323). Ensure `att_name` continues to be used with %I or otherwise quoted
safely.

In `@sql/spock--6.0.0.sql`:
- Around line 935-941: The relation identifier `rel` is a regclass and must be
properly schema- and table-quoted instead of using %I on `rel` directly; change
the SQL construction to split rel::text into schema and table, quote each with
quote_ident and build a qualified name (e.g. quote_ident(split_part(rel::text,
'.', 1)) || '.' || quote_ident(split_part(rel::text, '.', 2))) and use that
qualified string in the format call (use %s for the qualified name and %I for
att_name); apply the same fix for the other occurrence referenced (lines
962-969) so sqlstring is built with correctly quoted schema.table and column
identifiers.
- Around line 885-894: The current check unconditionally rejects tables with
relreplident='f', which causes tables that were originally CREATED WITH REPLICA
IDENTITY FULL to lose their user intent when spock.delta_apply(..., true) is
later removed. Modify the logic around the relreplident check (the IF
relreplident <> 'd' AND relreplident <> 'f' ... RAISE EXCEPTION block and the
code paths invoked by spock.delta_apply) so that: 1) pre-existing FULL tables
are rejected only if they do not already carry a Spock label (detect existing
Spock metadata/label for the table), and 2) when enabling delta_apply for a
table that was FULL before Spock touched it, persist the prior replica identity
(store relreplident) in Spock metadata and ensure the disable/remove path
restores that persisted state; update the error text in RAISE EXCEPTION to
mention the need for a Spock label when encountering pre-existing FULL.

In `@src/spock_apply_heap.c`:
- Around line 563-569: The code currently asserts loc_isnull after reading
loc_value with heap_getattr, but in non-assert builds that check vanishes; add
an explicit null check for loc_isnull before calling OidFunctionCall3Coll and
raise the same error path used for remote NULLs (ERRCODE_NULL_VALUE_NOT_ALLOWED)
when the local base value is NULL. Locate heap_getattr/TTS_TUP(localslot) and
the subsequent OidFunctionCall3Coll call that uses
rel->delta_apply_functions[remoteattnum], and if loc_isnull construct an
error/ereport with ERRCODE_NULL_VALUE_NOT_ALLOWED and a clear message
identifying the offending attribute (use the tuple descriptor/tupdesc and
attribute number remoteattnum to include the attribute name) so the function is
never invoked with an invalid Datum.

In `@src/spock_autoddl.c`:
- Around line 347-349: The current check lets every SecLabelStmt pass through,
so change the condition to only allow SECURITY LABEL statements that are for
Spock by additionally testing that castNode(SecLabelStmt, parsetree)->provider
equals SPOCK_SECLABEL_PROVIDER; i.e., keep the existing
GetCommandLogLevel(parsetree) != LOGSTMT_DDL check and replace the plain
IsA(parsetree, SecLabelStmt) allowance with a compound check that IsA(parsetree,
SecLabelStmt) && strcmp(castNode(SecLabelStmt, parsetree)->provider,
SPOCK_SECLABEL_PROVIDER) == 0 (or equivalent provider-equality test), so only
Spock-owned security labels are whitelisted.

In `@src/spock_relcache.c`:
- Around line 462-495: get_replication_identity currently scans every local
attribute for a security label, but spock_relation_open only treats
remote-mapped columns as delta candidates, causing inconsistent behavior; change
get_replication_identity to consult the same mapped-column predicate used by
spock_relation_open (i.e., only consider attributes that are mapped/replicated)
and ignore labels on local-only columns so that the function
(get_replication_identity) returns rel->rd_pkindex only when a mapped attribute
has the SPOCK_SECLABEL_PROVIDER label, keeping the decision consistent with
spock_relation_open and the has_delta_columns logic.
- Around line 142-168: In spock_relation_open(), after getting seclabel =
GetSecurityLabel(&object, SPOCK_SECLABEL_PROVIDER) and using it to lookup the
delta function (via spock_lookup_delta_function) and assigning
entry->delta_apply_functions[...] and entry->has_delta_columns = true, free the
seclabel allocation (e.g., pfree(seclabel)) on the success path before leaving
the non-NULL branch to avoid leaking memory; keep the existing behavior when
seclabel is NULL. Ensure you free only when seclabel != NULL and do so
immediately after you no longer need its value (after the
spock_lookup_delta_function call and related checks) inside
spock_relation_open().

In `@src/spock_repset.c`:
- Around line 1134-1135: The alter_replication_set path still checks
targetrel->rd_replidindex directly (causing inconsistency with add-table flow);
update alter_replication_set to use get_replication_identity(targetrel) for
identity validation (the same helper used by the add-table path) and
remove/replace any checks against targetrel->rd_replidindex so delta-apply and
security-label tables are validated consistently via
get_replication_identity(...).

In `@src/spock.c`:
- Around line 978-1002: spock_object_relabel currently only checks classId,
objectSubId and existence but not the relation type; update spock_object_relabel
to, when object->classId == RelationRelationId, fetch the relation kind (e.g.
via get_rel_relkind(object->objectId) or by opening the relation and checking
rd_rel->relkind) and reject with an error unless relkind == RELKIND_RELATION
(plain table). Ensure the error uses the existing elog/ereport pattern and
references the same context as the current checks.

---

Outside diff comments:
In `@src/spock_apply_heap.c`:
- Around line 531-543: The deltatup->changed array is not initialized for the
full local tuple descriptor (tupdesc->natts), causing garbage flags for
local-only columns; before the attidx loop (near the existing memset calls for
deltatup->values and deltatup->nulls) initialize all change flags to false for
the entire descriptor (e.g., memset(deltatup->changed, 0, tupdesc->natts *
sizeof(bool))) so heap_modify_tuple() sees deterministic change flags.

In `@src/spock_executor.c`:
- Around line 257-284: The code currently treats any object in the spock
namespace as dropping_spock_obj and calls
DeleteSecurityLabels(SPOCK_SECLABEL_PROVIDER), which removes all spock-scoped
labels globally; change this so DeleteSecurityLabels is only invoked when the
extension itself is being dropped (detect via classId == ExtensionRelationId and
objectId == get_extension_oid(EXTENSION_NAME, true)), and for ordinary objects
in the spock schema (detected via classId == RelationRelationId and namespace
comparison using get_namespace_oid and get_rel_namespace) instead remove only
labels for that specific object by calling the appropriate per-object label
removal API (e.g. DeleteSecurityLabel or the API that accepts classId/objectId
and SPOCK_SECLABEL_PROVIDER) rather than DeleteSecurityLabels.

---

Nitpick comments:
In `@tests/tap/t/012_delta_apply.pl`:
- Around line 63-71: The backgrounded IPC::Run::start invocation uses a bare
'psql' which can pick the wrong PostgreSQL binary; change the command array to
use the captured $pg_bin path (the same way psql_or_bail/scalar_query in
SpockTest.pm do) by replacing the literal 'psql' with the $pg_bin variable
(e.g., "$pg_bin/psql" or $pg_bin . '/psql') so IPC::Run::start invokes the exact
psql binary for the cluster under test.
🪄 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: edadbea3-978f-4aa2-ab53-96a99ad24e6c

📥 Commits

Reviewing files that changed from the base of the PR and between 94f4ad6 and 44b4c98.

⛔ Files ignored due to path filters (3)
  • tests/regress/expected/autoddl.out is excluded by !**/*.out
  • tests/regress/expected/basic.out is excluded by !**/*.out
  • tests/regress/expected/infofuncs.out is excluded by !**/*.out
📒 Files selected for processing (17)
  • Makefile
  • include/spock.h
  • include/spock_relcache.h
  • sql/spock--5.0.8--6.0.0.sql
  • sql/spock--6.0.0.sql
  • src/spock.c
  • src/spock_apply.c
  • src/spock_apply_heap.c
  • src/spock_autoddl.c
  • src/spock_executor.c
  • src/spock_relcache.c
  • src/spock_repset.c
  • tests/regress/sql/autoddl.sql
  • tests/regress/sql/basic.sql
  • tests/regress/sql/infofuncs.sql
  • tests/tap/schedule
  • tests/tap/t/012_delta_apply.pl
💤 Files with no reviewable changes (1)
  • Makefile

Comment thread sql/spock--5.0.8--6.0.0.sql
Comment on lines +289 to +295
IF (to_drop = true) THEN
sqlstring := format('SECURITY LABEL FOR spock ON COLUMN %I.%I IS NULL;' ,
rel, att_name);
ELSE
sqlstring := format('SECURITY LABEL FOR spock ON COLUMN %I.%I IS %L;' ,
rel, att_name, 'spock.delta_apply');
END IF;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Quote the relation name correctly before executing DDL.

%I is unsafe for a regclass value here. If rel::text is schema-qualified, the generated statement becomes "schema.table" instead of "schema"."table", so SECURITY LABEL / ALTER TABLE will fail or target the wrong identifier whenever the relation is not referenced as a bare visible name.

Also applies to: 316-323

🤖 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/spock--5.0.8--6.0.0.sql` around lines 289 - 295, The relation name must
be properly quoted when building the SECURITY LABEL statement: stop using %I
with the regclass value `rel` (it produces a single quoted token like
"schema.table"); instead build a schema-qualified, safely quoted name from
`rel::text` by splitting into schema and table and quoting each identifier (e.g.
quote_ident(split_part(rel::text, '.', 1)) || '.' ||
quote_ident(split_part(rel::text, '.', 2))) and use that string in the formatted
SQL assigned to `sqlstring` for the SECURITY LABEL branch (the same change must
be applied to the similar block referenced at lines 316-323). Ensure `att_name`
continues to be used with %I or otherwise quoted safely.

Comment thread sql/spock--6.0.0.sql
Comment thread sql/spock--6.0.0.sql
Comment on lines +935 to +941
IF (to_drop = true) THEN
sqlstring := format('SECURITY LABEL FOR spock ON COLUMN %I.%I IS NULL;' ,
rel, att_name);
ELSE
sqlstring := format('SECURITY LABEL FOR spock ON COLUMN %I.%I IS %L;' ,
rel, att_name, 'spock.delta_apply');
END IF;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Quote the relation name correctly before executing DDL.

%I is unsafe for a regclass value here. If rel::text is schema-qualified, the generated statement becomes "schema.table" instead of "schema"."table", so SECURITY LABEL / ALTER TABLE will fail or target the wrong identifier whenever the relation is not referenced as a bare visible name.

Also applies to: 962-969

🤖 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/spock--6.0.0.sql` around lines 935 - 941, The relation identifier `rel`
is a regclass and must be properly schema- and table-quoted instead of using %I
on `rel` directly; change the SQL construction to split rel::text into schema
and table, quote each with quote_ident and build a qualified name (e.g.
quote_ident(split_part(rel::text, '.', 1)) || '.' ||
quote_ident(split_part(rel::text, '.', 2))) and use that qualified string in the
format call (use %s for the qualified name and %I for att_name); apply the same
fix for the other occurrence referenced (lines 962-969) so sqlstring is built
with correctly quoted schema.table and column identifiers.

Comment thread src/spock_apply_heap.c
Comment thread src/spock_autoddl.c Outdated
Comment on lines +347 to +349
/* Only process DDL statements and SECURITY LABEL's */
if (GetCommandLogLevel(parsetree) != LOGSTMT_DDL &&
!IsA(parsetree, SecLabelStmt))

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

AutoDDL should only whitelist Spock-owned security labels.

This now admits every SecLabelStmt, not just SECURITY LABEL FOR spock .... Unrelated providers can be auto-replicated to subscribers, where they may fail outright or mutate node-local security policy. Gate this on castNode(SecLabelStmt, parsetree)->provider == SPOCK_SECLABEL_PROVIDER.

🤖 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_autoddl.c` around lines 347 - 349, The current check lets every
SecLabelStmt pass through, so change the condition to only allow SECURITY LABEL
statements that are for Spock by additionally testing that
castNode(SecLabelStmt, parsetree)->provider equals SPOCK_SECLABEL_PROVIDER;
i.e., keep the existing GetCommandLogLevel(parsetree) != LOGSTMT_DDL check and
replace the plain IsA(parsetree, SecLabelStmt) allowance with a compound check
that IsA(parsetree, SecLabelStmt) && strcmp(castNode(SecLabelStmt,
parsetree)->provider, SPOCK_SECLABEL_PROVIDER) == 0 (or equivalent
provider-equality test), so only Spock-owned security labels are whitelisted.

Comment thread src/spock_relcache.c
Comment thread src/spock_relcache.c
Comment thread src/spock_repset.c
Comment thread src/spock.c

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 `@tests/docker/Dockerfile-step-1.el9`:
- Around line 107-108: Replace the mutable branch checkout in the Dockerfile
clone step (the git clone line that uses --branch delta-apply-update) with a
pinned immutable ref: accept a BUILD_ARG like SPOCKBENCH_REF and use it in the
git clone (e.g., git clone --branch ${SPOCKBENCH_REF} --depth 1 ... or git clone
... && git checkout ${SPOCKBENCH_REF}) so CI can pass a specific commit SHA or
tag; update the Dockerfile to declare ARG SPOCKBENCH_REF with a sensible default
(the known SHA) and ensure compose/workflow passes that build-arg during image
builds.
🪄 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: 26629c97-bbe6-40f0-ac73-d18e2b0a727f

📥 Commits

Reviewing files that changed from the base of the PR and between 44b4c98 and 3b89ccd.

📒 Files selected for processing (1)
  • tests/docker/Dockerfile-step-1.el9

Comment thread tests/docker/Dockerfile-step-1.el9
Build the generated SECURITY LABEL / ALTER TABLE statements with %s on the
regclass instead of %I. regclass output is already schema-qualified and
per-identifier quoted; %I double-quoted the whole "schema.table" as a single
identifier, breaking on non-visible or quoted relations.

Also drops the "hack" wording from the revert-path comment.
- build_delta_tuple: ERROR with ERRCODE_NULL_VALUE_NOT_ALLOWED on a NULL
  local base value instead of only Assert(), which vanishes in non-assert
  builds and would call the delta function with an invalid Datum (a crash
  for by-reference types such as numeric/money).

- alter_replication_set: validate table identity via
  get_replication_identity() so delta_apply (REPLICA IDENTITY FULL) tables
  are handled consistently with the add-table path.

- spock_object_relabel: reject security labels on non-plain-table objects
  (views, matviews, sequences, indexes, foreign/partitioned tables, etc.).

- spock_relation_open: free the security label string after use.

- autoddl: document that SECURITY LABELs of every provider are replicated,
  so participating nodes must have the same label providers loaded.
@mason-sharp mason-sharp force-pushed the re-add-delta-apply-via-sec-label branch from ada61d6 to 79d46a4 Compare June 6, 2026 22:49

@danolivo danolivo left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something is wrong here. Let's call:

pgbench -p $PGPORT1 -i -s 1
ALTER TABLE pgbench_accounts ALTER COLUMN abalance SET NOT NULL;
SELECT spock.delta_apply('pgbench_accounts', 'abalance');

Loading with pgbench two cross-wired Spock nodes:

pgbench -c 5 -j 5 -P 3 -f script.pgb -T 30 -p $PGPORT1 &
pgbench -c 5 -j 5 -P 3 -f script.pgb -T 30 -p $PGPORT2

After the end of the sync I see:

-- $PGPORT1:
SELECT sum(abalance) FROM pgbench_accounts;
   sum   
---------
 -877032
(1 row)

-- $PGPORT2:
SELECT sum(abalance) FROM pgbench_accounts;
   sum   
---------
 -419749

pgbench script:

\set aid random(1, 100000 * :scale)
\set bid random(1, 1 * :scale)
\set tid random(1, 10 * :scale)
\set delta random(-5000, 5000)
  
BEGIN;
UPDATE pgbench_accounts SET abalance = :delta WHERE aid = :aid;
SELECT abalance FROM pgbench_accounts WHERE aid = :aid;
UPDATE pgbench_tellers SET tbalance = tbalance + :delta WHERE tid = :tid;
UPDATE pgbench_branches SET bbalance = bbalance + :delta WHERE bid = :bid;
INSERT INTO pgbench_history (tid, bid, aid, delta, mtime) VALUES (:tid, :bid, :aid, :delta, CURRENT_TIMESTAMP);
END;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants