Skip to content

Introduce patchsets - #551

Draft
mason-sharp wants to merge 2 commits into
mainfrom
feature/SPOC-636/patchset
Draft

Introduce patchsets#551
mason-sharp wants to merge 2 commits into
mainfrom
feature/SPOC-636/patchset

Conversation

@mason-sharp

@mason-sharp mason-sharp commented Jul 28, 2026

Copy link
Copy Markdown
Member

Introduce patchset version numbers. Over time we can start enforcing this in the extension as appropriate. For now avoid forcing users to update Postgres.

In addition, we lay the groundwork for spock lite mode.

By checking for the existence of symbols, Spock can determine what the situation with the Postgres it is using is:

  1. It contains a patchset version number.

  2. It does not contain a patchset version number, but from checking symbols we can see it is pgedge-patched Postgres

  3. It is unpatched Postgres. Instead of failing at library load time, fail at startup with a more descriptive message. In the future, we can instead output a message and operate in a mode with degraded functionality.

Original idea came from this PR: #421

Introduce a runtime handshake between Spock and the PostgreSQL core patches
it relies on, so Spock can tell which core patch-set "generation" a server
provides.

- New pgNN-000-spock-patchset.diff for PG 15-19 defines an exported global
  `int SpockCorePatchsetVersion` (the patch-set generation; starts at 1,
  0 reserved to mean a patch set predating the marker / baseline).
- Spock detects it at load time via dlsym(RTLD_DEFAULT, ...) and records it
  into `spock_detected_patchset`. dlsym (not a hard PGDLLIMPORT extern or a
  weak symbol) keeps behavior identical on Linux/ELF and macOS/Mach-O and
  lets spock.so load regardless of the symbol's presence.
- _PG_init() logs the detected generation and continues. Detection is
  informational only: no startup enforcement, so existing patched servers
  that predate the marker (reporting generation 0) keep working. The
  SPOCK_CORE_PATCHSET_TARGET macro records the generation this build targets
  for future use.
- spock.core_patchset() SQL function exposes the recorded value, added to
  both the fresh-install (spock--6.0.0.sql) and the 5.0.10->6.0.0 upgrade
  script so upgraded databases get it too.
- TAP test 106_core_patchset.pl covers the positive path.

Enforcement and feature-gating are deferred to a future release.
@coderabbitai

coderabbitai Bot commented Jul 28, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

Changes

The patch adds core patch-set version tracking for PostgreSQL 15–19, resolves patched symbols dynamically during module initialization, routes apply operations through those symbols, exposes spock.core_patchset(), and adds TAP coverage for generation 1.

Core patch-set integration

Layer / File(s) Summary
Patch-set version contract
include/spock.h, patches/{15,16,17,18,19}/...
Defines the target generation, exported runtime state, HLC symbol declarations, and SpockCorePatchsetVersion = 1 in each supported core patch set.
Runtime symbol wiring
src/spock.c, src/spock_apply.c, src/spock_apply_heap.c
Detects the patch-set generation, resolves HLC symbols with dlsym(), fails initialization when required symbols are absent, and uses resolved pointers during commit and delta apply.
SQL exposure and validation
sql/spock--*.sql, src/spock_functions.c, tests/tap/t/106_core_patchset.pl
Registers spock.core_patchset(), returns the detected generation, and verifies that a patched server reports generation 1.

Poem

I’m a rabbit with patches tucked under my ear,
A clock hops forward when core symbols appear.
dlsym() finds friends in the server’s bright light,
SQL tells the generation, neat and right.
The TAP tests nod: “One!”—then I nibble all night.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 40.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed It reflects the main change, introducing patchset support and related runtime detection, though it is broad.
Description check ✅ Passed The description matches the patchset-version and spock-lite groundwork implemented in the changeset.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feature/SPOC-636/patchset

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

@codacy-production

Copy link
Copy Markdown

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 0 duplication

Metric Results
Duplication 0

View in Codacy

NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.

@mason-sharp
mason-sharp marked this pull request as draft July 28, 2026 19:51

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/spock.c`:
- Around line 1055-1069: Require both resolved symbols in src/spock.c lines
1055-1069 by setting spock_hlc_available only when spock_remote_ts_ptr and
spock_subxact_setts are non-NULL, and report the specific missing symbol during
startup failure. In src/spock_apply_heap.c lines 776-784, remove the silent
fallback and invoke the SubTransactionIdSetCommitTsData function under this
initialization invariant, optionally asserting its pointer is non-NULL.
🪄 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 Plus

Run ID: 5ce6f731-c337-45d5-b84d-641437b4949c

📥 Commits

Reviewing files that changed from the base of the PR and between 84ad3e1 and 3d8abfc.

📒 Files selected for processing (13)
  • include/spock.h
  • patches/15/pg15-000-spock-patchset.diff
  • patches/16/pg16-000-spock-patchset.diff
  • patches/17/pg17-000-spock-patchset.diff
  • patches/18/pg18-000-spock-patchset.diff
  • patches/19/pg19-000-spock-patchset.diff
  • sql/spock--5.0.11--6.0.0.sql
  • sql/spock--6.0.0.sql
  • src/spock.c
  • src/spock_apply.c
  • src/spock_apply_heap.c
  • src/spock_functions.c
  • tests/tap/t/106_core_patchset.pl

Comment thread src/spock.c Outdated
Spock hard-referenced remoteTransactionStopTimestamp (HLC) and
SubTransactionIdSetCommitTsData (delta apply), so spock.so failed to
dlopen on unpatched PostgreSQL with a cryptic linker error before any
spock code ran.

Resolve both with dlsym(RTLD_DEFAULT, ...) in _PG_init() and access them
through NULL-guarded pointers. With no unresolved patch symbols, the same
binary loads on patched and unpatched servers alike.

On unpatched PostgreSQL the HLC symbol is absent, so _PG_init() now raises
a clear FATAL instead of the linker error. Degraded operation on unpatched
PostgreSQL is future work. No behavior change on patched PostgreSQL.
@mason-sharp
mason-sharp force-pushed the feature/SPOC-636/patchset branch from dca0a79 to 5c214aa Compare July 28, 2026 23:04

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/spock_apply.c`:
- Around line 985-994: Update the commit flow around CommitTransactionCommand
and append_feedback_position to make spock_remote_ts_ptr cleanup exception-safe:
establish a cleanup block that resets *spock_remote_ts_ptr before any error can
bypass the reset, while keeping feedback processing outside the guarded region
and avoiding normal returns from it. Preserve the existing conditional timestamp
assignment and feedback behavior.
🪄 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 Plus

Run ID: 8e2ab7fc-db9e-4ec7-a3dc-dfb7b4d44c1f

📥 Commits

Reviewing files that changed from the base of the PR and between dca0a79 and 5c214aa.

📒 Files selected for processing (4)
  • include/spock.h
  • src/spock.c
  • src/spock_apply.c
  • src/spock_apply_heap.c
🚧 Files skipped from review as they are similar to previous changes (3)
  • src/spock_apply_heap.c
  • include/spock.h
  • src/spock.c

Comment thread src/spock_apply.c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant