fix(log_object_drops): correct commit LSN, DROP DATABASE path, TEMP skip, and PITR docs (v5 review fixes)#55
fix(log_object_drops): correct commit LSN, DROP DATABASE path, TEMP skip, and PITR docs (v5 review fixes)#55NikolayS wants to merge 1 commit into
Conversation
…and PITR docs
Rebase of v5 ("Add log_object_drops GUC for DROP TABLE/DATABASE logging",
Dmitry Lebedev, rebased by Andrey Borodin) onto current master, plus fixes
for the four defects Kirk Wolak raised, and a fifth found while testing.
Rebase (v5 does not build on master as posted):
* guc_parameters.dat now enforces alphabetical ordering; log_object_drops was
appended after zero_damaged_pages, which fails the build outright.
* t/050_drop_table_logging.pl collided with the existing 050_redo_segment_missing.pl;
renumbered to 055.
1. Format-string bug (real UB, not cosmetic). The errmsg format had four %X
conversions ("drop LSN: %X/%X, commit LSN: %X/%X") but passed a single
LSN_FORMAT_ARGS(), which expands to two arguments. clang reports
-Wformat-insufficient-args; the trailing two %X read past the varargs and
print stack garbage. There is no "drop LSN" recorded anywhere, and it would
not be useful for PITR if there were, so the message now reports only the
commit LSN. Also switched %X/%X to master's %X/%08X convention, which makes
the logged value directly comparable to pg_lsn output.
2. DROP DATABASE used GetXLogInsertRecPtr() (Kirill Reshke's report). That is
read before the commit record is written, and dropdb() still forces an
immediate checkpoint afterwards, so the value could be far behind the actual
commit LSN and was not a valid recovery target. DROP DATABASE now goes
through the same deferred XactCallback path as tables. As a side effect, a
DROP DATABASE that errors out after the catalog delete is no longer logged.
3. Temporary tables are no longer logged. Their contents are not recoverable by
PITR, and they are dropped at every session exit, so the entries were noise.
4. Documented the PITR recipe, including the recovery_target_inclusive trap:
the default of on stops recovery *after* the target, so setting only
recovery_target_lsn replays the drop and loses the object again.
5. (Found while writing the PITR test.) The logged LSN was the *end* of the
commit record, since XactLogCommitRecord() returns XLogInsert()'s EndRecPtr.
recovery_target_lsn is compared against each record's *start*
(recoveryStopsBefore() tests record->ReadRecPtr), so the end LSN is the start
of the following record: recovery with recovery_target_inclusive=off would
still replay the drop. RecordTransactionCommit() now reports ProcLastRecPtr,
the start of the commit record, which is what a DBA can feed back to
recovery_target_lsn. Without this the documented recipe in (4) does not work,
and the TAP test in this commit fails.
Testing (src/test/recovery/t/055_drop_table_logging.pl, 54 assertions):
* the logged LSN is the actual commit LSN, verified against
pg_current_wal_insert_lsn() and by writing ~7MB of WAL between the DROP and
the COMMIT so that the drop LSN and commit LSN cannot be confused;
* nothing is logged when the GUC is off, on ROLLBACK, on ROLLBACK TO SAVEPOINT,
for TEMP tables (explicit, ON COMMIT DROP, and session-exit), or when
DROP DATABASE fails;
* an end-to-end PITR test restores a backup to the logged LSN and asserts the
dropped table and its rows come back with recovery_target_inclusive=off, and
that the default of on loses them.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
|
Tracking issue: #56 |
The LSN correctness bug (was #53)SummaryThe This is a design-level correctness bug, not a documentation gap. The feature as currently shipped does not do the thing it exists to do. Evidence (verified against master, 2026-07-15)What v5 logs — /* src/backend/access/transam/xact.c */
commit_lsn = XactLogCommitRecord(GetCurrentTransactionStopTimestamp(),
nchildren, children, nrels, rels,
...What recovery compares — /* src/backend/access/transam/xlogrecovery.c:2580-2582 */
if (recoveryTarget == RECOVERY_TARGET_LSN &&
!recoveryTargetInclusive &&
record->ReadRecPtr >= recoveryTargetLSN)
Why this breaksLet the DROP's commit record span
Either way the table is gone. There is no setting of Fix options
Recommend doing both: the XID recipe is what a human should copy-paste; the correct start LSN is what tooling should consume. Relationship to the other known issuesKirk Wolak's list from the 2026-07-15 session:
Test implicationA TAP test that asserts "an LSN was logged" cannot catch this. The test must assert the logged LSN actually restores the dropped table — i.e. run a real PITR to the logged target and check the table is present. Anything weaker passes against a broken feature. Refs
🤖 Generated with Claude Code |
Design review: generalizing log_object_drops (was #54)Design review of the Two agents ran, answering two different framings of Andrey's question: "can we spend the creativity budget on generalizing the feature — why is it needed, why is it valuable, how can this be done more in a Postgres way?" All code claims below were checked against Memo 1 — the general question (extension allowed)TL;DR: Everything this patch needs already exists in core: Verified facts
Key pointsKirill Reshke's unanswered question. In March 2025 he asked on the thread: "What stops us from logging all the same inside object access hook defined by extension?" Nobody replied. The answer is: nothing. An extension can hook Restore points vs. log lines — not either/or. A log line lives on the machine you may have just destroyed; a restore point is a WAL record — archived, replicated, readable by Orthogonality. The feature is a composition of three facilities that already exist (drop-event surface, commit-LSN exposure, recovery landmarks). That composition is the Postgres way — and is the argument for it living outside core. A general "record landmark on DDL class" core mechanism is architecture-astronomy. Docs. Primary home is the Continuous Archiving / PITR chapter, cross-linked from the Memo 2 — the same question with the extension option removedPer Andrey on the call: "maybe we can ask it to consider again if we don't want anything like an extension", and Kirk: "we try to get away with it in the core, and only push it into a contrib if we're forced to by hackers." TL;DR: In-core changes the answer — drop the restore-point machinery entirely. Core can hard-wire an emit call into Why in-core changes the shapeThe extension needed a PRE_COMMIT restore point because it couldn't trust the commit LSN (the Kirk's "it's not heavy" argument — steelmanned, then attackedWhen off, the cost is literally one bool test in
What actually gets it committed: remove the callback ABI change, keep the diff under ~300 lines, and make the log line demonstrably correct (see #53 — a committer who tests the current recipe and replays the drop anyway will bounce the patch instantly). The strongest in-core design
The pitch to hackers
Where the two memos disagreeMemo 1 says ship it as an extension/contrib; Memo 2, told to assume core, says core genuinely buys something an extension cannot: the exact commit-record LSN, and ownership of the docs that make the recipe correct. These are consistent — the disagreement is entirely about whether that gain justifies core surface area, which is a judgement call for -hackers, not for an agent. What both agree on:
Refs
🤖 Generated with Claude Code |
Fix tracking — Kirk's 4 items (was #56)Tracking issue for the review items Kirk Wolak raised against v5 of "PoC: Simplify recovery after dropping a table by LOGGING the restore LSN" (
Kirk's four items
Additional findings
Open questions for the list
Not done
|
Session summary 2026-07-15 (was #57)Summary of the Hacking Postgres session of 2026-07-15 (Nikolay Samokhvalov, Kirk Wolak, Andrey Borodin), which worked on the stalled The patch"PoC: Simplify recovery after dropping a table by LOGGING the restore LSN" — logs the commit LSN when a DROP TABLE / DROP DATABASE commits, so a DBA who dropped something by accident knows the PITR target.
Outputs from this session
What was found1. The feature did not work at all (#53)v5 logged the end LSN of the commit record ( Consequence: targeting the logged LSN never stops before the drop, with any value of Fixed in #55: logs This was found independently by two agents, from different directions. 2. v5 does not build on current master — explains 7 months of red CITwo independent breaks, both fixed in #55:
Nobody could have reviewed this patch even if they had wanted to. 3. Kirk's four defects — all fixed in #55
4. Open question raised on the call: unlogged tablesThe patch filters Design discussion (#54)Two memos: one with the extension option open, one with it explicitly off the table (per Andrey and Kirk).
Review feedback still to applyFrom Andrey, on the call:
Not verifiedStated plainly by the agent that produced #55:
Next steps
🤖 Generated with Claude Code |
1 participant
Fixes the four defects Kirk Wolak raised against v5 of "PoC: Simplify recovery after dropping a table by LOGGING the restore LSN", plus a fifth found while writing the PITR test. Andrey Borodin confirmed on the Hacking Postgres call that Kirk's list is "a real list of problems in the patch".
Targets the pgsql-hackers thread and CF #6272.
Base: v5 attachment
v5-0001-Add-log_object_drops-GUC-for-DROP-TABLE-DATABASE-.patch(authored by Dmitry Lebedev, rebased and posted by Andrey Borodin 2025-12-10 — note the thread's v5 is Andrey's post, not Dmitry's), applied onorigin/master@11ed011ae22.Rebase notes — v5 does not build on current master
The CF entry's CI has been red for months. Two independent reasons, both fixed here:
guc_parameters.datnow enforces alphabetical ordering. v5 appendslog_object_dropsafterzero_damaged_pages, which fails the build outright:log_min_messagesandlog_parameter_max_length.t/050_drop_table_logging.plcollides with master's existing050_redo_segment_missing.pl(master is up to054_*). Renumbered to055, which is the onlymeson.buildconflict.The four fixes
1. Format-string bug — confirmed as real UB, not cosmetic
The format had four
%Xbut passed oneLSN_FORMAT_ARGS(), which expands to two arguments:Verified with a standalone repro using the same
__attribute__((format(printf,...)))thaterrmsgcarries:The trailing two
%Xread past the varargs and print stack garbage. No "drop LSN" is recorded anywhere in the patch, and it would not be useful for PITR if it were, so the message now reports only the commit LSN. Also switched%X/%Xto master's%X/%08Xconvention (708 uses vs 26 legacy), which makes the logged value directly comparable topg_lsnoutput.2. DROP DATABASE still used
GetXLogInsertRecPtr()Kirill Reshke's report, unfixed on that path. The value is read before the commit record is written, and
dropdb()then forces an immediate checkpoint (RequestCheckpoint(CHECKPOINT_FAST | CHECKPOINT_FORCE | CHECKPOINT_WAIT)) before committing — so the logged LSN could be far behind the real commit LSN and was not a valid recovery target.DROP DATABASE now uses the same deferred
XactCallbackpath as tables. The per-transaction machinery independency.cis generalized from tables-only toDropObjectInfo { kind, oid, name, schema }, withRegisterDropDatabaseForLogging()exported fordbcommands.c(which does not go throughperformDeletion()).Side effect worth noting: a DROP DATABASE that errors out after the catalog delete is no longer logged. The old code logged unconditionally, before the drop was durable.
3. Skip TEMP
Temp tables are excluded via
get_rel_persistence() != RELPERSISTENCE_TEMP. They are not PITR-recoverable and are dropped at every session exit, so the entries were pure noise. Test 9 of v5 asserted the opposite (expected temp drops to be logged); it is inverted and expanded to also coverON COMMIT DROPand implicit session-exit cleanup.4. Document the PITR use, including the
recovery_target_inclusivetrapThe docs had no mention of PITR at all. Added a paragraph making the correct incantation unmissable —
recovery_target_inclusivedefaults toon, which stops recovery after the target, so a DBA setting onlyrecovery_target_lsnreplays the drop and loses the table again:5. Bonus defect found while writing the PITR test
The documented recipe in (4) did not actually work, because v5 logs the wrong end of the commit record.
XactLogCommitRecord()returnsXLogInsert()'sEndRecPtr— the end of the commit record. Butrecovery_target_lsnis compared against each record's start:The end LSN of the commit record is the start of the following record, so recovery with
recovery_target_inclusive = offstops before the record after the commit — i.e. it still replays the drop. The exact failure the feature exists to prevent, silently, even for a DBA who follows the docs correctly.RecordTransactionCommit()now reportsProcLastRecPtr, the start of the commit record just inserted — the same idiom xlog.c uses for checkpoints ("We now have ProcLastRecPtr = start of actual checkpoint record"). Confirmed withpg_waldump, the logged LSN lands exactly on the drop transaction's commit record:Without this fix the PITR test below fails outright. This is the item I would most like reviewed on-list — it changes what
commit_lsnmeans in theXactCallbackAPI v5 introduces.Testing
Andrey explicitly asked for better testing. v5's test largely asserted "a line exists", which cannot distinguish a correct LSN from garbage.
t/055_drop_table_logging.plis rewritten: 54 assertions, all passing.The key one writes ~7 MB of WAL between the DROP and the COMMIT, so the drop LSN and the commit LSN cannot be confused — a
GetXLogInsertRecPtr()-at-drop-time implementation fails it by a wide margin:logged == before_commitexactly, and sits ~7.6 MB past the drop LSN. DROP DATABASE is pinned the same way (# db logged=0/0279DC78 before=0/0279DA68 after=0/0279DCE0).End-to-end PITR — restores a real basebackup to the logged LSN and proves both directions of the trap:
Also covered: nothing logged when the GUC is off (with an on/off control pair), on ROLLBACK, on ROLLBACK TO SAVEPOINT, for TEMP (explicit /
ON COMMIT DROP/ session exit), for a TEMP sibling without suppressing a permanent table dropped in the same transaction, for views/indexes, and for a failed DROP DATABASE; plusCOMMIT AND CHAIN,ROLLBACK AND CHAIN, savepoints, partitions, inheritance,DROP SCHEMA CASCADE, and PL/pgSQL EXCEPTION subtransactions (both committed and rolled back).Regression runs on this branch (macOS/arm64, meson,
--with-cassert):What I could NOT verify
Stated plainly:
ninja alldocsfails fetching the DocBook 4.5 DTD over the network (Attempt to load network entity http://www.oasis-open.org/docbook/xml/4.5/docbookx.dtd) — a local toolchain gap, unrelated to this change. I verified the new<varlistentry>parses as well-formed XML and that both<xref linkend=...>targets (guc-recovery-target-inclusive,guc-recovery-target-lsn) exist inconfig.sgml. The rendered output is unverified.pg_bsd_indentis not installed here. Indentation was matched by hand andgit diff --checkis clean, but this has not been through the real formatter and may drift../configurebuild, no Windows, no CI.sepgsqlnot compiled (selinux: NOin this build), so its one-lineXactCallbacksignature change is untested beyond inspection. Same forpostgres_fdw's callback change: the file compiles, but I did not run thepostgres_fdwsuite.autovacuum = off,checkpoint_timeout = 1h,max_wal_size = 1GBto that end, and the start-of-commit-record bound is asserted as>=rather than==precisely because a commit record's start can be nudged past a WAL page header. Still, these are the assertions most likely to prove flaky on a loaded buildfarm animal, and that risk is real.Open questions for the list
XactCallbackfrom end-of-commit-record to start. If that callback is intended as general infrastructure rather than something private to this feature, end-of-record may be the more conventional choice, and this feature should derive the start itself — but then the GUC must not hand a DBA an LSN that silently replays the drop.Assert(!XLogRecPtrIsInvalid(commit_lsn))inherited from v5 is kept. It holds because any transaction dropping a table or database necessarily has an XID and writes a commit record, but it is an assert in a callback that fires on every commit.🤖 Generated with Claude Code