Repository navigation
feat(recovery): observe effective PITR target configuration #299
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Draft
seonghobae
wants to merge
8
commits into
feat/postgres-pitr-target-b84f0c9
Choose a base branch
from
feat/postgres-recovery-target-config-b84f0c9
base: feat/postgres-pitr-target-b84f0c9
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
049cd0f
test(recovery): define effective PITR target observation contract
seonghobae caf7325
feat(recovery): observe effective PITR target configuration
seonghobae 94d78bb
test(recovery): harden target observation review boundaries
seonghobae 148f654
chore(pr299): restack PITR target observation on current parent
seonghobae 014e1f5
test(recovery): bound target configuration result materialization
seonghobae c36aaf4
fix(recovery): bound target configuration result materialization
seonghobae 2c4bbe9
test(recovery): adapt target configuration fixtures to bounded fetch
seonghobae fa99353
test(recovery): keep target configuration review fixtures bounded
seonghobae File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,240 @@ | ||
| # SPDX-License-Identifier: Apache-2.0 | ||
| """Observe effective PostgreSQL PITR target settings without mutating recovery.""" | ||
|
|
||
| from __future__ import annotations | ||
|
|
||
| from dataclasses import dataclass, field | ||
| from weakref import WeakKeyDictionary | ||
|
|
||
| from .postgres_pitr_target import ( | ||
| PostgresPitrRecoveryTarget, | ||
| PostgresPitrTargetError, | ||
| ) | ||
|
|
||
|
|
||
| _SETTING_NAMES = ( | ||
| "recovery_target", | ||
| "recovery_target_action", | ||
| "recovery_target_inclusive", | ||
| "recovery_target_lsn", | ||
| "recovery_target_name", | ||
| "recovery_target_time", | ||
| "recovery_target_timeline", | ||
| "recovery_target_xid", | ||
| ) | ||
| _MAX_SETTING_BYTES = 1024 | ||
| _TARGET_CONFIGURATION_SQL = """ | ||
| SELECT | ||
| s.name::pg_catalog.text, | ||
| s.setting::pg_catalog.text, | ||
| s.pending_restart, | ||
| pg_catalog.pg_is_in_recovery() | ||
| FROM pg_catalog.pg_settings AS s | ||
| WHERE s.name IN ( | ||
| 'recovery_target', | ||
| 'recovery_target_action', | ||
| 'recovery_target_inclusive', | ||
| 'recovery_target_lsn', | ||
| 'recovery_target_name', | ||
| 'recovery_target_time', | ||
| 'recovery_target_timeline', | ||
| 'recovery_target_xid' | ||
| ) | ||
| ORDER BY s.name | ||
| """.strip() | ||
| _OBSERVATION_MARK = object() | ||
|
|
||
|
|
||
| class PostgresRecoveryTargetConfigurationObservationError(ValueError): | ||
| """Report a fail-closed PostgreSQL recovery-target observation violation.""" | ||
|
|
||
|
|
||
| @dataclass(frozen=True, eq=False) | ||
| class PostgresRecoveryTargetConfigurationObservation: | ||
| """Represent one content-free live match of effective PITR target settings. | ||
|
|
||
| Only :func:`observe_postgres_recovery_target_configuration` registers an | ||
| object as package-observed evidence. Public construction, copying, | ||
| subclassing, or post-construction field mutation therefore cannot be reused | ||
| as observation provenance. The record intentionally contains no recovery | ||
| target value, restore-point name, timeline identifier, connection detail, | ||
| filesystem path, or credential. | ||
| """ | ||
|
|
||
| recovery_in_progress: bool = True | ||
| settings_match: bool = True | ||
| pending_restart: bool = False | ||
| _observation_mark: object = field(default=None, repr=False, compare=False) | ||
|
|
||
| def as_dict(self) -> dict[str, bool]: | ||
| """Return stable content-free predicates from a live observed record.""" | ||
| recovery_in_progress, settings_match, pending_restart = _require_observed(self) | ||
| return { | ||
| "recovery_in_progress": recovery_in_progress, | ||
| "settings_match": settings_match, | ||
| "pending_restart": pending_restart, | ||
| } | ||
|
|
||
|
|
||
| _OBSERVED_CONFIGURATION: WeakKeyDictionary[ | ||
| PostgresRecoveryTargetConfigurationObservation, tuple[bool, bool, bool] | ||
| ] = WeakKeyDictionary() | ||
|
|
||
|
|
||
| def postgres_recovery_target_configuration_was_observed(evidence: object) -> bool: | ||
| """Return whether one exact live object still matches its observed snapshot.""" | ||
| if type(evidence) is not PostgresRecoveryTargetConfigurationObservation: | ||
| return False | ||
| observed = _OBSERVED_CONFIGURATION.get(evidence) | ||
| return observed is not None and ( | ||
| evidence._observation_mark, | ||
| evidence.recovery_in_progress, | ||
| evidence.settings_match, | ||
| evidence.pending_restart, | ||
| ) == (_OBSERVATION_MARK, *observed) | ||
|
|
||
|
|
||
| def _require_observed( | ||
| evidence: PostgresRecoveryTargetConfigurationObservation, | ||
| ) -> tuple[bool, bool, bool]: | ||
| """Return the immutable observation snapshot or fail closed.""" | ||
| if not postgres_recovery_target_configuration_was_observed(evidence): | ||
| raise PostgresRecoveryTargetConfigurationObservationError( | ||
| "PostgreSQL recovery target configuration observation provenance is invalid" | ||
| ) | ||
| return _OBSERVED_CONFIGURATION[evidence] | ||
|
|
||
|
|
||
| def _record_observation( | ||
| evidence: PostgresRecoveryTargetConfigurationObservation, | ||
| ) -> None: | ||
| """Remember the exact live observation object and its immutable predicates.""" | ||
| _OBSERVED_CONFIGURATION[evidence] = ( | ||
| evidence.recovery_in_progress, | ||
| evidence.settings_match, | ||
| evidence.pending_restart, | ||
| ) | ||
|
|
||
|
|
||
| def _snapshot_target(target: object) -> PostgresPitrRecoveryTarget: | ||
| """Copy one exact reviewed target into a fresh canonical authority snapshot.""" | ||
| if type(target) is not PostgresPitrRecoveryTarget: | ||
| raise PostgresRecoveryTargetConfigurationObservationError( | ||
| "invalid PostgreSQL recovery target configuration observation inputs" | ||
| ) | ||
| try: | ||
| return PostgresPitrRecoveryTarget( | ||
| target_kind=target.target_kind, | ||
| target_value=target.target_value, | ||
| inclusive=target.inclusive, | ||
| timeline=target.timeline, | ||
| recovery_target_action=target.recovery_target_action, | ||
| ) | ||
| except (AttributeError, PostgresPitrTargetError): | ||
| raise PostgresRecoveryTargetConfigurationObservationError( | ||
| "invalid PostgreSQL recovery target configuration observation inputs" | ||
| ) from None | ||
|
|
||
|
|
||
| def _expected_settings(target: PostgresPitrRecoveryTarget) -> dict[str, str]: | ||
| """Return the complete effective setting map expected for one target.""" | ||
| expected = {name: "" for name in _SETTING_NAMES} | ||
| for name, value in target.server_settings(): | ||
| expected[name] = value | ||
| if target.inclusive is None: | ||
| expected["recovery_target_inclusive"] = "on" | ||
| return expected | ||
|
|
||
|
|
||
| def _validate_rows(rows: object, expected: dict[str, str]) -> None: | ||
| """Fail closed unless one finite row set exactly matches target authority.""" | ||
| if type(rows) is not list or len(rows) != len(_SETTING_NAMES): | ||
| raise PostgresRecoveryTargetConfigurationObservationError( | ||
| "PostgreSQL recovery target configuration evidence is invalid" | ||
| ) | ||
| seen: set[str] = set() | ||
| for row in rows: | ||
| if type(row) is not tuple or len(row) != 4: | ||
| raise PostgresRecoveryTargetConfigurationObservationError( | ||
| "PostgreSQL recovery target configuration evidence is invalid" | ||
| ) | ||
| name, setting, pending_restart, recovery_in_progress = row | ||
| if not ( | ||
| type(name) is str | ||
| and type(setting) is str | ||
| and type(pending_restart) is bool | ||
| and type(recovery_in_progress) is bool | ||
| ): | ||
| raise PostgresRecoveryTargetConfigurationObservationError( | ||
| "PostgreSQL recovery target configuration evidence is invalid" | ||
| ) | ||
| if name not in expected or name in seen: | ||
| raise PostgresRecoveryTargetConfigurationObservationError( | ||
| "PostgreSQL recovery target configuration evidence is invalid" | ||
| ) | ||
| try: | ||
| setting_size = len(setting.encode("utf-8")) | ||
| except UnicodeError: | ||
| raise PostgresRecoveryTargetConfigurationObservationError( | ||
| "PostgreSQL recovery target configuration evidence is invalid" | ||
| ) from None | ||
| if setting_size > _MAX_SETTING_BYTES: | ||
| raise PostgresRecoveryTargetConfigurationObservationError( | ||
| "PostgreSQL recovery target configuration evidence is invalid" | ||
| ) | ||
| if pending_restart: | ||
| raise PostgresRecoveryTargetConfigurationObservationError( | ||
| "PostgreSQL recovery target configuration has pending restart state" | ||
| ) | ||
| if not recovery_in_progress: | ||
| raise PostgresRecoveryTargetConfigurationObservationError( | ||
| "PostgreSQL recovery target is not in recovery" | ||
| ) | ||
| if setting != expected[name]: | ||
| raise PostgresRecoveryTargetConfigurationObservationError( | ||
| "PostgreSQL recovery target configuration does not match target authority" | ||
| ) | ||
| seen.add(name) | ||
|
|
||
|
|
||
| def observe_postgres_recovery_target_configuration( | ||
| connection: object, | ||
| *, | ||
| target: object, | ||
| ) -> PostgresRecoveryTargetConfigurationObservation: | ||
| """Observe that effective recovery settings match one reviewed PITR target. | ||
|
|
||
| ``connection`` is caller-owned and already connected to an isolated | ||
| PostgreSQL recovery target. ``target`` must be the exact reviewed | ||
| :class:`PostgresPitrRecoveryTarget` type. The function snapshots that target, | ||
| executes one fixed catalog-qualified read-only query, requires the target to | ||
| remain in recovery with no pending-restart setting state, and compares all | ||
| eight recovery-target settings to the deterministic target authority. | ||
|
|
||
| PostgreSQL's default ``recovery_target_inclusive=on`` is checked explicitly | ||
| when a named or immediate target correctly omits that server setting. The | ||
| returned observation contains only fixed predicates and has live-object | ||
| provenance; target values and connection details are never returned. | ||
|
|
||
| This seam does not write PostgreSQL configuration, create ``recovery.signal``, | ||
| supply ``restore_command``, replay or validate WAL bytes, prove archive or | ||
| timeline ancestry, prove that the target was reached, pause or promote | ||
| recovery, prove application readiness, or establish RPO/RTO, HA/DR, CSAP, | ||
| SOC 2, or certification claims. | ||
| """ | ||
| reviewed_target = _snapshot_target(target) | ||
| expected = _expected_settings(reviewed_target) | ||
| try: | ||
| with connection.cursor() as cursor: | ||
| cursor.execute(_TARGET_CONFIGURATION_SQL) | ||
| rows = cursor.fetchall() | ||
| except Exception: | ||
| raise PostgresRecoveryTargetConfigurationObservationError( | ||
| "PostgreSQL recovery target configuration could not be inspected" | ||
| ) from None | ||
| _validate_rows(rows, expected) | ||
| evidence = PostgresRecoveryTargetConfigurationObservation( | ||
| _observation_mark=_OBSERVATION_MARK | ||
| ) | ||
| _record_observation(evidence) | ||
| return evidence | ||
Oops, something went wrong.
Oops, something went wrong.
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.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
Repository: ContextualWisdomLab/pg-llm-batch
Length of output: 276
🏁 Script executed:
Repository: ContextualWisdomLab/pg-llm-batch
Length of output: 2012
🏁 Script executed:
Repository: ContextualWisdomLab/pg-llm-batch
Length of output: 50388
🏁 Script executed:
Repository: ContextualWisdomLab/pg-llm-batch
Length of output: 9156
🏁 Script executed:
Repository: ContextualWisdomLab/pg-llm-batch
Length of output: 361
복구 관찰 계약과 공개 범위를 문서화하십시오.
README.md,CHANGELOG.md, 관련 ADR 및 doctoring 문서에 두 API의 고정 SQL 조회와 제한된 보장 범위를 추가하십시오. 두 API는 현재 루트 패키지의 공개 표면에 없으므로, 공개 API로 제공할 경우 import와__all__에 등록하고 그렇지 않으면 모듈 전용 범위를 문서화하십시오.🤖 Prompt for AI Agents
Source: Coding guidelines
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Finding remains valid. The canonical documentation writer #229 has now advanced to exact
045e42b3db7fa47e3e29d3fdbe7531d9e96d6d62and records #299 as an ACTIVE-PR bounded recovery-target configuration observer: exactly eight reviewed recovery-target settings pluspg_is_in_recovery(), caller-owned connection/timeout authority, module-scoped package surface, and explicit non-guarantees for WAL completeness, target attainment, promotion, application readiness, PITR success, and achieved RPO/RTO.tests/test_canonical_status_vocabulary.pynow pins that documentation/public-surface boundary and verifies the two functions are not silently promoted through rootpg_llm_batch.__init__.I am intentionally not resolving this thread yet. Root
README.md,CHANGELOG.md, the relevant ADR, and operator/doctoring material are owned by other active documentation lanes; copying them into #229 would violate the current writer split. Those owner paths still need to converge before this finding is fully satisfied. #229's prior GREEN runs are historical after the new head; fresh CI/Release Acceptance were queued at the latest read.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@seonghobae, acknowledged.The module-scoped API boundary and the canonical-status coverage in
#229support the intended contract. This finding remains open until the root documentation, ADR, and operator/doctoring updates converge and fresh CI/Release Acceptance complete for the latest head.You are interacting with an AI system.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fresh exact-head validation for the canonical #229 portion is no longer queued. #229 remains exact
bf6dd65d716a7324cf3ac1291dcedfae556fe666on exact #23301d231fde23b82e2ced258d7bfcb4721ed75706d; CI34553499878and Release Acceptance34553499932are both completed/success. Its effective surface is still the bounded 14-file canonical-doc/test delta, and fresh formal-review inventory still has no qualifying current-headAPPROVEDreview.The substantive cross-owner finding remains unresolved for the same reason as before: root
README.md,CHANGELOG.md, the relevant ADR, and operator/doctoring material have not yet converged under their active owner lanes. I am therefore leaving this thread open; repository-local GREEN on #229 removes only the stale queued-evidence subcondition and does not satisfy the remaining owner-path documentation contract.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@seonghobae, acknowledged.The successful CI and Release Acceptance runs remove the stale queued-validation condition for the canonical
#229portion.The finding remains open. The required root
README.md,CHANGELOG.md, ADR, and operator/doctoring documentation updates have not yet converged. The documentation contract is therefore not complete.You are interacting with an AI system.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@seonghobae, acknowledged.The owner lanes now have exact-head repository acceptance for the required documentation updates.
The finding remains open. The documentation changes are not yet integrated into the protected documentation state. After normal integration, re-read the protected
README.md,CHANGELOG.md, ADR, and doctoring paths against the bounded observer contract. Then evaluate the thread for closure.You are interacting with an AI system.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Current-head correction: #348 advanced normally after the predecessor GREEN generation. It is now
f0176d81f353e8a919a6079bccc2559f25ceb60c, 4 ahead / 0 behind #324, with the same three-file ownership surface. The three descendant commits add an executable/documented PostgreSQLrecovery_target_inclusive=ondefault handoff when the reviewed target has no explicit inclusion edge.Accordingly, #348's predecessor CI
35101232864/ Release Acceptance35101232935remain provenance only. Fresh current-head CI35146841280and Release Acceptance35146841327are queued/nonterminal, and formal review inventory is still empty. The thread therefore remains unresolved for the stronger reason: the moved ADR/operator owner head must reacquire its own GREEN and, after that, normal protected integration before this finding can close.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@seonghobae, acknowledged.The successful runs for
a00ecb5f554f03c62f5b7942f95dacd1c5e950c4do not validate the current#348head,f0176d81f353e8a919a6079bccc2559f25ceb60c.This finding remains open. Wait for CI
35146841280and Release Acceptance35146841327to complete successfully for the current head. Then require normal protected integration and re-read the integrated documentation paths before closure.You are interacting with an AI system.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fresh code-current correction after re-reading #299's implementation and review-regression tests: canonical ADR/operator owner #348 has advanced again to exact
29cd87759d35546fe0df389ca417851120a9756c, still on #32466cb316651aa172f595167ee4a8338141551756c, now 7 ahead / 0 behind with the same three-file ownership surface.The new delta preserves an observer trust boundary that the previous operator text did not state explicitly. #299's
_snapshot_target()requires exactPostgresPitrRecoveryTargetauthority and reconstructs it through the canonical target contract before cursor acquisition;test_invalid_target_authority_is_rejected_before_database_ioalready proves a wrong-type target and a mutated target that no longer satisfies that contract fail before the fixed query. #348 now makes that semantics executable in its documentation contract (eebe342a139ee4a7f7a7b0705f0ab37889f9bd1b), records it in ADR 0019 (47c8c66bb543bdf1c169d6700ecde443effd9385), and records the operator rule on current head29cd8775....The moved head has fresh CI
35165157088and Release Acceptance35165157134, both queued/nonterminal at this read; formal review and inline-thread inventories on #348 are empty. Predecessor GREEN therefore remains historical only. I am leaving this thread unresolved until this exact owner generation is accepted, the owner lanes integrate normally, and the protected documentation is re-read against the full bounded contract.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
Length of output: 1370
@seonghobae, verified.PR
#348is open and Draft at29cd87759d35546fe0df389ca417851120a9756c. Its base is66cb316651aa172f595167ee4a8338141551756c.CI
35165157088is queued for that exact head. Release Acceptance35165157134is also queued for that exact head.The prior successful runs do not validate the current
#348head. Keep this finding open until both current-head runs succeed, the owner lanes integrate normally, and the integrated documentation is checked against the full bounded observer contract.You are interacting with an AI system.