log_object_drops (CF #6272): Kirk Wolak's 4 review items against v5, + commit-LSN off-by-one-record and v5 not building on master

[NikolayS]

Tracking issue for the review items Kirk Wolak raised against v5 of "PoC: Simplify recovery after dropping a table by LOGGING the restore LSN" (log_object_drops GUC), posted verbatim to the Hacking Postgres call chat. Andrey Borodin confirmed on the call this is "a real list of problems in the patch".

• Thread: https://www.postgresql.org/message-id/flat/CACLU5mSr%2Bt%2B6K_Ww6z1fk_%3DjP%3D5-U0CEqMsHjii8uG_6UWQo-g%40mail.gmail.com
• Commitfest: https://commitfest.postgresql.org/patch/6272/
• v5 attachment: authored by Dmitry Lebedev, rebased and posted by Andrey Borodin 2025-12-10
• PR: fix(log_object_drops): correct commit LSN, DROP DATABASE path, TEMP skip, and PITR docs (v5 review fixes) #55

Kirk's four items

• 1. Format-string bug. errmsg format has 4 %X ("drop LSN: %X/%X, commit LSN: %X/%X") but passes one LSN_FORMAT_ARGS() = 2 args. Confirmed real UB, not cosmetic — clang: warning: more '%' conversions than data arguments [-Wformat-insufficient-args]. The last two %X read past the varargs. No "drop LSN" is recorded anywhere in the patch; message now reports the commit LSN only, in master's %X/%08X format.
• 2. DROP DATABASE still uses GetXLogInsertRecPtr(). Kirill Reshke's issue, unfixed on that path. Read before the commit record exists, and dropdb() forces an immediate checkpoint before committing, so it could be far behind the real commit LSN. Now routed through the same deferred XactCallback path as tables.
• 3. Skip TEMP. Not PITR-recoverable and dropped at every session exit → noise. v5's Test 9 asserted the opposite; inverted and expanded (explicit / ON COMMIT DROP / session exit).
• 4. Document PITR use (recovery_target_inclusive). The trap: it defaults to on = stop after the target, so setting only recovery_target_lsn replays the drop and loses the table again. Documented with the required recovery_target_inclusive = off.

Additional findings

• 5. v5 logs the wrong end of the commit record (found while writing the PITR test). XactLogCommitRecord() returns EndRecPtr, but recovery_target_lsn is compared against each record's start (recoveryStopsBefore() tests record->ReadRecPtr). The end LSN is the start of the next record, so even recovery_target_inclusive = off replays the drop — the documented recipe in item 4 does not work without this fix. Now reports ProcLastRecPtr. This one deserves list discussion: it changes the meaning of the LSN in the XactCallback API v5 introduces.
• 6. v5 does not build on current master (explains the long-red CF CI):
  • 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 050_redo_segment_missing.pl → renumbered to 055.
• 7. Testing strengthened (Andrey's ask). v5's tests largely asserted "a line exists", which cannot distinguish a correct LSN from garbage. Now 54 assertions incl. LSN pinned to the actual commit LSN (with ~7 MB of WAL written between DROP and COMMIT so drop LSN and commit LSN cannot be confused), GUC-off, ROLLBACK, ROLLBACK TO SAVEPOINT, TEMP, failed DROP DATABASE, and an end-to-end PITR round trip proving both directions of the recovery_target_inclusive trap.

Open questions for the list

• Should unlogged tables be skipped as well? Contents are not PITR-recoverable either, though the definition is. Kirk's item said TEMP only, so scope was not expanded.
• Is end-of-commit-record or start-of-commit-record the right thing for a general XactCallback LSN (item 5)?
• v5's Assert(!XLogRecPtrIsInvalid(commit_lsn)) is retained — it holds, but fires on every commit.

Not done

• Not posted to pgsql-hackers / not attached to CF #6272 — GitHub PR only so far.
• Docs not rendered (DocBook DTD unavailable offline locally); pgindent not run (pg_bsd_indent not installed); verified on macOS/arm64 + meson + cassert only. See the PR's "What I could NOT verify" section.

[NikolayS]

Consolidating everything for this topic into #55 — content moved to #55 (comment)