Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a pluggable PostgreSQL nextval() hook and a Spock SeqAM dispatcher with per-sequence "kinds" stored in ChangesDistributed Snowflake Sequences
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| ErrorProne | 1 high (1 false positive) |
| Security | 1 critical (1 false positive) |
| Complexity | 10 medium |
🟢 Metrics 137 complexity · 4 duplication
Metric Results Complexity 137 Duplication 4
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.
Actionable comments posted: 7
🧹 Nitpick comments (6)
tests/regress/sql/seqam_snowflake.sql (1)
185-188: 💤 Low valueDirect
DELETE FROM spock.sequence_kindbypasses the dispatcher invalidation contract.The documentation/dispatcher design relies on relcache invalidation triggered through
spock.alter_sequence_set_kind()to flush per-backend dispatcher caches; a rawDELETEagainst the catalog table skips that path. For an end-of-file cleanup in a single-backend regression this is unlikely to cause observable issues, but it sets a poor example. Prefer iterating withalter_sequence_set_kind(..., 'local')(or simply rely on theDROP SEQUENCEcascade if the executor hook removes the row) so the cleanup matches the supported surface.🤖 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/regress/sql/seqam_snowflake.sql` around lines 185 - 188, Replace the raw catalog deletion with the supported dispatcher-invalidation calls: instead of "DELETE FROM spock.sequence_kind" iterate over the sequence entries (e.g., local_seq, other_seq, sf_seq) and call spock.alter_sequence_set_kind(sequence_regclass, 'local') for each to trigger relcache invalidation; alternatively rely on the existing DROP SEQUENCE statements to cascade the cleanup if the executor hook removes the spock.sequence_kind rows. Ensure you use spock.alter_sequence_set_kind() rather than direct manipulation of spock.sequence_kind to preserve the invalidation contract.docs/distributed_sequences.md (1)
142-148: 💤 Low valueAdd a language to the fenced code block (markdownlint MD040).
✏️ Proposed fix
-``` +```text bit 63 : reserved, always 0 (sign bit, keeps values non-negative)🤖 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 `@docs/distributed_sequences.md` around lines 142 - 148, The fenced code block showing the bit layout needs a language tag to satisfy markdownlint MD040; update the opening fence for that block (the triple backticks before "bit 63 ...") to include a language identifier such as "text" or "txt" so it becomes "```text", leaving the block contents unchanged.src/spock_executor.c (2)
239-259: 💤 Low valueConsider reusing outer-scope variables to avoid shadowing.
Lines 250-251 declare
Oid save_userid; int save_sec_context;in the inner scope, but these shadow the already-declared variables at lines 205-206. While not incorrect, reusing the outer declarations would improve readability and reduce cognitive load for maintainers.♻️ Optional refactor to reuse outer-scope variables
if (get_rel_relkind(objectId) == RELKIND_SEQUENCE && !dropping_spock_obj) { - Oid save_userid; - int save_sec_context; - GetUserIdAndSecContext(&save_userid, &save_sec_context); SetUserIdAndSecContext(BOOTSTRAP_SUPERUSERID, save_sec_context |🤖 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 239 - 259, The inner block in the sequence-drop handling shadows outer variables save_userid and save_sec_context; remove the inner declarations so the block reuses the outer-scope save_userid and save_sec_context variables (keep the calls to GetUserIdAndSecContext, SetUserIdAndSecContext and the spock_seqam_drop_sequence_record(objectId) call intact) to avoid variable shadowing and improve readability around the RELKIND_SEQUENCE check and sequence cleanup logic.
295-316: 💤 Low valueConsider reusing outer-scope variables to avoid shadowing.
Lines 307-308 declare
Oid saved_userid; int saved_sec_context;in the OAT_POST_ALTER branch, but these shadow the variables declared at lines 205-206 (though with slightly different names:saved_*vssave_*). Reusing the outer declarations would improve consistency and readability.♻️ Optional refactor to reuse outer-scope variables
else if (access == OAT_POST_ALTER && subId == 0 && classId == RelationRelationId && get_rel_relkind(objectId) == RELKIND_SEQUENCE) { - Oid saved_userid; - int saved_sec_context; - - GetUserIdAndSecContext(&saved_userid, &saved_sec_context); + GetUserIdAndSecContext(&save_userid, &save_sec_context); SetUserIdAndSecContext(BOOTSTRAP_SUPERUSERID, - saved_sec_context | + save_sec_context | SECURITY_LOCAL_USERID_CHANGE); spock_seqam_relocate_sequence_record(objectId); - SetUserIdAndSecContext(saved_userid, saved_sec_context); + SetUserIdAndSecContext(save_userid, save_sec_context); }🤖 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 295 - 316, Remove the inner declarations that shadow the outer saved user/context variables and reuse the existing outer-scope variables instead: in the OAT_POST_ALTER branch drop the lines declaring "Oid saved_userid; int saved_sec_context;" and call GetUserIdAndSecContext, SetUserIdAndSecContext and the spock_seqam_relocate_sequence_record(objectId) using the previously declared outer variables (e.g., save_userid/save_sec_context or the existing saved_* identifiers) so you don't shadow the outer scope and keep naming consistent.sql/spock--6.0.0.sql (1)
405-411: ⚡ Quick winConsider making the view more robust to stale OIDs.
The
sequence_infoview castssk.seqoid::regclassat line 407. After a logical restore, stale OIDs may no longer reference valid sequences, causing the cast to ERROR and making the entire view unusable.Suggestion:
Add a LEFT JOIN topg_classand filter or mark invalid entries, so the view remains queryable even when OIDs are stale:♻️ Proposed robustness improvement
CREATE VIEW spock.sequence_info AS SELECT - sk.seqoid::regclass AS sequence_name, + CASE WHEN c.oid IS NOT NULL + THEN sk.seqoid::regclass + ELSE NULL + END AS sequence_name, sk.kind, CASE WHEN spock.sequence_hook_available() THEN 'active' ELSE 'inactive' END AS hook_status -FROM spock.sequence_kind sk; +FROM spock.sequence_kind sk +LEFT JOIN pg_class c ON c.oid = sk.seqoid AND c.relkind = 'S';Alternatively, document that operators must run
spock.convert_all_sequences()immediately afterpg_restoreto refresh OIDs before querying the view.🤖 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 405 - 411, The view spock.sequence_info currently uses sk.seqoid::regclass (in the CREATE VIEW spock.sequence_info SELECT block), which will ERROR on stale OIDs after logical restore; modify the view to LEFT JOIN pg_class (join pg_class c ON c.oid = sk.seqoid) and use c.relname (or construct schema-qualified name from pg_namespace + relname) to present sequence_name while marking or filtering rows where c.oid IS NULL as invalid/stale (e.g., set sequence_name to NULL or 'INVALID_OID'); keep the existing spock.sequence_hook_available() usage for hook_status and document or leave a note to run spock.convert_all_sequences() after pg_restore if users prefer to refresh OIDs.src/spock_seqam.c (1)
841-846: ⚡ Quick winStale
seqmaxbound in the header comment forvalidate_sequence_target.The comment states the snowflake path refuses
seqmax < 1 << 22, but the actual check at line 900 ismaxv < PG_INT64_MAX(requiresNO MAXVALUE). The implementation is far stricter than the doc claims, which is misleading for anyone reading the header to understand the validation contract.📝 Suggested comment correction
- * - seqmax < 1 << 22 (any non-zero snowflake timestamp shifts left - * 22 bits; smaller MAXVALUE excludes the snowflake range), + * - seqmax < PG_INT64_MAX (snowflake emissions use the full + * non-negative int64 range up to year ~2092, so any user-imposed + * MAXVALUE would eventually be violated; require NO MAXVALUE),🤖 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_seqam.c` around lines 841 - 846, Header comment for validate_sequence_target is misleading: it says SPOCK_SEQAM_SNOWFLAKE refuses seqmax < 1 << 22 but the code actually requires NO MAXVALUE (maxv < PG_INT64_MAX). Update the header to match the implementation by replacing the "seqmax < 1 << 22" clause with a statement that snowflake mode requires no MAXVALUE (i.e., maxv == PG_INT64_MAX or equivalent), and mention the actual check symbol names SPOCK_SEQAM_SNOWFLAKE, validate_sequence_target, maxv, PG_INT64_MAX so readers can correlate comment to the code paths.
🤖 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 `@docs/distributed_sequences.md`:
- Around line 156-159: The "Monotonicity per node" bullet is inaccurate about
state storage; update the wording to say that per-sequence state is persisted in
the sequence relation's heap tuple (the last_value column) and protected by WAL
pre-log reservation rather than stored in extension-private shared memory, and
clarify that backend restarts read last_value from the heap tuple to continue
monotonic issuance (reference symbols: last_value, sequence relation heap tuple,
WAL pre-log reservation, and the "Monotonicity per node" bullet).
- Around line 270-278: The numbered list in docs/distributed_sequences.md has a
duplicate "3." causing the sequence to read 1, 2, 3, 3; update the final list
item to "4." so the steps read 1, 2, 3, 4 — specifically edit the line
describing the new node generating Snowflake values on first `nextval()` call
(following `spock.preview_snowflake_node_id(...)` and `spock.node_create(...)`)
and change its leading ordinal from "3." to "4.".
In `@sql/spock--6.0.0.sql`:
- Around line 332-334: The comment/doc mismatch: spock.sequence_kind is
described as node-local and claims convergence via re-executing
spock.alter_sequence_set_kind() on each node via AutoDDL, but the current
spock_autoddl_process only replicates LOGSTMT_DDL utility statements and does
not handle spock.alter_sequence_set_kind(), while src/spock_seqam.c and
docs/distributed_sequences.md imply AutoDDL-driven propagation; to fix, choose
one of two approaches and implement it consistently: (A) implement AutoDDL
replay for spock.alter_sequence_set_kind() by ensuring the function invocation
is logged as LOGSTMT_DDL and adding replay/handling in spock_autoddl_process
(and update src/spock_seqam.c and docs/distributed_sequences.md to state
cluster-wide propagation), or (B) update the SQL comment, src/spock_seqam.c, and
docs/distributed_sequences.md to explicitly document that spock.sequence_kind is
node-local, that operators must run spock.alter_sequence_set_kind() or
spock.convert_all_sequences() on each node manually (and describe the behavior
if only one node is changed), choosing the option that matches intended design.
In `@src/spock_seqam.c`:
- Around line 1090-1101: The reset currently always calls
seqam_reset_sequence_data() when kind != SPOCK_SEQAM_LOCAL, which wipes existing
snowflake state even if the row already had the same kind; change the guard to
only reset when we are actually transitioning kinds: in alter_sequence_set_kind,
use the previously-read found and row_kind values (from the earlier branch that
fetched the existing row) and call seqam_reset_sequence_data(seqrel) only when
kind != SPOCK_SEQAM_LOCAL AND (NOT found OR row_kind != kind), so existing
sequences with the same kind are not reset while new inserts or actual kind
changes still trigger the reset.
In `@tests/regress/sql/seqam_snowflake.sql`:
- Around line 18-26: The test fails because snowflake sequences need a
configured Spock node; either call spock.node_create('your_node_name') near the
top so spock.preview_snowflake_node_id('your_node_name') yields the expected 42,
or change the assertion that sets decoded_node_correct (currently comparing
node_id = 42) to compute the expected node id via
spock.preview_snowflake_node_id(current_node_name) instead of hard-coding 42;
specifically update the header to create a node with spock.node_create(...)
and/or replace the literal 42 in decoded_node_correct with
spock.preview_snowflake_node_id(...) so nextval() calls succeed and the test
produces deterministic output.
In `@tests/tap/t/050_seqam_snowflake.pl`:
- Around line 8-35: Update the test header in 050_seqam_snowflake.pl to reflect
the current Snowflake design: state is stored in the sequence heap tuple (not a
separate shmem slot allocator), the per-node ID is derived from the node name
(not spock.snowflake_node_id), and the test exercises sequence heap-backed
Snowflake state and spock.sequence_kind replication rather than any shmem
allocator; keep references to the higher-level mechanisms under test
(nextval_hook, dispatcher, snowflake CAS loop) but remove or replace the
incorrect lines about a shmem slot allocator and spock.snowflake_node_id setup
so the header accurately documents the implementation.
- Around line 46-63: The test currently writes the removed GUC
spock.snowflake_node_id via set_snowflake_node_id and restarts, which causes
startup failures; instead change the setup to derive per-node 10-bit snowflake
ids at runtime using spock.preview_snowflake_node_id(node_name) (callable from
the test via scalar_query), create nodes with spock.node_create(node_name, ...)
using those node names, replace the hard-coded 101/202 assertions with the
derived ids captured into variables (via scalar_query) and use those variables
in assertions, and replace any use of current_setting('spock.snowflake_node_id')
in INSERT payloads with either a literal node tag or
current_setting('spock.node_name', true) so no removed GUC is referenced.
---
Nitpick comments:
In `@docs/distributed_sequences.md`:
- Around line 142-148: The fenced code block showing the bit layout needs a
language tag to satisfy markdownlint MD040; update the opening fence for that
block (the triple backticks before "bit 63 ...") to include a language
identifier such as "text" or "txt" so it becomes "```text", leaving the block
contents unchanged.
In `@sql/spock--6.0.0.sql`:
- Around line 405-411: The view spock.sequence_info currently uses
sk.seqoid::regclass (in the CREATE VIEW spock.sequence_info SELECT block), which
will ERROR on stale OIDs after logical restore; modify the view to LEFT JOIN
pg_class (join pg_class c ON c.oid = sk.seqoid) and use c.relname (or construct
schema-qualified name from pg_namespace + relname) to present sequence_name
while marking or filtering rows where c.oid IS NULL as invalid/stale (e.g., set
sequence_name to NULL or 'INVALID_OID'); keep the existing
spock.sequence_hook_available() usage for hook_status and document or leave a
note to run spock.convert_all_sequences() after pg_restore if users prefer to
refresh OIDs.
In `@src/spock_executor.c`:
- Around line 239-259: The inner block in the sequence-drop handling shadows
outer variables save_userid and save_sec_context; remove the inner declarations
so the block reuses the outer-scope save_userid and save_sec_context variables
(keep the calls to GetUserIdAndSecContext, SetUserIdAndSecContext and the
spock_seqam_drop_sequence_record(objectId) call intact) to avoid variable
shadowing and improve readability around the RELKIND_SEQUENCE check and sequence
cleanup logic.
- Around line 295-316: Remove the inner declarations that shadow the outer saved
user/context variables and reuse the existing outer-scope variables instead: in
the OAT_POST_ALTER branch drop the lines declaring "Oid saved_userid; int
saved_sec_context;" and call GetUserIdAndSecContext, SetUserIdAndSecContext and
the spock_seqam_relocate_sequence_record(objectId) using the previously declared
outer variables (e.g., save_userid/save_sec_context or the existing saved_*
identifiers) so you don't shadow the outer scope and keep naming consistent.
In `@src/spock_seqam.c`:
- Around line 841-846: Header comment for validate_sequence_target is
misleading: it says SPOCK_SEQAM_SNOWFLAKE refuses seqmax < 1 << 22 but the code
actually requires NO MAXVALUE (maxv < PG_INT64_MAX). Update the header to match
the implementation by replacing the "seqmax < 1 << 22" clause with a statement
that snowflake mode requires no MAXVALUE (i.e., maxv == PG_INT64_MAX or
equivalent), and mention the actual check symbol names SPOCK_SEQAM_SNOWFLAKE,
validate_sequence_target, maxv, PG_INT64_MAX so readers can correlate comment to
the code paths.
In `@tests/regress/sql/seqam_snowflake.sql`:
- Around line 185-188: Replace the raw catalog deletion with the supported
dispatcher-invalidation calls: instead of "DELETE FROM spock.sequence_kind"
iterate over the sequence entries (e.g., local_seq, other_seq, sf_seq) and call
spock.alter_sequence_set_kind(sequence_regclass, 'local') for each to trigger
relcache invalidation; alternatively rely on the existing DROP SEQUENCE
statements to cascade the cleanup if the executor hook removes the
spock.sequence_kind rows. Ensure you use spock.alter_sequence_set_kind() rather
than direct manipulation of spock.sequence_kind to preserve the invalidation
contract.
🪄 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: dddbeff7-0e27-4b17-9054-4fe02332381c
⛔ Files ignored due to path filters (1)
tests/regress/expected/init.outis excluded by!**/*.out
📒 Files selected for processing (21)
docs/distributed_sequences.mdinclude/spock_node.hinclude/spock_seqam.hmkdocs.ymlpatches/15/pg15-050-nextval-hook.diffpatches/16/pg16-050-nextval-hook.diffpatches/17/pg17-050-nextval-hook.diffpatches/18/pg18-050-nextval-hook.diffsql/spock--6.0.0.sqlsrc/compat/18/spock_compat.hsrc/spock.csrc/spock_apply.csrc/spock_executor.csrc/spock_node.csrc/spock_seqam.csrc/spock_seqam_snowflake.csrc/spock_sequences.ctests/regress/sql/init.sqltests/regress/sql/seqam_snowflake.sqltests/tap/scheduletests/tap/t/050_seqam_snowflake.pl
💤 Files with no reviewable changes (1)
- src/compat/18/spock_compat.h
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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 `@docs/distributed_sequences.md`:
- Around line 17-19: The document currently conflicts about whether
spock.sequence_kind is replicated via user_catalog_table or is node-local via
alter_sequence_set_kind(); choose one consistent semantics (either: 1)
sequence_kind is replicated cluster-wide via user_catalog_table, or 2)
sequence_kind is node-local and alter_sequence_set_kind() does not replicate)
and update all mentions to that behavior (including the paragraphs referencing
spock.sequence_kind, the description of user_catalog_table, the docs around
alter_sequence_set_kind(), and the operator guidance/warnings) so every section
(the earlier statement, the alter_sequence_set_kind() discussion, and the later
notes) present the same behavior and implications for cluster-wide uniqueness
and upgrade procedures. Ensure the chosen behavior also updates any examples,
caveats, and recommended operator steps to prevent accidental per-node
conversions.
In `@sql/spock--6.0.0.sql`:
- Around line 324-330: The runtime is using cached seqoid values that may be
stale after logical restore, causing wrong-row resolution/deletes; update the
runtime surface (the OAT_POST_ALTER hook and any view that references seqoid,
and functions like spock.alter_sequence_set_kind, spock.convert_all_sequences,
spock.sequence_info, and the spock.sequence_kind table usage) to never trust
stored seqoid alone: either resolve sequences by (nspname, relname) first and
refresh seqoid before performing any operations, or validate seqoid by
attempting safe resolution (e.g. check pg_class entry for the tuple and replace
stale seqoid with the current regclass OID) and only proceed when refreshed;
ensure any view/join uses (nspname, relname) as the authoritative key and update
stored seqoid values lazily when accessed.
In `@src/spock_executor.c`:
- Around line 253-258: Wrap the temporary elevation to BOOTSTRAP_SUPERUSERID
around the SeqAM calls with a PG_TRY / PG_FINALLY pair so the original
credentials are restored even if an ERROR is thrown: call
GetUserIdAndSecContext(&save_userid, &save_sec_context); then
SetUserIdAndSecContext(BOOTSTRAP_SUPERUSERID, save_sec_context |
SECURITY_LOCAL_USERID_CHANGE); invoke spock_seqam_drop_sequence_record(objectId)
(and apply the same pattern for the OAT_POST_ALTER relocation block), and in
PG_FINALLY call SetUserIdAndSecContext(save_userid, save_sec_context); end with
PG_END_TRY(); ensure you keep the same local variables (save_userid,
save_sec_context) and the BOOTSTRAP_SUPERUSERID/SECURITY_LOCAL_USERID_CHANGE
flags so the restoration always runs on error or normal return.
In `@src/spock_node.c`:
- Around line 168-181: The helper spock_node_id_for_name currently returns any
16-bit Oid (possibly 0/InvalidOid) and doesn't ensure uniqueness in the
Snowflake domain; update logic so spock_node_id_for_name never returns
InvalidOid (reject or rehash until non-zero) and, where nodes are inserted (the
code path that uses node->id), validate that (node->id &
SPOCK_SNOWFLAKE_MAX_NODE_ID) is unique before writing the catalog row; if the
masked value collides, return an error to caller and refuse insert. Ensure you
reference SPOCK_SNOWFLAKE_NODE_BITS / SPOCK_SNOWFLAKE_MAX_NODE_ID for masking
and treat InvalidOid specially.
In `@src/spock_seqam.c`:
- Around line 547-556: When repopulating SeqAmCache you must revalidate
non-local managed kinds so we don't cache an incompatible sequence; after
getting kind from seqam_catalog_lookup (and after seqam_init_cache if needed)
add a call to validate_sequence_target(seqoid) for non-default kinds and, if
that validation fails, treat the sequence as unmanaged (e.g. set kind =
spock_seqam_default_kind) before creating the SeqAmCacheEntry; this prevents an
invalid managed kind from being cached and later passed to
spock_seqam_snowflake_nextval().
- Around line 564-568: Change the unsupported-state Assert checks into runtime
guards: replace the Assert(...) calls for RecoveryInProgress(),
creating_extension, IsBinaryUpgrade and the kind range check (kind >
SPOCK_SEQAM_LOCAL && kind < SPOCK_SEQAM_NMETHODS) with an if that detects any of
these conditions at runtime and raises a PostgreSQL error (ereport/elog)
returning ERRCODE_FEATURE_NOT_SUPPORTED and a clear errmsg explaining that the
requested spock.default_sequence_kind (e.g. "snowflake") is unsupported during
recovery, extension creation or binary-upgrade; reference the symbols
RecoveryInProgress, creating_extension, IsBinaryUpgrade, kind,
SPOCK_SEQAM_LOCAL, SPOCK_SEQAM_NMETHODS and mention spock.default_sequence_kind
in the message so callers get a descriptive runtime error instead of an
assertion in production.
🪄 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: 726d1de6-af5f-474e-8354-d13abe1c24a8
⛔ Files ignored due to path filters (1)
tests/regress/expected/init.outis excluded by!**/*.out
📒 Files selected for processing (22)
docs/distributed_sequences.mdinclude/spock_node.hinclude/spock_seqam.hmkdocs.ymlpatches/15/pg15-050-nextval-hook.diffpatches/16/pg16-050-nextval-hook.diffpatches/17/pg17-050-nextval-hook.diffpatches/18/pg18-050-nextval-hook.diffsql/spock--6.0.0.sqlsrc/compat/15/spock_compat.hsrc/compat/16/spock_compat.hsrc/compat/17/spock_compat.hsrc/compat/18/spock_compat.hsrc/spock.csrc/spock_apply.csrc/spock_executor.csrc/spock_node.csrc/spock_seqam.csrc/spock_seqam_snowflake.csrc/spock_sequences.ctests/regress/sql/init.sqltests/tap/schedule
💤 Files with no reviewable changes (4)
- src/compat/18/spock_compat.h
- src/compat/17/spock_compat.h
- src/compat/15/spock_compat.h
- src/compat/16/spock_compat.h
✅ Files skipped from review due to trivial changes (1)
- mkdocs.yml
Comment on lines
+17
to
+19
| `spock.sequence_kind` and replicated to all nodes via Spock's | ||
| `user_catalog_table` machinery, so every node converges on the same view | ||
| of which sequences are managed. |
There was a problem hiding this comment.
Conflicting replication semantics for spock.sequence_kind need reconciliation.
Line 17 says assignments replicate via user_catalog_table, but Line 87 through Line 94 says alter_sequence_set_kind() is node-local and does not replicate. The same conflict reappears at Line 239 through Line 243 and Line 260. Please align these sections to one behavior, otherwise operators may convert only one node and lose cluster-wide uniqueness guarantees.
🤖 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 `@docs/distributed_sequences.md` around lines 17 - 19, The document currently
conflicts about whether spock.sequence_kind is replicated via user_catalog_table
or is node-local via alter_sequence_set_kind(); choose one consistent semantics
(either: 1) sequence_kind is replicated cluster-wide via user_catalog_table, or
2) sequence_kind is node-local and alter_sequence_set_kind() does not replicate)
and update all mentions to that behavior (including the paragraphs referencing
spock.sequence_kind, the description of user_catalog_table, the docs around
alter_sequence_set_kind(), and the operator guidance/warnings) so every section
(the earlier statement, the alter_sequence_set_kind() discussion, and the later
notes) present the same behavior and implications for cluster-wide uniqueness
and upgrade procedures. Ensure the chosen behavior also updates any examples,
caveats, and recommended operator steps to prevent accidental per-node
conversions.
Comment on lines
+253
to
+258
| GetUserIdAndSecContext(&save_userid, &save_sec_context); | ||
| SetUserIdAndSecContext(BOOTSTRAP_SUPERUSERID, | ||
| save_sec_context | | ||
| SECURITY_LOCAL_USERID_CHANGE); | ||
| spock_seqam_drop_sequence_record(objectId); | ||
| SetUserIdAndSecContext(save_userid, save_sec_context); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/spock_executor.c"
# Show the referenced regions with line numbers
echo "=== Region around 240-330 ==="
nl -ba "$FILE" | sed -n '220,340p'
# Also locate the helper calls to confirm exact surrounding context
echo "=== Find SeqAM helper calls ==="
rg -n "spock_seqam_(drop_sequence_record|relocate_sequence_record)_record|SetUserIdAndSecContext|BOOTSTRAP_SUPERUSERID|PG_TRY|PG_FINALLY|PG_END_TRY" "$FILE"Repository: pgEdge/spock
Length of output: 147
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/spock_executor.c"
echo "=== Region around 240-330 ==="
# Use awk to add line numbers (nl may be unavailable)
awk 'NR>=220 && NR<=340 {printf "%6d\t%s\n", NR, $0}' "$FILE"
echo "=== Find SeqAM helper calls + privilege switches + PG_TRY/FINALLY usage ==="
rg -n "spock_seqam_(drop_sequence_record|relocate_sequence_record)_record|SetUserIdAndSecContext|BOOTSTRAP_SUPERUSERID|PG_TRY|PG_FINALLY|PG_END_TRY" "$FILE"Repository: pgEdge/spock
Length of output: 5226
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "PG_TRY|PG_FINALLY|PG_END_TRY" src/spock_executor.c || true
# Find definitions/implementations of the SeqAM helper functions
rg -n "spock_seqam_(drop_sequence_record|relocate_sequence_record)_record" -S src || true
# Show candidate implementation locations (function headers)
rg -n "void\s+spock_seqam_(drop_sequence_record|relocate_sequence_record)_record|spock_seqam_(drop_sequence_record|relocate_sequence_record)_record\s*\(" -S src || trueRepository: pgEdge/spock
Length of output: 38
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Locate SeqAM helper declarations/definitions across repo ==="
rg -n "spock_seqam_(drop_sequence_record|relocate_sequence_record)_record" -S . || true
echo "=== Where PG_TRY/PG_FINALLY is used around privilege switching in this repo ==="
rg -n "GetUserIdAndSecContext\\(|SetUserIdAndSecContext\\(|BOOTSTRAP_SUPERUSERID" -S src | head -n 200
echo "=== Check if SetUserIdAndSecContext/ GetUserIdAndSecContext wrappers already handle ERROR cleanup ==="
rg -n "GetUserIdAndSecContext\\b|SetUserIdAndSecContext\\b" -S . || true
echo "=== Search for PG_FINALLY/PG_TRY usage patterns in executor hooks ==="
rg -n "PG_TRY\\(|PG_FINALLY\\(|PG_END_TRY\\(" -S src | head -n 200Repository: pgEdge/spock
Length of output: 4911
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and show implementations in src/spock_seqam.c
FILE="src/spock_seqam.c"
echo "=== Drop sequence record implementation (if present) ==="
rg -n "spock_seqam_drop_sequence_record\\s*\\(" "$FILE" || true
# Print a window around the match (if any)
line=$(rg -n "spock_seqam_drop_sequence_record\\s*\\(" "$FILE" | head -n1 | cut -d: -f1 || true)
if [[ -n "${line:-}" ]]; then
start=$((line-30)); end=$((line+120));
awk -v s=$start -v e=$end 'NR>=s && NR<=e {printf "%6d\t%s\n", NR, $0}' "$FILE"
fi
echo "=== Relocate sequence record implementation (if present) ==="
rg -n "spock_seqam_relocate_sequence_record\\s*\\(" "$FILE" || true
line=$(rg -n "spock_seqam_relocate_sequence_record\\s*\\(" "$FILE" | head -n1 | cut -d: -f1 || true)
if [[ -n "${line:-}" ]]; then
start=$((line-30)); end=$((line+120));
awk -v s=$start -v e=$end 'NR>=s && NR<=e {printf "%6d\t%s\n", NR, $0}' "$FILE"
fi
# Inspect security-context helper definitions
echo "=== GetUserIdAndSecContext / SetUserIdAndSecContext implementations ==="
rg -n "GetUserIdAndSecContext\\s*\\(|SetUserIdAndSecContext\\s*\\(" src/spock_common.c src -S | head -n 50
echo "=== Show spock_common.c around those helpers ==="
FILE="src/spock_common.c"
# Show early part of file where they likely live
awk 'NR>=1 && NR<=140 {printf "%6d\t%s\n", NR, $0}' "$FILE"Repository: pgEdge/spock
Length of output: 19045
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/spock_seqam.c"
echo "=== ereport(ERROR) occurrences in file (to see if helpers can throw) ==="
rg -n "ereport\\(ERROR" "$FILE" || true
echo "=== Show seqam_resolve_seq_name implementation (definition + any ERROR) ==="
rg -n "seqam_resolve_seq_name\\s*\\(" "$FILE"
line=$(rg -n "seqam_resolve_seq_name\\s*\\(" "$FILE" | head -n1 | cut -d: -f1)
start=$((line-60)); end=$((line+120));
awk -v s=$start -v e=$end 'NR>=s && NR<=e {printf "%6d\t%s\n", NR, $0}' "$FILE"
echo "=== Scan inside drop/relocate functions for ereport/ERROR-like calls ==="
# approximate ranges from earlier output: drop around 653-695, relocate around 715-807
awk 'NR>=640 && NR<=710 {print}' "$FILE" | rg -n "ereport\\(|elog\\(|ERROR" || true
awk 'NR>=700 && NR<=830 {print}' "$FILE" | rg -n "ereport\\(|elog\\(|ERROR" || trueRepository: pgEdge/spock
Length of output: 6811
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find where GetUserIdAndSecContext / SetUserIdAndSecContext are defined
rg -n "GetUserIdAndSecContext\\s*\\(" -S src | head -n 50
rg -n "SetUserIdAndSecContext\\s*\\(" -S src | head -n 50
# Specifically look for definitions (not just calls) by matching with return type patterns
rg -n "void\\s+GetUserIdAndSecContext\\b|static\\s+void\\s+GetUserIdAndSecContext\\b|GetUserIdAndSecContext\\b\\s*\\{" -S src || true
rg -n "void\\s+SetUserIdAndSecContext\\b|static\\s+void\\s+SetUserIdAndSecContext\\b|SetUserIdAndSecContext\\b\\s*\\{" -S src || true
# Confirm whether any ereport/elog appears within the drop/relocate functions ranges
FILE="src/spock_seqam.c"
echo "=== ereport/elog inside drop function range (640-710) ==="
awk 'NR>=640 && NR<=710 {print}' "$FILE" | rg -n "ereport\\(|elog\\(|ERROR" || true
echo "=== ereport/elog inside relocate function range (700-820) ==="
awk 'NR>=700 && NR<=820 {print}' "$FILE" | rg -n "ereport\\(|elog\\(|ERROR" || trueRepository: pgEdge/spock
Length of output: 1687
Harden the BOOTSTRAP_SUPERUSERID elevation with PG_TRY/PG_FINALLY (both sequence blocks)
The two sequences blocks in src/spock_executor.c (drop cleanup and OAT_POST_ALTER relocation) restore the caller’s userid only on the normal return path. The SeqAM helpers don’t explicitly ereport(ERROR) in their bodies, but catalog/syscache operations they call could still throw an ERROR; wrapping those elevations in PG_TRY/PG_FINALLY would guarantee SetUserIdAndSecContext(saved_*...) runs even on ERROR.
🤖 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 253 - 258, Wrap the temporary elevation to
BOOTSTRAP_SUPERUSERID around the SeqAM calls with a PG_TRY / PG_FINALLY pair so
the original credentials are restored even if an ERROR is thrown: call
GetUserIdAndSecContext(&save_userid, &save_sec_context); then
SetUserIdAndSecContext(BOOTSTRAP_SUPERUSERID, save_sec_context |
SECURITY_LOCAL_USERID_CHANGE); invoke spock_seqam_drop_sequence_record(objectId)
(and apply the same pattern for the OAT_POST_ALTER relocation block), and in
PG_FINALLY call SetUserIdAndSecContext(save_userid, save_sec_context); end with
PG_END_TRY(); ensure you keep the same local variables (save_userid,
save_sec_context) and the BOOTSTRAP_SUPERUSERID/SECURITY_LOCAL_USERID_CHANGE
flags so the restoration always runs on error or normal return.
Comment on lines
+168
to
+181
| /* | ||
| * Derive a Spock node id from a node name. Truncated to 16 bits so the | ||
| * value fits cleanly inside an Oid column and leaves room for downstream | ||
| * uses (snowflake sequences mask off the low 10 bits as their node_id). | ||
| * Same input -> same output across nodes; callers that need cluster-wide | ||
| * uniqueness must verify by inspecting peers. | ||
| */ | ||
| Oid | ||
| spock_node_id_for_name(const char *name) | ||
| { | ||
| return (Oid) | ||
| (DatumGetUInt32(hash_any((const unsigned char *) name, | ||
| strlen(name))) & 0xffff); | ||
| } |
There was a problem hiding this comment.
Validate the Snowflake node-id domain before storing the node.
This path accepts any 16-bit Oid, but Snowflake only encodes the low 10 bits (SPOCK_SNOWFLAKE_NODE_BITS). Two different node IDs that differ outside those 10 bits can both be stored here and later emit identical Snowflake values on different nodes. hash_any(...) & 0xffff can also produce 0, so this helper can persist InvalidOid as a generated node id.
Please reject InvalidOid and enforce uniqueness on the effective Snowflake node id (node->id & SPOCK_SNOWFLAKE_MAX_NODE_ID) before inserting the catalog row.
Also applies to: 200-202
🤖 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_node.c` around lines 168 - 181, The helper spock_node_id_for_name
currently returns any 16-bit Oid (possibly 0/InvalidOid) and doesn't ensure
uniqueness in the Snowflake domain; update logic so spock_node_id_for_name never
returns InvalidOid (reject or rehash until non-zero) and, where nodes are
inserted (the code path that uses node->id), validate that (node->id &
SPOCK_SNOWFLAKE_MAX_NODE_ID) is unique before writing the catalog row; if the
masked value collides, return an error to caller and refuse insert. Ensure you
reference SPOCK_SNOWFLAKE_NODE_BITS / SPOCK_SNOWFLAKE_MAX_NODE_ID for masking
and treat InvalidOid specially.
Comment on lines
+547
to
+556
| if (!seqam_catalog_lookup(seqoid, &kind)) | ||
| kind = spock_seqam_default_kind; | ||
|
|
||
| /* Re-create if a callback nuked the cache during the lookup. */ | ||
| if (SeqAmCache == NULL) | ||
| seqam_init_cache(); | ||
|
|
||
| entry = (SeqAmCacheEntry *) hash_search(SeqAmCache, &seqoid, | ||
| HASH_ENTER, &found); | ||
| entry->kind = kind; |
There was a problem hiding this comment.
Revalidate non-local kinds when repopulating the cache.
validate_sequence_target() only runs in the SQL admin entry points. After a relcache invalidation, or when spock.default_sequence_kind = snowflake, this path caches a managed kind without rechecking AS bigint / NO MAXVALUE / CACHE 1 / positive INCREMENT. That lets an incompatible or later-altered sequence fall through into spock_seqam_snowflake_nextval() with semantics it cannot honor.
Suggested fix
- if (!seqam_catalog_lookup(seqoid, &kind))
- kind = spock_seqam_default_kind;
+ if (!seqam_catalog_lookup(seqoid, &kind))
+ kind = spock_seqam_default_kind;
+
+ if (kind != SPOCK_SEQAM_LOCAL)
+ validate_sequence_target(seqoid, kind);🤖 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_seqam.c` around lines 547 - 556, When repopulating SeqAmCache you
must revalidate non-local managed kinds so we don't cache an incompatible
sequence; after getting kind from seqam_catalog_lookup (and after
seqam_init_cache if needed) add a call to validate_sequence_target(seqoid) for
non-default kinds and, if that validation fails, treat the sequence as unmanaged
(e.g. set kind = spock_seqam_default_kind) before creating the SeqAmCacheEntry;
this prevents an invalid managed kind from being cached and later passed to
spock_seqam_snowflake_nextval().
Comment on lines
+564
to
+568
| /* Managed dispatch is unsupported in these states. */ | ||
| Assert(!RecoveryInProgress()); | ||
| Assert(!creating_extension); | ||
| Assert(!IsBinaryUpgrade); | ||
| Assert(kind > SPOCK_SEQAM_LOCAL && kind < SPOCK_SEQAM_NMETHODS); |
There was a problem hiding this comment.
Turn these unsupported-state asserts into runtime guards.
In production builds, these Asserts disappear. A cluster-wide spock.default_sequence_kind = snowflake can still route nextval() calls made during extension creation or binary upgrade into a path the code explicitly says is unsupported.
Suggested fix
- Assert(!RecoveryInProgress());
- Assert(!creating_extension);
- Assert(!IsBinaryUpgrade);
+ if (RecoveryInProgress() || creating_extension || IsBinaryUpgrade)
+ return sequence_am_local_nextval(rel, incby, maxv, minv,
+ cache, cycle, last);📝 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
| /* Managed dispatch is unsupported in these states. */ | |
| Assert(!RecoveryInProgress()); | |
| Assert(!creating_extension); | |
| Assert(!IsBinaryUpgrade); | |
| Assert(kind > SPOCK_SEQAM_LOCAL && kind < SPOCK_SEQAM_NMETHODS); | |
| /* Managed dispatch is unsupported in these states. */ | |
| if (RecoveryInProgress() || creating_extension || IsBinaryUpgrade) | |
| return sequence_am_local_nextval(rel, incby, maxv, minv, | |
| cache, cycle, last); | |
| Assert(kind > SPOCK_SEQAM_LOCAL && kind < SPOCK_SEQAM_NMETHODS); |
🤖 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_seqam.c` around lines 564 - 568, Change the unsupported-state
Assert checks into runtime guards: replace the Assert(...) calls for
RecoveryInProgress(), creating_extension, IsBinaryUpgrade and the kind range
check (kind > SPOCK_SEQAM_LOCAL && kind < SPOCK_SEQAM_NMETHODS) with an if that
detects any of these conditions at runtime and raises a PostgreSQL error
(ereport/elog) returning ERRCODE_FEATURE_NOT_SUPPORTED and a clear errmsg
explaining that the requested spock.default_sequence_kind (e.g. "snowflake") is
unsupported during recovery, extension creation or binary-upgrade; reference the
symbols RecoveryInProgress, creating_extension, IsBinaryUpgrade, kind,
SPOCK_SEQAM_LOCAL, SPOCK_SEQAM_NMETHODS and mention spock.default_sequence_kind
in the message so callers get a descriptive runtime error instead of an
assertion in production.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
docs/distributed_sequences.md (2)
282-283:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix duplicate ordered-list numbering.
The list currently goes
1, 2, 3, 3at Line 282. The final item should be4..🤖 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 `@docs/distributed_sequences.md` around lines 282 - 283, Fix the duplicate ordered-list numbering by changing the final list item label from "3." to "4." so the sequence reads 1, 2, 3, 4; locate the ordered list containing the lines mentioning "nextval()" and "Snowflake" and update the numeric marker for that last item to "4." to correct the Markdown numbering.
17-19:⚠️ Potential issue | 🟠 Major | ⚡ Quick winConflicting replication semantics for
spock.sequence_kindare still present.Line 17 says assignments replicate via
user_catalog_table, but Line 80-87 says changes are node-local and not replicated; Line 243-244 and Line 280-281 repeat the “replicates” wording. Please pick one behavior and align all sections so operator guidance is unambiguous.Also applies to: 222-224, 243-244, 280-281
🤖 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 `@docs/distributed_sequences.md` around lines 17 - 19, The document currently contains conflicting statements about whether spock.sequence_kind assignments replicate via Spock's user_catalog_table or are node-local; pick one definitive behavior and make the wording consistent across all places that mention sequence replication (every paragraph referencing spock.sequence_kind, user_catalog_table, or saying "replicates" vs "node-local" — e.g., the sections that describe assignment/replication semantics and the paragraphs that describe changes being node-local). Update those sections to use the chosen wording, adjust any operator guidance to match that behavior (including any instructions for making changes cluster-wide vs per-node), and ensure the terms "replicates", "replicated", and "node-local" are used unambiguously and consistently wherever spock.sequence_kind is discussed.
🧹 Nitpick comments (2)
src/spock_executor.c (1)
250-251: 💤 Low valueVariable shadowing: inner
save_userid/save_sec_contexthide outer declarations.Lines 205-206 already declare
save_useridandsave_sec_contextat function scope (used later in the OAT_DROP path at lines 274-293). The inner declarations here shadow those, which compiles but is confusing and could lead to bugs if someone later tries to use the outer variables expecting them to be set.Consider removing the inner declarations and reusing the outer ones, or rename these to match the OAT_POST_ALTER block's naming (
saved_userid,saved_sec_context).♻️ Suggested fix
if (get_rel_relkind(objectId) == RELKIND_SEQUENCE && !dropping_spock_obj) { - Oid save_userid; - int save_sec_context; - GetUserIdAndSecContext(&save_userid, &save_sec_context); SetUserIdAndSecContext(BOOTSTRAP_SUPERUSERID, save_sec_context |🤖 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 250 - 251, The inner declarations "Oid save_userid; int save_sec_context;" inside the OAT_DROP handling shadow the function-scope variables with the same names declared earlier; remove these inner declarations and reuse the outer save_userid and save_sec_context, or rename the inner ones to match the OAT_POST_ALTER pattern (saved_userid, saved_sec_context) so there is no shadowing; update any assignments/uses in the OAT_DROP block (and corresponding restore logic later) to reference the chosen uniquely named variables (save_userid/save_sec_context or saved_userid/saved_sec_context) consistently.src/spock_seqam_snowflake.c (1)
239-262: 💤 Low valueConfirm the design decision to silently ignore
maxv,minv,cache,cycle.The function casts these to
(void)and never validates against them. For users this meansALTER SEQUENCE ... MAXVALUE / MINVALUE / CYCLE / CACHEon a snowflake-managed sequence has no effect on the emitted values — snowflakes can and will exceed the configured MAXVALUE without error. If this is the intended contract (which the file header and PR summary suggest it is), it's worth at least anelog(DEBUG2, ...)or a one-line comment near the cast to make the silence explicit, or alternatively a guard inalter_sequence_set_kind()so that switching kind rejects non-defaults to fail loudly.🤖 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_seqam_snowflake.c` around lines 239 - 262, spock_seqam_snowflake_nextval currently ignores maxv, minv, cache, and cycle by casting them to (void) which can surprise users; either make the silence explicit by adding an elog(DEBUG2, "...") in spock_seqam_snowflake_nextval near the (void) casts (mentioning that snowflake-managed sequences ignore MAXVALUE/MINVALUE/CACHE/CYCLE), or instead add a guard in alter_sequence_set_kind() to reject/non-accept non-default MAXVALUE/MINVALUE/CACHE/CYCLE when switching to the snowflake kind (e.g., validate and elog(ERROR) or return an error), and document the behavior in a one-line comment adjacent to the (void) casts; reference spock_seqam_snowflake_nextval and alter_sequence_set_kind when implementing the change.
🤖 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 `@docs/distributed_sequences.md`:
- Around line 140-146: The fenced code block showing the bit layout (the block
starting with the three backticks and lines like "bit 63 : reserved" and "bits
62..22 : 41-bit timestamp") is missing a language identifier and triggers
markdownlint MD040; update the opening fence from ``` to ```text (or another
appropriate language) so the block is explicitly marked as plain text, leaving
the interior content (bit 63, bits 62..22, bits 21..12, bits 11..0) unchanged.
In `@src/spock_seqam_snowflake.c`:
- Around line 318-361: The candidate_ctr handling fails to guard negative incby
values; update the overflow check that currently tests "candidate_ctr >
SPOCK_SNOWFLAKE_COUNTER_MASK" to also detect "candidate_ctr < 0" and treat it as
an error path: when candidate_ctr is out of [0, SPOCK_SNOWFLAKE_COUNTER_MASK]
(i.e., < 0 or > COUNTER_MASK) release the buffer (UnlockReleaseBuffer(buf)) and
raise an ereport(ERROR) similar to the existing branch so we never cast a
negative candidate_ctr into new_ctr; use the same error code/message pattern and
reference the same symbols (candidate_ctr, incby, new_ctr,
SPOCK_SNOWFLAKE_COUNTER_MASK, new_ts, UnlockReleaseBuffer, ereport) so the
negative-INCREMENT case is rejected rather than producing a negative new_ctr
that corrupts the encoded ID.
- Around line 147-157: sf_unpack is extracting the timestamp from the wrong bit
range because SF_STATE_TS_SHIFT is defined as SPOCK_SNOWFLAKE_COUNTER_BITS (12)
instead of the full SPOCK_SNOWFLAKE_TIMESTAMP_SHIFT (node_bits + counter_bits =
22); update the mask/shift definitions used by sf_unpack so the timestamp is
decoded with SPOCK_SNOWFLAKE_TIMESTAMP_SHIFT (and SF_STATE_TS_MASK is computed
from that shift), or directly use SPOCK_SNOWFLAKE_TIMESTAMP_SHIFT in sf_unpack
to extract *ts from packed; keep SF_STATE_CTR_MASK extraction for *ctr
unchanged.
---
Duplicate comments:
In `@docs/distributed_sequences.md`:
- Around line 282-283: Fix the duplicate ordered-list numbering by changing the
final list item label from "3." to "4." so the sequence reads 1, 2, 3, 4; locate
the ordered list containing the lines mentioning "nextval()" and "Snowflake" and
update the numeric marker for that last item to "4." to correct the Markdown
numbering.
- Around line 17-19: The document currently contains conflicting statements
about whether spock.sequence_kind assignments replicate via Spock's
user_catalog_table or are node-local; pick one definitive behavior and make the
wording consistent across all places that mention sequence replication (every
paragraph referencing spock.sequence_kind, user_catalog_table, or saying
"replicates" vs "node-local" — e.g., the sections that describe
assignment/replication semantics and the paragraphs that describe changes being
node-local). Update those sections to use the chosen wording, adjust any
operator guidance to match that behavior (including any instructions for making
changes cluster-wide vs per-node), and ensure the terms "replicates",
"replicated", and "node-local" are used unambiguously and consistently wherever
spock.sequence_kind is discussed.
---
Nitpick comments:
In `@src/spock_executor.c`:
- Around line 250-251: The inner declarations "Oid save_userid; int
save_sec_context;" inside the OAT_DROP handling shadow the function-scope
variables with the same names declared earlier; remove these inner declarations
and reuse the outer save_userid and save_sec_context, or rename the inner ones
to match the OAT_POST_ALTER pattern (saved_userid, saved_sec_context) so there
is no shadowing; update any assignments/uses in the OAT_DROP block (and
corresponding restore logic later) to reference the chosen uniquely named
variables (save_userid/save_sec_context or saved_userid/saved_sec_context)
consistently.
In `@src/spock_seqam_snowflake.c`:
- Around line 239-262: spock_seqam_snowflake_nextval currently ignores maxv,
minv, cache, and cycle by casting them to (void) which can surprise users;
either make the silence explicit by adding an elog(DEBUG2, "...") in
spock_seqam_snowflake_nextval near the (void) casts (mentioning that
snowflake-managed sequences ignore MAXVALUE/MINVALUE/CACHE/CYCLE), or instead
add a guard in alter_sequence_set_kind() to reject/non-accept non-default
MAXVALUE/MINVALUE/CACHE/CYCLE when switching to the snowflake kind (e.g.,
validate and elog(ERROR) or return an error), and document the behavior in a
one-line comment adjacent to the (void) casts; reference
spock_seqam_snowflake_nextval and alter_sequence_set_kind when implementing the
change.
🪄 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: cd1f675b-b936-4bce-9903-afe3e819912c
⛔ Files ignored due to path filters (1)
tests/regress/expected/init.outis excluded by!**/*.out
📒 Files selected for processing (22)
docs/distributed_sequences.mdinclude/spock_node.hinclude/spock_seqam.hmkdocs.ymlpatches/15/pg15-050-nextval-hook.diffpatches/16/pg16-050-nextval-hook.diffpatches/17/pg17-050-nextval-hook.diffpatches/18/pg18-050-nextval-hook.diffsql/spock--6.0.0.sqlsrc/compat/15/spock_compat.hsrc/compat/16/spock_compat.hsrc/compat/17/spock_compat.hsrc/compat/18/spock_compat.hsrc/spock.csrc/spock_apply.csrc/spock_executor.csrc/spock_node.csrc/spock_seqam.csrc/spock_seqam_snowflake.csrc/spock_sequences.ctests/regress/sql/init.sqltests/tap/schedule
💤 Files with no reviewable changes (4)
- src/compat/16/spock_compat.h
- src/compat/15/spock_compat.h
- src/compat/18/spock_compat.h
- src/compat/17/spock_compat.h
✅ Files skipped from review due to trivial changes (1)
- mkdocs.yml
Comment on lines
+140
to
+146
| ``` | ||
| bit 63 : reserved, always 0 (sign bit, keeps values non-negative) | ||
| bits 62..22 : 41-bit timestamp in milliseconds since the Spock epoch | ||
| (2023-01-01 00:00:00 UTC; matches the standalone `snowflake` extension) | ||
| bits 21..12 : 10-bit node id (0..1023) | ||
| bits 11..0 : 12-bit counter (0..4095) | ||
| ``` |
There was a problem hiding this comment.
Add a language to the fenced code block.
The fence at Line 140 is missing a language identifier and trips markdownlint MD040. Use something like ```text for the bit layout block.
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 140-140: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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 `@docs/distributed_sequences.md` around lines 140 - 146, The fenced code block
showing the bit layout (the block starting with the three backticks and lines
like "bit 63 : reserved" and "bits 62..22 : 41-bit timestamp") is missing a
language identifier and triggers markdownlint MD040; update the opening fence
from ``` to ```text (or another appropriate language) so the block is explicitly
marked as plain text, leaving the interior content (bit 63, bits 62..22, bits
21..12, bits 11..0) unchanged.
Comment on lines
+147
to
+157
| #define SF_STATE_TS_SHIFT SPOCK_SNOWFLAKE_COUNTER_BITS | ||
| #define SF_STATE_CTR_MASK SPOCK_SNOWFLAKE_COUNTER_MASK | ||
| #define SF_STATE_TS_MASK (((UINT64CONST(1) << SPOCK_SNOWFLAKE_TIMESTAMP_BITS) - 1) \ | ||
| << SF_STATE_TS_SHIFT) | ||
|
|
||
| static inline void | ||
| sf_unpack(uint64 packed, int64 *ts, int32 *ctr) | ||
| { | ||
| *ctr = (int32) (packed & SF_STATE_CTR_MASK); | ||
| *ts = (int64) ((packed & SF_STATE_TS_MASK) >> SF_STATE_TS_SHIFT); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Dump the snowflake bit-layout constants in the header and any uses elsewhere.
fd -t f 'spock_seqam.h' | xargs -I{} sh -c 'echo "==== {} ===="; cat "{}"'
echo "---- uses of TIMESTAMP_SHIFT / NODE_SHIFT / COUNTER_BITS / NODE_MASK ----"
rg -nP '\bSPOCK_SNOWFLAKE_(TIMESTAMP_SHIFT|NODE_SHIFT|NODE_MASK|COUNTER_BITS|TIMESTAMP_BITS|COUNTER_MASK|MAX_NODE_ID)\b' -C2Repository: pgEdge/spock
Length of output: 12560
🏁 Script executed:
#!/bin/bash
# Inspect incby validation context in src/spock_seqam.c
sed -n '900,990p' src/spock_seqam.c
# Where sf_unpack / SF_STATE_TS_SHIFT are used
rg -n "SF_STATE_TS_SHIFT|sf_unpack\s*\(" -S src/spock_seqam_snowflake.cRepository: pgEdge/spock
Length of output: 3732
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '120,190p' src/spock_seqam_snowflake.c
sed -n '260,330p' src/spock_seqam_snowflake.c
sed -n '360,420p' src/spock_seqam_snowflake.c
sed -n '430,520p' src/spock_seqam_snowflake.cRepository: pgEdge/spock
Length of output: 8971
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "INCREMENT|increment" src/spock_seqam.c src/spock_executor.c src/spock_seqam_snowflake.c include/spock_seqam.h
rg -n "SPOCK_SNOWFLAKE_COUNTER_MASK" src/spock_seqam.c src/spock_seqam_snowflake.c src/spock_executor.cRepository: pgEdge/spock
Length of output: 1387
Fix sf_unpack to decode the timestamp using SPOCK_SNOWFLAKE_TIMESTAMP_SHIFT
include/spock_seqam.h defines SPOCK_SNOWFLAKE_TIMESTAMP_SHIFT = (SPOCK_SNOWFLAKE_NODE_BITS + SPOCK_SNOWFLAKE_COUNTER_BITS) = 22 and SPOCK_SNOWFLAKE_NODE_SHIFT = 12. seq->last_value is written using new_ts << SPOCK_SNOWFLAKE_TIMESTAMP_SHIFT, but cur_packed = seq->last_value & ~SPOCK_SNOWFLAKE_NODE_MASK only zeroes node bits (does not shift the timestamp). sf_unpack currently sets SF_STATE_TS_SHIFT to SPOCK_SNOWFLAKE_COUNTER_BITS (12), so it extracts the timestamp from the wrong bit range and drops the upper bits, corrupting cur_ts and the subsequent control/WAL decisions.
Diff
-#define SF_STATE_TS_SHIFT SPOCK_SNOWFLAKE_COUNTER_BITS
+#define SF_STATE_TS_SHIFT SPOCK_SNOWFLAKE_TIMESTAMP_SHIFT
`#define` SF_STATE_CTR_MASK SPOCK_SNOWFLAKE_COUNTER_MASK
`#define` SF_STATE_TS_MASK (((UINT64CONST(1) << SPOCK_SNOWFLAKE_TIMESTAMP_BITS) - 1) \
<< SF_STATE_TS_SHIFT)Negative INCREMENT handling is already rejected up-front (spock_seqam.c errors on incby <= 0), so that specific concern doesn’t apply here.
🤖 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_seqam_snowflake.c` around lines 147 - 157, sf_unpack is extracting
the timestamp from the wrong bit range because SF_STATE_TS_SHIFT is defined as
SPOCK_SNOWFLAKE_COUNTER_BITS (12) instead of the full
SPOCK_SNOWFLAKE_TIMESTAMP_SHIFT (node_bits + counter_bits = 22); update the
mask/shift definitions used by sf_unpack so the timestamp is decoded with
SPOCK_SNOWFLAKE_TIMESTAMP_SHIFT (and SF_STATE_TS_MASK is computed from that
shift), or directly use SPOCK_SNOWFLAKE_TIMESTAMP_SHIFT in sf_unpack to extract
*ts from packed; keep SF_STATE_CTR_MASK extraction for *ctr unchanged.
Comment on lines
+318
to
+361
| * Do the arithmetic in int64 so a user who ALTERs INCREMENT to | ||
| * a large value after the kind was set (bypassing | ||
| * validate_sequence_target) cannot trigger int32 overflow here. | ||
| * validate_sequence_target enforces 1..COUNTER_MASK at SET KIND | ||
| * time; this is belt-and-braces. | ||
| */ | ||
| new_ts = cur_ts; | ||
| candidate_ctr = (int64) cur_ctr + incby; | ||
|
|
||
| if (candidate_ctr > (int64) SPOCK_SNOWFLAKE_COUNTER_MASK) | ||
| { | ||
| /* | ||
| * Counter exhausted in this millisecond. Advance the | ||
| * timestamp by one ms and reset the counter. We are now | ||
| * synthesising a future timestamp; the wall clock will | ||
| * catch up. | ||
| */ | ||
| new_ts = cur_ts + 1; | ||
| new_ctr = 0; | ||
|
|
||
| /* | ||
| * Sustained > 4M nextval()/s/sequence/node could push the | ||
| * synthesised timestamp arbitrarily far into the future and | ||
| * ultimately overflow the 41-bit field. Refuse the write | ||
| * here rather than silently wrapping. | ||
| */ | ||
| if (((uint64) new_ts) >> SPOCK_SNOWFLAKE_TIMESTAMP_BITS) | ||
| { | ||
| UnlockReleaseBuffer(buf); | ||
| ereport(ERROR, | ||
| (errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED), | ||
| errmsg("snowflake timestamp field exhausted under saturation"), | ||
| errdetail("Per-millisecond counter exhaustion has pushed the " | ||
| "synthesised timestamp beyond the 41-bit field " | ||
| "(year ~2092). Reduce sustained nextval() throughput " | ||
| "or assign additional snowflake-managed sequences " | ||
| "to spread the load."))); | ||
| } | ||
| } | ||
| else | ||
| { | ||
| /* Safe: candidate_ctr is in [0, COUNTER_MASK], fits int32. */ | ||
| new_ctr = (int32) candidate_ctr; | ||
| } |
There was a problem hiding this comment.
Belt-and-braces only guards the upper bound on candidate_ctr.
The comment explicitly anticipates a user ALTER SEQUENCE … INCREMENT BY <n> bypassing validate_sequence_target, but the check at line 327 only catches candidate_ctr > SPOCK_SNOWFLAKE_COUNTER_MASK. A negative incby (e.g., user altered INCREMENT to -1) produces a negative candidate_ctr, which then casts to a negative int32 new_ctr at line 360. At line 387 that new_ctr is widened with (uint64) new_ctr, sign-extending the value into the timestamp/node-id bit ranges of value and corrupting the encoded ID. Adding a candidate_ctr < 0 arm to the same check would close the gap.
🛡 Proposed fix
- if (candidate_ctr > (int64) SPOCK_SNOWFLAKE_COUNTER_MASK)
+ if (candidate_ctr < 0 ||
+ candidate_ctr > (int64) SPOCK_SNOWFLAKE_COUNTER_MASK)
{
/*
- * Counter exhausted in this millisecond. Advance the
- * timestamp by one ms and reset the counter. We are now
- * synthesising a future timestamp; the wall clock will
- * catch up.
+ * Counter exhausted in this millisecond (or `incby` is
+ * non-positive after a stray ALTER SEQUENCE). Advance
+ * the timestamp by one ms and reset the counter. We are
+ * now synthesising a future timestamp; the wall clock
+ * will catch up.
*/🤖 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_seqam_snowflake.c` around lines 318 - 361, The candidate_ctr
handling fails to guard negative incby values; update the overflow check that
currently tests "candidate_ctr > SPOCK_SNOWFLAKE_COUNTER_MASK" to also detect
"candidate_ctr < 0" and treat it as an error path: when candidate_ctr is out of
[0, SPOCK_SNOWFLAKE_COUNTER_MASK] (i.e., < 0 or > COUNTER_MASK) release the
buffer (UnlockReleaseBuffer(buf)) and raise an ereport(ERROR) similar to the
existing branch so we never cast a negative candidate_ctr into new_ctr; use the
same error code/message pattern and reference the same symbols (candidate_ctr,
incby, new_ctr, SPOCK_SNOWFLAKE_COUNTER_MASK, new_ts, UnlockReleaseBuffer,
ereport) so the negative-INCREMENT case is rejected rather than producing a
negative new_ctr that corrupts the encoded ID.
Introduces a sequence access method framework in Spock plus a snowflake-ID provider, so nextval() on a participating sequence yields globally unique, monotonically increasing IDs without cross-node coordination. Includes a PG18 nextval-hook patch, catalogue entries for spock 6.0.0-devel, docs, and regress/TAP coverage.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (8)
docs/distributed_sequences.md (2)
17-19:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReplication semantics for
spock.sequence_kindstill contradict each other.Lines 17–19 state assignments replicate to all nodes via
user_catalog_table, but Lines 80–95 explicitly sayalter_sequence_set_kind()is node-local and the row does not replicate (andspock.sequence_kindis intentionally not auser_catalog_table). Then Lines 222–227 swing back to "spock.sequence_kind... is auser_catalog_tablereplicated to peer nodes", and the security implication built on top of that ("Any role you grantEXECUTE... can change sequence behaviour on every node") only holds under the replicated interpretation. Pick one model and reconcile all three sections plus the security implications; otherwise operators will draw opposite conclusions about cluster-wide uniqueness and the trust model.Also applies to: 80-95, 222-227
🤖 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 `@docs/distributed_sequences.md` around lines 17 - 19, Choose a single replication model for spock.sequence_kind and update all references to be consistent: either document that spock.sequence_kind is a user_catalog_table and thus replicated to peers (and adjust the description of alter_sequence_set_kind() to be cluster-visible, plus keep the security implication that EXECUTE can change behavior across all nodes), or document that spock.sequence_kind is node-local (and mark alter_sequence_set_kind() as non-replicated, remove/adjust any claims about cluster-wide changes and the EXECUTE security scope). Specifically, reconcile and edit the paragraphs mentioning spock.sequence_kind (lines ~17–19 and ~222–227), the description and behavior of alter_sequence_set_kind() (lines ~80–95), and the security implication about EXECUTE so they all state the same replication semantics and trust model.
140-146:⚠️ Potential issue | 🟡 Minor | 💤 Low valueFenced block still missing a language identifier (MD040).
✏️ Proposed fix
-``` +```text bit 63 : reserved, always 0 (sign bit, keeps values non-negative)🤖 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 `@docs/distributed_sequences.md` around lines 140 - 146, The fenced code block showing the bit-field layout (the lines starting with "bit 63 : reserved..." through "bits 11..0 : 12-bit counter") lacks a language identifier; update the opening fence from ``` to ```text so the block becomes a text code fence and satisfies MD040.src/spock_seqam.c (2)
564-568:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPast review comment still applies: turn unsupported-state asserts into runtime guards.
In production builds, these
Asserts are compiled out. Whenspock.default_sequence_kind = snowflake, nextval() calls during recovery, extension creation, or binary upgrade would bypass the local fallback and invoke the dispatcher with unsupported state.Please see the existing review comment for the recommended fix.
🤖 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_seqam.c` around lines 564 - 568, Replace the compile-time Assert checks (Assert(!RecoveryInProgress()), Assert(!creating_extension), Assert(!IsBinaryUpgrade), Assert(kind > SPOCK_SEQAM_LOCAL && kind < SPOCK_SEQAM_NMETHODS)) with runtime guards that detect these unsupported states and force a local fallback instead of asserting; specifically, in the spock_seqam dispatch path where these asserts appear, check if RecoveryInProgress() || creating_extension || IsBinaryUpgrade || !(kind > SPOCK_SEQAM_LOCAL && kind < SPOCK_SEQAM_NMETHODS) and if so set kind = SPOCK_SEQAM_LOCAL (or otherwise route to the local sequence handler) so nextval() will use the local implementation in those states rather than invoking the managed dispatcher.
547-556:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPast review comment still applies: revalidate non-local kinds when repopulating the cache.
When
spock.default_sequence_kind = snowflake, this path caches a managed kind without rechecking the sequence's compatibility constraints (AS bigint,NO MAXVALUE,CACHE 1, positiveINCREMENT). An incompatible or later-altered sequence can reachspock_seqam_snowflake_nextval()with semantics it cannot honor.Please see the existing review comment for the recommended fix.
🤖 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_seqam.c` around lines 547 - 556, When repopulating SeqAmCache you must revalidate any non-local kind before caching to avoid storing an incompatible managed kind (e.g. when spock_seqam_default_kind == snowflake); add a validation step after determining kind (between the seqam_catalog_lookup/seqam_init_cache block and the hash_search) that calls a sequence-attribute checker (implement a helper such as spock_seqam_validate_sequence_for_kind(seqoid, kind)) to verify the sequence has AS bigint, NO MAXVALUE, CACHE 1, positive INCREMENT, etc.; if validation fails, fall back to a safe kind (e.g. LOCAL or the default-safe kind) and only then insert into SeqAmCache (entry->kind), so spock_seqam_snowflake_nextval() never gets an incompatible sequence kind cached.src/spock_node.c (1)
168-181:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPast review comment still applies: validate Snowflake node-id domain and reject InvalidOid.
The current implementation can return
InvalidOidwhenhash_any() & 0xffffproduces0, and does not validate uniqueness of the low 10 bits used by Snowflake encoding. Two nodes whose IDs differ only outsideSPOCK_SNOWFLAKE_NODE_BITSwould emit identical Snowflake values.Please see the existing review comment for the recommended fix.
🤖 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_node.c` around lines 168 - 181, spock_node_id_for_name currently returns 0 (InvalidOid) or a value whose low SPOCK_SNOWFLAKE_NODE_BITS are all zero; change it so after computing raw = (hash_any(...) & 0xffff) you validate and fix the value: if raw == InvalidOid (0) set raw to a non-zero value (e.g. 1), and if (raw & ((1 << SPOCK_SNOWFLAKE_NODE_BITS) - 1)) == 0 set the low SPOCK_SNOWFLAKE_NODE_BITS to a non-zero pattern (while preserving the other bits) before casting to Oid; keep all work inside spock_node_id_for_name and use the SPOCK_SNOWFLAKE_NODE_BITS constant to build the mask.src/spock_executor.c (1)
253-258:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRestore the caller's security context in
PG_FINALLY.At Line 253 and Line 310, the code elevates to
BOOTSTRAP_SUPERUSERIDbut restores the original userid only on the normal return path. If either SeqAM helper throws, the surrounding error handling continues under the elevated context.Suggested pattern
- GetUserIdAndSecContext(&save_userid, &save_sec_context); - SetUserIdAndSecContext(BOOTSTRAP_SUPERUSERID, - save_sec_context | - SECURITY_LOCAL_USERID_CHANGE); - spock_seqam_drop_sequence_record(objectId); - SetUserIdAndSecContext(save_userid, save_sec_context); + GetUserIdAndSecContext(&save_userid, &save_sec_context); + PG_TRY(); + { + SetUserIdAndSecContext(BOOTSTRAP_SUPERUSERID, + save_sec_context | + SECURITY_LOCAL_USERID_CHANGE); + spock_seqam_drop_sequence_record(objectId); + } + PG_FINALLY(); + { + SetUserIdAndSecContext(save_userid, save_sec_context); + } + PG_END_TRY();Apply the same pattern to the
spock_seqam_relocate_sequence_record(objectId)block.Also applies to: 310-315
🤖 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 253 - 258, The code elevates privileges using GetUserIdAndSecContext/SetUserIdAndSecContext before calling spock_seqam_drop_sequence_record and spock_seqam_relocate_sequence_record but only restores the original context on the normal path; if the SeqAM helpers throw, the elevated context remains. Fix both blocks by wrapping the SetUserIdAndSecContext(BOOTSTRAP_SUPERUSERID...) and the subsequent spock_seqam_* call in a PG_TRY { ... } PG_FINALLY { SetUserIdAndSecContext(save_userid, save_sec_context); } PG_END_TRY; so the original user/security context is always restored (use the same save_userid/save_sec_context variables and apply the pattern around spock_seqam_drop_sequence_record(objectId) and spock_seqam_relocate_sequence_record(objectId)).sql/spock--6.0.0.sql (1)
391-397:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBuild
sequence_namefrom(nspname, relname), not cachedseqoid.Lines 328-330 already document that
seqoidis stale after logical restore until a later refresh runs. Usingsk.seqoid::regclasshere meansspock.sequence_infocan show the wrong object, or just an OID string, during exactly that window.Suggested fix
CREATE VIEW spock.sequence_info AS SELECT - sk.seqoid::regclass AS sequence_name, + to_regclass(format('%I.%I', sk.nspname, sk.relname)) AS sequence_name, sk.kind, CASE WHEN spock.sequence_hook_available() THEN 'active' ELSE 'inactive' END AS hook_status FROM spock.sequence_kind sk;🤖 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 391 - 397, Replace the use of the cached OID field sk.seqoid::regclass in the spock.sequence_info view with a name-based construction from sk.nspname and sk.relname so the view resolves the correct object after logical restore; update the SELECT to build the sequence_name from (nspname, relname) and cast that to regclass (or use format/qualified identifier construction) instead of using seqoid, keeping the rest of the view (kind and hook_status via spock.sequence_hook_available()) unchanged and still selecting from spock.sequence_kind sk.src/spock_seqam_snowflake.c (1)
147-156:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winDecode the stored timestamp with the full Snowflake shift.
Line 147 still unpacks
last_valueas if the timestamp starts immediately above the counter bits. The write path storesnew_ts << SPOCK_SNOWFLAKE_TIMESTAMP_SHIFT, sosf_unpack()reads the wrong bit range and corruptscur_ts, which then feeds the monotonicity and WAL-reservation logic.Suggested fix
-#define SF_STATE_TS_SHIFT SPOCK_SNOWFLAKE_COUNTER_BITS +#define SF_STATE_TS_SHIFT SPOCK_SNOWFLAKE_TIMESTAMP_SHIFT `#define` SF_STATE_CTR_MASK SPOCK_SNOWFLAKE_COUNTER_MASK `#define` SF_STATE_TS_MASK (((UINT64CONST(1) << SPOCK_SNOWFLAKE_TIMESTAMP_BITS) - 1) \ << SF_STATE_TS_SHIFT)🤖 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_seqam_snowflake.c` around lines 147 - 156, sf_unpack is extracting the timestamp using SF_STATE_TS_SHIFT (currently set to SPOCK_SNOWFLAKE_COUNTER_BITS) which doesn't match the write path that stores timestamps shifted by SPOCK_SNOWFLAKE_TIMESTAMP_SHIFT; update the unpacking to use the full Snowflake timestamp shift/mask so the same bit range is decoded. Specifically, ensure SF_STATE_TS_SHIFT (or the shift used inside sf_unpack) equals SPOCK_SNOWFLAKE_TIMESTAMP_SHIFT and that SF_STATE_TS_MASK covers (SPOCK_SNOWFLAKE_TIMESTAMP_BITS << SPOCK_SNOWFLAKE_TIMESTAMP_SHIFT), then extract *ts by shifting the packed value by SPOCK_SNOWFLAKE_TIMESTAMP_SHIFT and keep *ctr extracted with SF_STATE_CTR_MASK so cur_ts and counter are decoded from the same bit layout used by the write path.
🧹 Nitpick comments (3)
include/spock_seqam.h (2)
13-16: 💤 Low value
postgres.hin a public header is non-idiomatic for PG.PostgreSQL convention is that
postgres.his included as the first include of every.cfile, and headers assume it has already been included. Including it from a public header works (header guards make it a no-op when callers do the right thing) but it leaks the assumption and can interact poorly with downstream tooling that expects header order to be controllable. Consider documenting "include afterpostgres.h" instead and dropping the include here.🤖 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 `@include/spock_seqam.h` around lines 13 - 16, Remove the `#include` "postgres.h" from the public header and instead add a short comment at the top of include/spock_seqam.h stating that this header must be included only after postgres.h has been included (so callers include postgres.h first in their .c files); keep the other includes ("catalog/pg_sequence.h", "commands/sequence.h", "utils/relcache.h") as-is and ensure any .c files that currently rely on this header still include postgres.h first (or add postgres.h to those .c files) so symbols used by SeqTable and nextval_hook_type from commands/sequence.h resolve correctly.
100-104: 💤 Low valueInconsistent mask convention between counter and node masks.
SPOCK_SNOWFLAKE_COUNTER_MASKis the raw 12-bit mask (0xFFF), butSPOCK_SNOWFLAKE_NODE_MASKis pre-shifted into its in-value position (0x3FF << 12). Consumers must remember to writevalue & COUNTER_MASKfor one and(value & NODE_MASK) >> NODE_SHIFTfor the other, which is easy to mix up. Consider one consistent convention — either both masks pre-shifted (and add explicit_SHIFThelpers for extraction) or both raw with an explicit shift at the call site.Also,
SPOCK_SNOWFLAKE_MAX_NODE_IDon line 104 uses untyped1while the masks above useUINT64CONST(1); harmless at the current bit width but stylistically inconsistent and would silently overflow ifNODE_BITSever grew past 30.🤖 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 `@include/spock_seqam.h` around lines 100 - 104, SPOCK_SNOWFLAKE_COUNTER_MASK and SPOCK_SNOWFLAKE_NODE_MASK use inconsistent conventions (counter is raw, node is pre-shifted) which is error-prone; change the masks so they follow the same convention (prefer both raw/unshifted): redefine SPOCK_SNOWFLAKE_NODE_MASK as (((UINT64CONST(1) << SPOCK_SNOWFLAKE_NODE_BITS) - 1)) (no shift) and add a new helper SPOCK_SNOWFLAKE_NODE_MASK_SHIFTED = (SPOCK_SNOWFLAKE_NODE_MASK << SPOCK_SNOWFLAKE_NODE_SHIFT) for in-value masking/typing, and also make SPOCK_SNOWFLAKE_MAX_NODE_ID use UINT64CONST(1) ((UINT64CONST(1) << SPOCK_SNOWFLAKE_NODE_BITS) - 1) to keep typing consistent; update any consumers that expect the old pre-shifted NODE_MASK to use the new _SHIFTED helper or perform the explicit shift.patches/15/pg15-050-nextval-hook.diff (1)
95-134: 💤 Low valueMinor:
*lastwritten inside the inner loop may inhibit register promotion.The body of the reservation loop now writes
*last = next;on every iteration (line 100) where before this was a register-friendly local. The compiler can't generally hoist this becauselastis a pointer with potential aliasing. With defaultcache=1this is a non-issue, but for sequences with a largeCACHEsetting it's a measurable difference under hot-path nextval traffic.Consider keeping a local mirror and assigning to
*lastonce after the loop:♻️ Sketch
+ int64 last_local; ... - *last = next = result = seq->last_value; + last_local = next = result = seq->last_value; ... - *last = next; + last_local = next; ... - seq->last_value = *last; /* last fetched number */ + *last = last_local; + seq->last_value = last_local; /* last fetched number */🤖 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/15/pg15-050-nextval-hook.diff` around lines 95 - 134, In nextval_internal, avoid writing through the pointer last on every loop iteration (which prevents register promotion); instead introduce a local scalar (e.g., last_local) to mirror the current "last" value inside the reservation loop (update last_local = next and use last_local for comparisons), and after the loop assign the pointer once with *last = last_local; ensure subsequent uses that expect the pointer-updated value (e.g., seq->last_value = *last) are adjusted to use last_local or performed after the single write so semantics remain unchanged.
🤖 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 `@patches/15/pg15-050-nextval-hook.diff`:
- Around line 25-50: In nextval_internal, initialize the local variable last
defensively before passing &last to nextval_hook or sequence_am_local_nextval to
avoid leaving it uninitialized if a hook returns without setting *last; locate
the nextval_internal function and add a single initialization of last (e.g., set
to the returned value sentinel or the incoming last value semantics) immediately
before the block that calls nextval_hook/sequence_am_local_nextval so that
subsequent use (elm->cached = last) cannot read uninitialized memory.
In `@tests/tap/schedule`:
- Line 48: The schedule change only added a blank line (at the location near
line 48) but did not add the expected TAP test entry for Snowflake SeqAM; either
insert the intended test entry key (for example add a line like test:
0NN_snowflake_seqam) into the schedule file so the Snowflake SeqAM regression is
enabled, or revert/remove the stray blank-line edit and update the PR/stack
description to reflect that no TAP test was added; locate and modify the
schedule file where other test: entries are defined (search for existing "test:"
lines) to place the new entry consistent with formatting.
---
Duplicate comments:
In `@docs/distributed_sequences.md`:
- Around line 17-19: Choose a single replication model for spock.sequence_kind
and update all references to be consistent: either document that
spock.sequence_kind is a user_catalog_table and thus replicated to peers (and
adjust the description of alter_sequence_set_kind() to be cluster-visible, plus
keep the security implication that EXECUTE can change behavior across all
nodes), or document that spock.sequence_kind is node-local (and mark
alter_sequence_set_kind() as non-replicated, remove/adjust any claims about
cluster-wide changes and the EXECUTE security scope). Specifically, reconcile
and edit the paragraphs mentioning spock.sequence_kind (lines ~17–19 and
~222–227), the description and behavior of alter_sequence_set_kind() (lines
~80–95), and the security implication about EXECUTE so they all state the same
replication semantics and trust model.
- Around line 140-146: The fenced code block showing the bit-field layout (the
lines starting with "bit 63 : reserved..." through "bits 11..0 :
12-bit counter") lacks a language identifier; update the opening fence from ```
to ```text so the block becomes a text code fence and satisfies MD040.
In `@sql/spock--6.0.0.sql`:
- Around line 391-397: Replace the use of the cached OID field
sk.seqoid::regclass in the spock.sequence_info view with a name-based
construction from sk.nspname and sk.relname so the view resolves the correct
object after logical restore; update the SELECT to build the sequence_name from
(nspname, relname) and cast that to regclass (or use format/qualified identifier
construction) instead of using seqoid, keeping the rest of the view (kind and
hook_status via spock.sequence_hook_available()) unchanged and still selecting
from spock.sequence_kind sk.
In `@src/spock_executor.c`:
- Around line 253-258: The code elevates privileges using
GetUserIdAndSecContext/SetUserIdAndSecContext before calling
spock_seqam_drop_sequence_record and spock_seqam_relocate_sequence_record but
only restores the original context on the normal path; if the SeqAM helpers
throw, the elevated context remains. Fix both blocks by wrapping the
SetUserIdAndSecContext(BOOTSTRAP_SUPERUSERID...) and the subsequent
spock_seqam_* call in a PG_TRY { ... } PG_FINALLY {
SetUserIdAndSecContext(save_userid, save_sec_context); } PG_END_TRY; so the
original user/security context is always restored (use the same
save_userid/save_sec_context variables and apply the pattern around
spock_seqam_drop_sequence_record(objectId) and
spock_seqam_relocate_sequence_record(objectId)).
In `@src/spock_node.c`:
- Around line 168-181: spock_node_id_for_name currently returns 0 (InvalidOid)
or a value whose low SPOCK_SNOWFLAKE_NODE_BITS are all zero; change it so after
computing raw = (hash_any(...) & 0xffff) you validate and fix the value: if raw
== InvalidOid (0) set raw to a non-zero value (e.g. 1), and if (raw & ((1 <<
SPOCK_SNOWFLAKE_NODE_BITS) - 1)) == 0 set the low SPOCK_SNOWFLAKE_NODE_BITS to a
non-zero pattern (while preserving the other bits) before casting to Oid; keep
all work inside spock_node_id_for_name and use the SPOCK_SNOWFLAKE_NODE_BITS
constant to build the mask.
In `@src/spock_seqam_snowflake.c`:
- Around line 147-156: sf_unpack is extracting the timestamp using
SF_STATE_TS_SHIFT (currently set to SPOCK_SNOWFLAKE_COUNTER_BITS) which doesn't
match the write path that stores timestamps shifted by
SPOCK_SNOWFLAKE_TIMESTAMP_SHIFT; update the unpacking to use the full Snowflake
timestamp shift/mask so the same bit range is decoded. Specifically, ensure
SF_STATE_TS_SHIFT (or the shift used inside sf_unpack) equals
SPOCK_SNOWFLAKE_TIMESTAMP_SHIFT and that SF_STATE_TS_MASK covers
(SPOCK_SNOWFLAKE_TIMESTAMP_BITS << SPOCK_SNOWFLAKE_TIMESTAMP_SHIFT), then
extract *ts by shifting the packed value by SPOCK_SNOWFLAKE_TIMESTAMP_SHIFT and
keep *ctr extracted with SF_STATE_CTR_MASK so cur_ts and counter are decoded
from the same bit layout used by the write path.
In `@src/spock_seqam.c`:
- Around line 564-568: Replace the compile-time Assert checks
(Assert(!RecoveryInProgress()), Assert(!creating_extension),
Assert(!IsBinaryUpgrade), Assert(kind > SPOCK_SEQAM_LOCAL && kind <
SPOCK_SEQAM_NMETHODS)) with runtime guards that detect these unsupported states
and force a local fallback instead of asserting; specifically, in the
spock_seqam dispatch path where these asserts appear, check if
RecoveryInProgress() || creating_extension || IsBinaryUpgrade || !(kind >
SPOCK_SEQAM_LOCAL && kind < SPOCK_SEQAM_NMETHODS) and if so set kind =
SPOCK_SEQAM_LOCAL (or otherwise route to the local sequence handler) so
nextval() will use the local implementation in those states rather than invoking
the managed dispatcher.
- Around line 547-556: When repopulating SeqAmCache you must revalidate any
non-local kind before caching to avoid storing an incompatible managed kind
(e.g. when spock_seqam_default_kind == snowflake); add a validation step after
determining kind (between the seqam_catalog_lookup/seqam_init_cache block and
the hash_search) that calls a sequence-attribute checker (implement a helper
such as spock_seqam_validate_sequence_for_kind(seqoid, kind)) to verify the
sequence has AS bigint, NO MAXVALUE, CACHE 1, positive INCREMENT, etc.; if
validation fails, fall back to a safe kind (e.g. LOCAL or the default-safe kind)
and only then insert into SeqAmCache (entry->kind), so
spock_seqam_snowflake_nextval() never gets an incompatible sequence kind cached.
---
Nitpick comments:
In `@include/spock_seqam.h`:
- Around line 13-16: Remove the `#include` "postgres.h" from the public header and
instead add a short comment at the top of include/spock_seqam.h stating that
this header must be included only after postgres.h has been included (so callers
include postgres.h first in their .c files); keep the other includes
("catalog/pg_sequence.h", "commands/sequence.h", "utils/relcache.h") as-is and
ensure any .c files that currently rely on this header still include postgres.h
first (or add postgres.h to those .c files) so symbols used by SeqTable and
nextval_hook_type from commands/sequence.h resolve correctly.
- Around line 100-104: SPOCK_SNOWFLAKE_COUNTER_MASK and
SPOCK_SNOWFLAKE_NODE_MASK use inconsistent conventions (counter is raw, node is
pre-shifted) which is error-prone; change the masks so they follow the same
convention (prefer both raw/unshifted): redefine SPOCK_SNOWFLAKE_NODE_MASK as
(((UINT64CONST(1) << SPOCK_SNOWFLAKE_NODE_BITS) - 1)) (no shift) and add a new
helper SPOCK_SNOWFLAKE_NODE_MASK_SHIFTED = (SPOCK_SNOWFLAKE_NODE_MASK <<
SPOCK_SNOWFLAKE_NODE_SHIFT) for in-value masking/typing, and also make
SPOCK_SNOWFLAKE_MAX_NODE_ID use UINT64CONST(1) ((UINT64CONST(1) <<
SPOCK_SNOWFLAKE_NODE_BITS) - 1) to keep typing consistent; update any consumers
that expect the old pre-shifted NODE_MASK to use the new _SHIFTED helper or
perform the explicit shift.
In `@patches/15/pg15-050-nextval-hook.diff`:
- Around line 95-134: In nextval_internal, avoid writing through the pointer
last on every loop iteration (which prevents register promotion); instead
introduce a local scalar (e.g., last_local) to mirror the current "last" value
inside the reservation loop (update last_local = next and use last_local for
comparisons), and after the loop assign the pointer once with *last =
last_local; ensure subsequent uses that expect the pointer-updated value (e.g.,
seq->last_value = *last) are adjusted to use last_local or performed after the
single write so semantics remain unchanged.
🪄 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: b48e069a-81aa-42ce-8aa0-d57f30e452fa
⛔ Files ignored due to path filters (1)
tests/regress/expected/init.outis excluded by!**/*.out
📒 Files selected for processing (22)
docs/distributed_sequences.mdinclude/spock_node.hinclude/spock_seqam.hmkdocs.ymlpatches/15/pg15-050-nextval-hook.diffpatches/16/pg16-050-nextval-hook.diffpatches/17/pg17-050-nextval-hook.diffpatches/18/pg18-050-nextval-hook.diffsql/spock--6.0.0.sqlsrc/compat/15/spock_compat.hsrc/compat/16/spock_compat.hsrc/compat/17/spock_compat.hsrc/compat/18/spock_compat.hsrc/spock.csrc/spock_apply.csrc/spock_executor.csrc/spock_node.csrc/spock_seqam.csrc/spock_seqam_snowflake.csrc/spock_sequences.ctests/regress/sql/init.sqltests/tap/schedule
💤 Files with no reviewable changes (4)
- src/compat/16/spock_compat.h
- src/compat/17/spock_compat.h
- src/compat/15/spock_compat.h
- src/compat/18/spock_compat.h
✅ Files skipped from review due to trivial changes (1)
- mkdocs.yml
Comment on lines
+25
to
+50
| int64 incby, | ||
| maxv, | ||
| minv, | ||
| cache, | ||
| - log, | ||
| - fetch, | ||
| last; | ||
| - int64 result, | ||
| - next, | ||
| - rescnt = 0; | ||
| + int64 result; | ||
| bool cycle; | ||
| - bool logit = false; | ||
|
|
||
| /* open and lock sequence */ | ||
| init_sequence(relid, &elm, &seqrel); | ||
| @@ -699,12 +693,51 @@ nextval_internal(Oid relid, bool check_permissions) | ||
| cycle = pgsform->seqcycle; | ||
| ReleaseSysCache(pgstuple); | ||
|
|
||
| + /* Spock: hook overrides; swap for rel->rd_sequenceam->nextval at SeqAM merge. */ | ||
| + if (nextval_hook != NULL) | ||
| + result = (*nextval_hook) (seqrel, incby, maxv, minv, cache, cycle, &last); | ||
| + else | ||
| + result = sequence_am_local_nextval(seqrel, incby, maxv, minv, | ||
| + cache, cycle, &last); |
There was a problem hiding this comment.
Initialize last defensively before dispatching to the hook.
last is declared (line 31) but never initialized before &last is passed to nextval_hook at line 47. The local-case helper writes *last unconditionally on entry, but a third-party hook that returns without writing *last (e.g. an early-return error path that returns a sentinel) will cause elm->cached = last; a few lines below to read uninitialized stack memory and poison the in-core CACHE fast path for the remainder of the session. The header comment documents the contract ("Methods that do not prefetch ... must set *last to the returned value") but the cost of a defensive init is one line.
🛡️ Suggested defensive init
int64 incby,
maxv,
minv,
cache,
- last;
+ last = 0;
int64 result;🤖 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/15/pg15-050-nextval-hook.diff` around lines 25 - 50, In
nextval_internal, initialize the local variable last defensively before passing
&last to nextval_hook or sequence_am_local_nextval to avoid leaving it
uninitialized if a hook returns without setting *last; locate the
nextval_internal function and add a single initialization of last (e.g., set to
the returned value sentinel or the incoming last value semantics) immediately
before the block that calls nextval_hook/sequence_am_local_nextval so that
subsequent use (elm->cached = last) cannot read uninitialized memory.
| test: 022_rmgr_progress_post_checkpoint_crash | ||
| # Upgrade schema match test (builds from source, slow): | ||
| #test: 018_upgrade_schema_match | ||
|
|
There was a problem hiding this comment.
Schedule change does not match the PR/stack description.
The stack context for this cohort states this file should "enable the Snowflake SeqAM regression test in TAP schedule", but the only change here is an added blank line at 48 — no new test: entry was introduced. Either add the intended TAP test entry for the Snowflake SeqAM coverage (e.g. test: 0NN_snowflake_seqam) or drop the stray whitespace change and update the stack/PR summary accordingly.
🤖 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/schedule` at line 48, The schedule change only added a blank line
(at the location near line 48) but did not add the expected TAP test entry for
Snowflake SeqAM; either insert the intended test entry key (for example add a
line like test: 0NN_snowflake_seqam) into the schedule file so the Snowflake
SeqAM regression is enabled, or revert/remove the stray blank-line edit and
update the PR/stack description to reflect that no TAP test was added; locate
and modify the schedule file where other test: entries are defined (search for
existing "test:" lines) to place the new entry consistent with formatting.
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.
Summary
Add a distributed-sequence framework to Spock with a transparent Snowflake-based generator. Application code,
serialdefaults,GENERATED AS IDENTITYcolumns, and trigger-drivennextval()calls all benefit without changes: the in-corenextval_hook(added by the included PG core patch) routes per-sequence to a Snowflake method whose 64-bit values are cluster-wide unique without coordinator round-trips.What changes
PostgreSQL core (
patches/{15,16,17,18}/pgNN-050-nextval-hook.diff) — adds a singlenextval_hookfunction pointer tonextval_internal()and factors asequence_am_local_nextval()helper out of the existing body. Hook signature is identical to Paquier's in-flight SeqAMnextvalcallback (SequenceAmRoutine.nextval); when his patch lands, the call-site swap torel->rd_sequenceam->nextval(...)is mechanical. Per-major patch files cover PG 15/16/17/18, ~172 lines each, each forward- and reverse-applies cleanly against itsREL_NN_STABLEbranch.Catalog (
spock.sequence_kind) — name-keyed(nspname, relname)registry mapping sequences to generation methods (localorsnowflake). Registered withpg_extension_config_dumpso initial subscription sync carries rows to the destination node.Dispatcher (
src/spock_seqam.c) — attached tonextval_hookin_PG_init; resolves the kind for each sequence via a backend-local HTAB cache, dispatches to a per-kind method. Probe-then-populate pattern means the relcache invalidation callback can free entries directly without dangling-pointer hazards from in-flight dispatcher frames.Snowflake method (
src/spock_seqam_snowflake.c) — bit layout[41-bit ms-since-2023-01-01-UTC | 10-bit node_id | 12-bit counter]. Per-sequence state lives in the sequence relation's own heap tuple (last_value, with node_id bits masked off on read); no DSA / dshash / extension-private shared memory. Buffer-lock concurrency model identical to stock sequences. Crash recovery via the standardXLOG_SEQ_LOGmachinery;SEQ_MAGICpage-magic validated on every read; stalexmax(pre-PG-12 SELECT-FOR-UPDATE class) defensively cleared.Time-domain WAL batching —
seq->log_cntrepurposed as a future-dated reservation threshold. We pre-log a 30-ms window of generation and skip WAL on calls within it; crash recovery restores the reservation so post-crash emissions are strictly greater than any pre-crash emission (timestamp may jump forward up to 30 ms; no duplicates are possible). ~30× WAL volume reduction at typical rates relative to "one record per call". Same idea core PG us