Summary of the Hacking Postgres session of 2026-07-15 (Nikolay Samokhvalov, Kirk Wolak, Andrey Borodin), which worked on the stalled log_object_drops patch.
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
|
|
| PR #55 — the fix |
#55 |
| #56 — fix tracking issue |
#56 |
| #53 — the LSN correctness bug |
#53 |
| #54 — design review (two memos) |
#54 |
| Branch |
fix/drop-lsn-v5-fixes, commit 15aa5609c65, off origin/master @ 11ed011ae22 |
What was found
1. The feature did not work at all (#53)
v5 logged the end LSN of the commit record (XactLogCommitRecord() returns XLogInsert's EndPos). Recovery compares recovery_target_lsn against each record's start (recoveryStopsBefore() tests record->ReadRecPtr, xlogrecovery.c:2580-2582).
Consequence: targeting the logged LSN never stops before the drop, with any value of recovery_target_inclusive — recovery replays the commit and stops before the next record. The object is lost again. As Andrey put it on the call: the logged LSN is one record past the actual commit of the drop. And the DBA cannot correct it by hand, because the record length is not recoverable from the log line.
Fixed in #55: logs ProcLastRecPtr (xlog.h:33), which XLogInsertRecord() sets to the start of the record just inserted (xlog.c:1112). Confirmed with pg_waldump that the logged LSN lands exactly on the drop transaction's COMMIT record. Same idiom CreateCheckPoint already relies on (xlog.c:7772).
This was found independently by two agents, from different directions.
2. v5 does not build on current master — explains 7 months of red CI
Two independent breaks, both fixed in #55:
guc_parameters.dat now enforces alphabetical ordering; log_object_drops was appended after zero_damaged_pages → hard build failure.t/050_drop_table_logging.pl collides with master's existing 050_redo_segment_missing.pl → renumbered to 055.
Nobody could have reviewed this patch even if they had wanted to.
3. Kirk's four defects — all fixed in #55
- Format-string bug — errmsg had 4×
%X but one LSN_FORMAT_ARGS (expands to 2 args). Confirmed real UB, not cosmetic; reproduced warning: more '%' conversions than data arguments.
- DROP DATABASE still used
GetXLogInsertRecPtr() — dropdb() forces a checkpoint before committing, so the old value could be far behind the commit LSN. Now routed through the same deferred path.
- TEMP skipped — v5's Test 9 asserted the opposite; inverted and expanded.
- PITR docs —
recovery_target_inclusive = off documented as required.
4. Open question raised on the call: unlogged tables
The patch filters RELPERSISTENCE_TEMP only, so unlogged tables are still logged. PITR recovers an unlogged table's definition but zero rows — the catalog entry is WAL-logged, the data is not. The log line therefore promises a recovery that returns an empty table. Suggest filtering != RELPERSISTENCE_PERMANENT.
Design discussion (#54)
Two memos: one with the extension option open, one with it explicitly off the table (per Andrey and Kirk).
- Restore points vs. log lines. Kirk's "fake LSN before the commit" is precisely a restore point. Concluded unnecessary:
ProcLastRecPtr gives the exact commit-record start for free, no WAL, no name. A restore point costs WAL on every DROP (including ones that then ROLLBACK), needs wal_level >= replica, needs a unique name, and recovery_target_name stops after the first name match. Its one advantage: it lives in WAL, so it survives losing the logs.
- Why not an extension? Kirill Reshke asked exactly this on the thread in March 2025 — "What stops us from logging all the same inside object access hook defined by extension?" — and was never answered. Nik's userspace version (a
sql_drop event trigger calling pg_create_restore_point()) works today for DROP TABLE. The honest answers: it does not work for DROP DATABASE (event triggers are per-database), it cannot see the commit LSN, and it is opt-in per database so protects nobody by default. The thread needs an answer to Reshke ready.
- XactCallback ABI. v5 mutates the public typedef in place (
xact.h:139). Andrey correctly noted this is not a blocker — Postgres breaks ABI between major releases routinely. What remains is an API-churn argument: every xact-callback author must edit their signature to benefit one feature, when core can just call the function directly.
Review feedback still to apply
From Andrey, on the call:
- Commit message is not Postgres style — needs 2-3 paragraphs on why and what changed, not our process, plus a
Discussion: link.
- The
ProcLastRecPtr comment should be shorter, and should assert the invariant (that the pointer is the actual commit record, and is never invalid inside RecordTransactionCommit) rather than narrate the problem.
- Attribution needs fixing — the commit is currently authored solely as Nikolay Samokhvalov, with no
Author: for Dmitry Lebedev, whose patch this mostly is.
- CI is now enabled on this fork (
PG_CI_ENABLED=1), so the next push runs the real matrix — Linux, Windows, 32-bit, macOS.
Not verified
Stated plainly by the agent that produced #55:
- Docs do not build locally (DocBook DTD fetch fails); the new
<varlistentry> is well-formed and both xref targets exist, but rendered output is unverified.
- pgindent not run (
pg_bsd_indent not installed).
- macOS/arm64 only — no Linux, no 32-bit, no autoconf, no Windows. (CI now closes this.)
- sepgsql not compiled; its XactCallback signature change is inspection-only.
- LSN-exactness assertions assume a quiet WAL stream — the most plausible buildfarm flakiness risk.
- Nothing has been posted to pgsql-hackers or attached to CF #6272.
Next steps
- Rewrite the commit message in Postgres style; tighten the comment; fix attribution.
- Decide on unlogged tables.
- Send v6 to pgsql-hackers with a plain-text email, bump the thread, move CF #6272 to an open commitfest (PG20-1).
- Have an answer ready for Reshke's question.
🤖 Generated with Claude Code
Summary of the Hacking Postgres session of 2026-07-15 (Nikolay Samokhvalov, Kirk Wolak, Andrey Borodin), which worked on the stalled
log_object_dropspatch.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
fix/drop-lsn-v5-fixes, commit15aa5609c65, offorigin/master@11ed011ae22What was found
1. The feature did not work at all (#53)
v5 logged the end LSN of the commit record (
XactLogCommitRecord()returnsXLogInsert'sEndPos). Recovery comparesrecovery_target_lsnagainst each record's start (recoveryStopsBefore()testsrecord->ReadRecPtr, xlogrecovery.c:2580-2582).Consequence: targeting the logged LSN never stops before the drop, with any value of
recovery_target_inclusive— recovery replays the commit and stops before the next record. The object is lost again. As Andrey put it on the call: the logged LSN is one record past the actual commit of the drop. And the DBA cannot correct it by hand, because the record length is not recoverable from the log line.Fixed in #55: logs
ProcLastRecPtr(xlog.h:33), whichXLogInsertRecord()sets to the start of the record just inserted (xlog.c:1112). Confirmed withpg_waldumpthat the logged LSN lands exactly on the drop transaction's COMMIT record. Same idiomCreateCheckPointalready relies on (xlog.c:7772).This was found independently by two agents, from different directions.
2. v5 does not build on current master — explains 7 months of red CI
Two independent breaks, both fixed in #55:
guc_parameters.datnow enforces alphabetical ordering;log_object_dropswas appended afterzero_damaged_pages→ hard build failure.t/050_drop_table_logging.plcollides with master's existing050_redo_segment_missing.pl→ renumbered to 055.Nobody could have reviewed this patch even if they had wanted to.
3. Kirk's four defects — all fixed in #55
%Xbut oneLSN_FORMAT_ARGS(expands to 2 args). Confirmed real UB, not cosmetic; reproducedwarning: more '%' conversions than data arguments.GetXLogInsertRecPtr()—dropdb()forces a checkpoint before committing, so the old value could be far behind the commit LSN. Now routed through the same deferred path.recovery_target_inclusive = offdocumented as required.4. Open question raised on the call: unlogged tables
The patch filters
RELPERSISTENCE_TEMPonly, so unlogged tables are still logged. PITR recovers an unlogged table's definition but zero rows — the catalog entry is WAL-logged, the data is not. The log line therefore promises a recovery that returns an empty table. Suggest filtering!= RELPERSISTENCE_PERMANENT.Design discussion (#54)
Two memos: one with the extension option open, one with it explicitly off the table (per Andrey and Kirk).
ProcLastRecPtrgives the exact commit-record start for free, no WAL, no name. A restore point costs WAL on every DROP (including ones that then ROLLBACK), needswal_level >= replica, needs a unique name, andrecovery_target_namestops after the first name match. Its one advantage: it lives in WAL, so it survives losing the logs.sql_dropevent trigger callingpg_create_restore_point()) works today for DROP TABLE. The honest answers: it does not work for DROP DATABASE (event triggers are per-database), it cannot see the commit LSN, and it is opt-in per database so protects nobody by default. The thread needs an answer to Reshke ready.xact.h:139). Andrey correctly noted this is not a blocker — Postgres breaks ABI between major releases routinely. What remains is an API-churn argument: every xact-callback author must edit their signature to benefit one feature, when core can just call the function directly.Review feedback still to apply
From Andrey, on the call:
Discussion:link.ProcLastRecPtrcomment should be shorter, and should assert the invariant (that the pointer is the actual commit record, and is never invalid insideRecordTransactionCommit) rather than narrate the problem.Author:for Dmitry Lebedev, whose patch this mostly is.PG_CI_ENABLED=1), so the next push runs the real matrix — Linux, Windows, 32-bit, macOS.Not verified
Stated plainly by the agent that produced #55:
<varlistentry>is well-formed and both xref targets exist, but rendered output is unverified.pg_bsd_indentnot installed).Next steps
🤖 Generated with Claude Code