Skip to content

feat(recovery): bind caller-owned physical/WAL/PITR profiles - #217

Closed
cursor[bot] wants to merge 1 commit into
mainfrom
cursor/bc-97f93c73-7f2b-44fa-90e5-317c6fdf7b38-30ba
Closed

feat(recovery): bind caller-owned physical/WAL/PITR profiles#217
cursor[bot] wants to merge 1 commit into
mainfrom
cursor/bc-97f93c73-7f2b-44fa-90e5-317c6fdf7b38-30ba

Conversation

@cursor

@cursor cursor Bot commented Aug 16, 2026

Copy link
Copy Markdown
Contributor

Why this exists

Issue #204 still needs a separately reviewed physical/WAL/PITR contract. Protected main can record logical-recovery receipts and artifact/schema hashes, but a buyer cannot yet state in machine-readable form whether recovery is crash-consistent only or can follow time.

This slice binds that profile. It does not execute pg_basebackup, archive WAL, or replay a cluster.

Operator action

Evidence

  • Local coverage run --branch --source=pg_llm_batch.postgres_physical_recovery: 41 passed, 68 stmts / 22 branches / 0 miss / 0 partial = 100%.
  • interrogate 100%. ruff clean on the new files.
  • Docs: docs/doctoring/postgres-physical-pitr-profile.md, docs/adr/0019-postgres-physical-pitr-profile.md.

Writer boundary

New files only. Does not race #208 backup execution, #212 restore seek, #214 docs overlay, #215 receipt binding, or #216 catalog inspection. CHANGELOG/README/ARCHITECTURE stay with those writers.

This slice does not prove live WAL replay, schema/RLS parity, CSAP, or SOC 2 readiness. Refs #204.

Open in Web View Automation 

Record a content-free physical or PITR recovery profile without executing
pg_basebackup or WAL replay. Point-in-time targets require a PITR method
and a WAL archive. Isolated-target readiness is mandatory. Optional RPO/RTO
values remain deployer objectives and cannot be persisted as a package
capability claim.

Refs #204.

Co-authored-by: Seongho Bae <seonghobae@users.noreply.github.com>
@cursor
cursor Bot requested a review from seonghobae August 16, 2026 15:42
@seonghobae
seonghobae marked this pull request as ready for review August 16, 2026 15:44
@cursor

cursor Bot commented Aug 16, 2026

Copy link
Copy Markdown
Contributor Author

Bugbot is not enabled for your account, so this pull request was not reviewed.

Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs.

@cursor cursor Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Stale comment

Review of #217 at 2aaa6d84

This slice correctly stays evidence-only: it does not execute pg_basebackup, archive WAL, or restore a cluster. Point-in-time kinds require pitr plus a WAL archive, isolated-target is mandatory, and package_capability_claim cannot persist as true.

Two Important defects remain on this exact head. Do not merge 2aaa6d84.

CodeRabbit CLI 0.7.3 is installed here, but coderabbit auth login --agent timed out. This is not a CodeRabbit result.

Important

  1. parse_postgres_physical_recovery_profile leaks UnicodeEncodeError on lone surrogates. The sibling receipt parser on main wraps the same encode("utf-8") in except UnicodeError and re-raises the typed error. Hosts that catch only PostgresPhysicalRecoveryError miss this path.
  2. The physical+immediate test says a base backup "may omit WAL". wal_archive_required=False means no continuous WAL archive. A crash-consistent physical restore still needs backup-internal WAL (pg_basebackup -X stream or -X fetch). PostgreSQL 18 continuous-archiving docs require keeping the WAL generated during the backup.

Minor

Doctoring steps 2–3 did not separate backup-internal WAL from a continuous archive, and pitr+immediate (end-of-backup, not end-of-archive) was untested.

Operator next action

Prefer successor #219 at b03d913 (cursor/bc-2b2c5a03-fe6c-461c-be59-74976ca5f6fc-c4c5). That head wraps the encode path, adds the surrogate test, and rewrites the WAL wording. Close or leave #217 unmerged once #219 is the review target.

Do not self-approve. Do not start a live #204 pg_basebackup + WAL drill on this head. Keep #208 and #212 as the logical dump/restore executors. Merge only after required checks are terminal-success on the unchanged successor head and a non-author human approval lands.

Open in Web View Automation 

Sent by Cursor Automation: Fix Issues

Comment thread pg_llm_batch/postgres_physical_recovery.py
Comment thread tests/test_postgres_physical_recovery.py
Comment thread docs/doctoring/postgres-physical-pitr-profile.md

@cursor cursor Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Review (2aaa6d8)

This head is superseded. Do not merge 2aaa6d84ae00d9e1f8fbf67b5d935a9cadc69032.

Two must-fix defects remain on this tree:

  1. parse_postgres_physical_recovery_profile() encodes the document without catching UnicodeError. Lone-surrogate text leaks UnicodeEncodeError. Hosts catching only PostgresPhysicalRecoveryError miss that path.
  2. test_physical_immediate_profile_allows_crash_consistent_base_backup and doctoring steps 2–3 treat wal_archive_required=False as "may omit WAL". That confuses continuous WAL archive with backup-internal WAL from pg_basebackup -X stream|fetch. Crash-consistent restore still needs the backup's own WAL. pitr+immediate is also untested here.

Landing vehicle is #220 at 1c2ce7d8d8fd68e01940978786f267e217e888db on protected main d2f1e32. It keeps the profile seam, wraps UTF-8 failures as PostgresPhysicalRecoveryError, and records the continuous-archive vs backup-internal WAL distinction.

Do not fold this physical binder onto #208 or #212. This automation will not self-approve #217 or #220.

View PR

Open in Web View Automation 

Sent by Cursor Automation: Fix Issues

@cursor cursor Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Review (2aaa6d8)

This head is superseded. Do not merge 2aaa6d84ae00d9e1f8fbf67b5d935a9cadc69032.

Two must-fix defects remain on this tree:

  1. parse_postgres_physical_recovery_profile() encodes the document without catching UnicodeError. Lone-surrogate text leaks UnicodeEncodeError.
  2. Physical+immediate wording treats wal_archive_required=False as "may omit WAL". That confuses continuous WAL archive with backup-internal WAL from pg_basebackup -X stream|fetch.

Landing vehicle is #219 at b03d91389afb4b32cd5350f890d9ddc682a7edeb. Do not merge sibling successor #220; it repairs the same two defects and is not a second landing reason.

Do not fold this physical binder onto #208 or #212. This automation will not self-approve #217, #219, or #220.

View PR

Open in Web View Automation 

Sent by Cursor Automation: Fix Issues

@seonghobae seonghobae closed this Aug 16, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants