Re-add delta apply via security label commits#489
Open
mason-sharp wants to merge 8 commits into
Open
Conversation
This reverts commit 92c474a.
This reverts commit 2aed580.
This reverts commit 64b0bf3.
…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).
…class citizen." This reverts commit 6e33718.
This reverts commit de817e4.
This reverts commit 71f754e.
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.
📝 WalkthroughWalkthroughReplaces 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. ChangesDelta-Apply via Security Labels
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 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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 winInitialize
deltatup->changedfor the full local descriptor.
heap_modify_tuple()consumesdesc->nattschange flags, but this function only assignschanged[]inside therel->nattsloop. When the subscriber has extra local columns, the remaining entries stay as stack garbage and can make random local-only columns look updated.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.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));🤖 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 winDon't purge all Spock security labels on ordinary drops inside schema
spock.
dropping_spock_objbecomes true for any relation under schemaspock, and this branch then deletes everyprovider='spock'row frompg_seclabelbefore returning. The upgrade script insql/spock--5.0.8--6.0.0.sqldropsspock.lag_trackerandspock.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 winUse
$pg_binfor the backgroundedpsqlinstead of relying onPATH.
$pg_binis captured (Line 21) and used elsewhere via the helpers, but thisIPC::Runinvocation calls a barepsql. 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_queryinSpockTest.pmbuild their command from$pg_binso 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
⛔ 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 (17)
Makefileinclude/spock.hinclude/spock_relcache.hsql/spock--5.0.8--6.0.0.sqlsql/spock--6.0.0.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 (1)
- Makefile
Comment on lines
+239
to
+248
| /* | ||
| * Allow only DEFAULT type of replica identity. FULL type means we have | ||
| * already requested delta_apply feature on this table. | ||
| * Avoid INDEX type because indexes may have different names on the nodes and | ||
| * it would be better to stay paranoid than afraid of consequences. | ||
| */ | ||
| IF (relreplident <> 'd' AND relreplident <> 'f') | ||
| THEN | ||
| RAISE EXCEPTION 'spock can apply delta_apply feature to the DEFAULT replica identity type only. This table holds "%" idenity', relreplident; | ||
| END IF; |
There was a problem hiding this comment.
Don't assume REPLICA IDENTITY FULL was introduced by Spock.
This accepts tables that were already FULL, but the last-label removal path always switches them back to DEFAULT. A table that started life with REPLICA IDENTITY FULL loses that user-defined setting after spock.delta_apply(..., true). Reject pre-existing FULL tables unless they already carry a Spock label, or persist and restore the prior state.
Also applies to: 308-324
🤖 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 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.
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; |
There was a problem hiding this comment.
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 on lines
+885
to
+894
| /* | ||
| * Allow only DEFAULT type of replica identity. FULL type means we have | ||
| * already requested delta_apply feature on this table. | ||
| * Avoid INDEX type because indexes may have different names on the nodes and | ||
| * it would be better to stay paranoid than afraid of consequences. | ||
| */ | ||
| IF (relreplident <> 'd' AND relreplident <> 'f') | ||
| THEN | ||
| RAISE EXCEPTION 'spock can apply delta_apply feature to the DEFAULT replica identity type only. This table holds "%" idenity', relreplident; | ||
| END IF; |
There was a problem hiding this comment.
Don't assume REPLICA IDENTITY FULL was introduced by Spock.
This accepts tables that were already FULL, but the last-label removal path always switches them back to DEFAULT. A table that started life with REPLICA IDENTITY FULL loses that user-defined setting after spock.delta_apply(..., true). Reject pre-existing FULL tables unless they already carry a Spock label, or persist and restore the prior state.
Also applies to: 954-970
🤖 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 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.
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; |
There was a problem hiding this comment.
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 on lines
+563
to
+569
| loc_value = heap_getattr(TTS_TUP(localslot), remoteattnum + 1, tupdesc, | ||
| &loc_isnull); | ||
| Assert(!loc_isnull); | ||
|
|
||
| result = OidFunctionCall3Coll(rel->delta_apply_functions[remoteattnum], | ||
| InvalidOid, oldtup->values[remoteattnum], | ||
| newtup->values[remoteattnum], loc_value); |
There was a problem hiding this comment.
Handle NULL local base values outside Assert.
The new checks reject NULL remote OLD/NEW values, but localslot can still be NULL when nodes have drifted constraints or pre-existing bad rows. In non-assert builds Assert(!loc_isnull) disappears, and OidFunctionCall3Coll() is then invoked with an invalid Datum. This should take the same ERRCODE_NULL_VALUE_NOT_ALLOWED path as the remote NULL case.
🤖 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 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.
Comment on lines
+347
to
+349
| /* Only process DDL statements and SECURITY LABEL's */ | ||
| if (GetCommandLogLevel(parsetree) != LOGSTMT_DDL && | ||
| !IsA(parsetree, SecLabelStmt)) |
There was a problem hiding this comment.
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 on lines
+142
to
168
| ObjectAddressSubSet(object, RelationRelationId, | ||
| RelationGetRelid(entry->rel), | ||
| entry->attmap[i] + 1); | ||
| seclabel = GetSecurityLabel(&object, SPOCK_SECLABEL_PROVIDER); | ||
| if (seclabel != NULL) | ||
| { | ||
| char *fname; | ||
| Form_pg_attribute att; | ||
| Oid dfunc; | ||
| Form_pg_attribute att; | ||
| Oid dfunc; | ||
|
|
||
| att = TupleDescAttr(desc, entry->attmap[i]); | ||
| fname = pstrdup(GET_STRING_RELOPTION(aopt, | ||
| delta_apply_function)); | ||
| dfunc = spock_lookup_delta_function(fname, att->atttypid); | ||
| pfree(fname); | ||
| dfunc = spock_lookup_delta_function(seclabel, att->atttypid); | ||
|
|
||
| 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], | ||
| GET_STRING_RELOPTION(aopt, delta_apply_function)); | ||
| entry->attnames[i], seclabel); | ||
|
|
||
|
|
||
| entry->has_delta_columns = true; | ||
| 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; | ||
| } |
There was a problem hiding this comment.
Free seclabel in spock_relation_open() after using it
get_replication_identity() already frees seclabel, but spock_relation_open() doesn’t free it on the success path, leaking one allocation per processed labeled column.
🧹 Suggested fix
seclabel = GetSecurityLabel(&object, SPOCK_SECLABEL_PROVIDER);
if (seclabel != NULL)
{
Form_pg_attribute att;
Oid dfunc;
@@
entry->delta_apply_functions[entry->attmap[i]] = dfunc;
Assert(entry->delta_apply_functions[entry->attmap[i]] != InvalidOid);
entry->has_delta_columns = true;
+ pfree(seclabel);
}
else
{
/* Main case */
entry->delta_apply_functions[entry->attmap[i]] = InvalidOid;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ObjectAddressSubSet(object, RelationRelationId, | |
| RelationGetRelid(entry->rel), | |
| entry->attmap[i] + 1); | |
| seclabel = GetSecurityLabel(&object, SPOCK_SECLABEL_PROVIDER); | |
| if (seclabel != NULL) | |
| { | |
| char *fname; | |
| Form_pg_attribute att; | |
| Oid dfunc; | |
| Form_pg_attribute att; | |
| Oid dfunc; | |
| att = TupleDescAttr(desc, entry->attmap[i]); | |
| fname = pstrdup(GET_STRING_RELOPTION(aopt, | |
| delta_apply_function)); | |
| dfunc = spock_lookup_delta_function(fname, att->atttypid); | |
| pfree(fname); | |
| dfunc = spock_lookup_delta_function(seclabel, att->atttypid); | |
| 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], | |
| GET_STRING_RELOPTION(aopt, delta_apply_function)); | |
| entry->attnames[i], seclabel); | |
| entry->has_delta_columns = true; | |
| 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; | |
| } | |
| ObjectAddressSubSet(object, RelationRelationId, | |
| RelationGetRelid(entry->rel), | |
| entry->attmap[i] + 1); | |
| seclabel = GetSecurityLabel(&object, SPOCK_SECLABEL_PROVIDER); | |
| if (seclabel != NULL) | |
| { | |
| Form_pg_attribute att; | |
| Oid dfunc; | |
| att = TupleDescAttr(desc, entry->attmap[i]); | |
| dfunc = spock_lookup_delta_function(seclabel, att->atttypid); | |
| if (dfunc == InvalidOid) | |
| elog(ERROR, "SPOCK: column %s.%s.%s is configured for " | |
| "delta_apply function %s - function not found", | |
| entry->nspname, entry->relname, | |
| entry->attnames[i], seclabel); | |
| entry->delta_apply_functions[entry->attmap[i]] = dfunc; | |
| Assert(entry->delta_apply_functions[entry->attmap[i]] != InvalidOid); | |
| entry->has_delta_columns = true; | |
| pfree(seclabel); | |
| } | |
| else | |
| { | |
| /* Main case */ | |
| entry->delta_apply_functions[entry->attmap[i]] = InvalidOid; | |
| } |
🤖 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 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().
Comment on lines
+462
to
+495
| Oid | ||
| get_replication_identity(Relation rel) | ||
| { | ||
| TupleDesc tupDesc = RelationGetDescr(rel); | ||
| int i = 0; | ||
|
|
||
| if (OidIsValid(rel->rd_replidindex)) | ||
| return rel->rd_replidindex; | ||
|
|
||
| if (rel->rd_rel->relreplident != REPLICA_IDENTITY_FULL || | ||
| !OidIsValid(rel->rd_pkindex)) | ||
| return InvalidOid; | ||
|
|
||
| for (i = 0; i < tupDesc->natts; i++) | ||
| { | ||
| char *seclabel; | ||
| ObjectAddress object; | ||
|
|
||
| ObjectAddressSubSet(object, RelationRelationId, | ||
| RelationGetRelid(rel), i + 1); | ||
| seclabel = GetSecurityLabel(&object, SPOCK_SECLABEL_PROVIDER); | ||
|
|
||
| if (seclabel != NULL) | ||
| { | ||
| /* | ||
| * Relation has at least on security label on it. Treat it as a | ||
| * delta_apply feature. | ||
| */ | ||
| pfree(seclabel); | ||
| return rel->rd_pkindex; | ||
| } | ||
| } | ||
|
|
||
| return InvalidOid; |
There was a problem hiding this comment.
Keep get_replication_identity() consistent with spock_relation_open().
This helper scans every local attribute, while spock_relation_open() only treats remote-mapped columns as delta candidates. In the supported “local has more columns than remote” case, a label on a local-only column makes this return rd_pkindex, so src/spock_autoddl.c:261-291 and src/spock_repset.c:1113-1144 can accept UPDATE/DELETE replication even though the apply side still leaves has_delta_columns false. Derive both decisions from the same mapped-column predicate, or reject labels on columns that are absent from the replicated shape. Based on learnings, the local-wider-than-remote case is expected in src/spock_relcache.c, so this helper needs to stay aligned with the mapped-column logic in spock_relation_open().
🤖 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 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.
Comment on lines
+1134
to
1135
| if (!OidIsValid(get_replication_identity(targetrel)) && | ||
| (repset->replicate_update || repset->replicate_delete)) |
There was a problem hiding this comment.
Align alter_replication_set identity validation with the new get_replication_identity logic.
Line 1134 correctly moved add-table validation to get_replication_identity(...), but Line 864 still uses targetrel->rd_replidindex. That leaves inconsistent behavior between add and alter flows for delta-apply/security-label tables.
Suggested fix
diff --git a/src/spock_repset.c b/src/spock_repset.c
@@
- if (!OidIsValid(targetrel->rd_replidindex) &&
+ if (!OidIsValid(get_replication_identity(targetrel)) &&
(repset->replicate_update || repset->replicate_delete))
ereport(ERROR,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("replication set %s cannot be altered to "
"replicate UPDATEs or DELETEs because it "
"contains tables without PRIMARY KEY",
repset->name)));🤖 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_repset.c` around lines 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(...).
Comment on lines
+978
to
+1002
| /* | ||
| * Spock security label hook. | ||
| * | ||
| * Just to be sure user adds a proper label: only table columns may be applied. | ||
| */ | ||
| static void | ||
| spock_object_relabel(const ObjectAddress *object, const char *seclabel) | ||
| { | ||
| Oid extoid; | ||
|
|
||
| extoid = get_extension_oid(EXTENSION_NAME, true); | ||
| if (!OidIsValid(extoid)) | ||
| elog(ERROR, "spock extension is not created yet"); | ||
|
|
||
| /* | ||
| * Check: classId must be pg_class, objectId should an existing table and | ||
| * attnum must be more than 0. | ||
| */ | ||
| if (object->classId != RelationRelationId || object->objectSubId <= 0 || | ||
| !SearchSysCacheExists1(RELOID, ObjectIdGetDatum(object->objectId))) | ||
| ereport(ERROR, | ||
| (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), | ||
| errmsg("spock provider does not support labels on %s", | ||
| getObjectTypeDescription(object, false)))); | ||
| } |
There was a problem hiding this comment.
Reject non-table pg_class objects in the label hook.
In spock_object_relabel (src/spock.c:978-1002), the guard only checks object->classId == RelationRelationId, object->objectSubId > 0, and that the objectId exists in RELOID; it does not validate the relation’s relkind/type. Add an explicit relkind check and reject anything other than plain tables before accepting the label.
🤖 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.c` around lines 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.
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 `@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
📒 Files selected for processing (1)
tests/docker/Dockerfile-step-1.el9
Comment on lines
+107
to
108
| git clone --branch delta-apply-update --depth 1 \ | ||
| https://github.com/pgEdge/spockbench.git /home/pgedge/spockbench && \ |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify current remote head for the mutable branch and capture SHA to pin.
git ls-remote --heads https://github.com/pgEdge/spockbench.git delta-apply-update
# Verify Dockerfile currently uses a branch (mutable ref).
rg -n 'git clone --branch|SPOCKBENCH_COMMIT|spockbench\.git' tests/docker/Dockerfile-step-1.el9Repository: pgEdge/spock
Length of output: 309
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="tests/docker/Dockerfile-step-1.el9"
echo "---- Dockerfile-step-1.el9: lines 90-130 ----"
nl -ba "$file" | sed -n '90,130p'
echo "---- Dockerfile-step-1.el9: lines 130-190 ----"
nl -ba "$file" | sed -n '130,190p'
echo "---- Search for SPOCKBENCH_COMMIT / LATEST_TAG in repository ----"
rg -n "SPOCKBENCH_COMMIT|LATEST_TAG|spockbench\.git" -S .Repository: pgEdge/spock
Length of output: 163
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="tests/docker/Dockerfile-step-1.el9"
echo "---- Dockerfile-step-1.el9: lines 90-130 ----"
nl -ba "$file" | sed -n '90,130p'
echo "---- Dockerfile-step-1.el9: lines 130-190 ----"
nl -ba "$file" | sed -n '130,190p'
echo "---- Search for SPOCKBENCH_COMMIT / LATEST_TAG in repository ----"
rg -n "SPOCKBENCH_COMMIT|LATEST_TAG|spockbench\.git" -S .Repository: pgEdge/spock
Length of output: 163
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="tests/docker/Dockerfile-step-1.el9"
echo "---- $file: lines 80-170 ----"
awk 'NR>=80 && NR<=170 {printf "%d:%s\n", NR, $0}' "$file"
echo "---- Search for SPOCKBENCH_COMMIT / LATEST_TAG / spockbench.git in repo ----"
rg -n "SPOCKBENCH_COMMIT|LATEST_TAG|spockbench\.git" -S .Repository: pgEdge/spock
Length of output: 4319
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "SPOCKBENCH_SOURCE_DIR|/home/pgedge/spockbench|spockbench\.yml|run-tests\.sh|spockbench" -S .Repository: pgEdge/spock
Length of output: 1332
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "spockbench\.yml|Dockerfile-step-1\.el9|SPOCKBENCH_SOURCE_DIR" -S .github tests/docker README.md || true
ls -R .github/workflows 2>/dev/null || trueRepository: pgEdge/spock
Length of output: 1602
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- .github/workflows/spockbench.yml: lines 1-140 ----"
sed -n '1,140p' .github/workflows/spockbench.yml
echo "---- .github/workflows/spockbench.yml: lines 140-320 ----"
sed -n '140,320p' .github/workflows/spockbench.yml
echo "---- tests/docker/docker-compose.yml: lines 1-140 ----"
sed -n '1,140p' tests/docker/docker-compose.yml
echo "---- tests/docker/docker-compose.yml: lines 140-260 ----"
sed -n '140,260p' tests/docker/docker-compose.yml
echo "---- Search spockbench-related build args/mentions ----"
rg -n "SPOCKBENCH|spockbench|delta-apply-update|commit|SHA|build:\s*\{|\bargs:\b" .github/workflows/spockbench.yml tests/docker/docker-compose.ymlRepository: pgEdge/spock
Length of output: 10847
🏁 Script executed:
rg -n "(\bspockbench/|\bspockbench\b.*\.(sh|py|yml)|/home/pgedge/spockbench|SPOCKBENCH_SOURCE_DIR)" -S .github tests README.mdRepository: pgEdge/spock
Length of output: 634
Pin spockbench to an immutable commit to avoid flaky/breaking CI builds.
In tests/docker/Dockerfile-step-1.el9, Spockbench is cloned from the mutable branch delta-apply-update (git clone --branch delta-apply-update --depth 1 ...). That branch currently points to 6066856b2b3f3a90875def10ec713d2d27659f0a and can move/vanish, changing the installed code and making the Docker image non-deterministic. Pin to a specific commit SHA (or immutable tag) and wire it through build args (current compose/workflow don’t pass any pinned Spockbench ref).
Suggested change
+ARG SPOCKBENCH_COMMIT=<replace-with-known-good-sha>
RUN set -eux && \
- git clone --branch delta-apply-update --depth 1 \
- https://github.com/pgEdge/spockbench.git /home/pgedge/spockbench && \
+ git clone --depth 1 https://github.com/pgEdge/spockbench.git /home/pgedge/spockbench && \
+ cd /home/pgedge/spockbench && \
+ git fetch --depth 1 origin ${SPOCKBENCH_COMMIT} && \
+ git checkout --detach ${SPOCKBENCH_COMMIT} && \
echo "export SPOCKBENCH_SOURCE_DIR=/home/pgedge/spockbench" >> /home/pgedge/.bashrc📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| git clone --branch delta-apply-update --depth 1 \ | |
| https://github.com/pgEdge/spockbench.git /home/pgedge/spockbench && \ | |
| ARG SPOCKBENCH_COMMIT=6066856b2b3f3a90875def10ec713d2d27659f0a | |
| RUN set -eux && \ | |
| git clone --depth 1 https://github.com/pgEdge/spockbench.git /home/pgedge/spockbench && \ | |
| cd /home/pgedge/spockbench && \ | |
| git fetch --depth 1 origin ${SPOCKBENCH_COMMIT} && \ | |
| git checkout --detach ${SPOCKBENCH_COMMIT} && \ | |
| echo "export SPOCKBENCH_SOURCE_DIR=/home/pgedge/spockbench" >> /home/pgedge/.bashrc |
🤖 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/docker/Dockerfile-step-1.el9` around lines 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.
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
1 participant
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.
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.