schema: review-report-v2
target: commitfest-7265
source: postgresql-master
slug: commitfest-7265-review
head: 412ef97d925c54d6135afb4c22cceccb6421d8a2
base: 412ef97d925c54d6135afb4c22cceccb6421d8a2
diff_sha: 7e697bfcf8dea06db874516919a8797af567d5eb1abf8d0e5834c39ec363c6fe
scope_identity: diff7e697bfc
verdict: waiting-on-author
blocker_counts:
  verified: 1
  unverified: 0
date: 2026-09-09

CommitFest 7265 Review — Logical Decoding: Skip Unnecessary Snapshot Distribution

Scope

Patch Summary

The patch optimizes logical decoding snapshot building. When a committed transaction has catalog changes in a different database than the slot's database, and those changes do not touch shared catalogs, the snapshot is not rebuilt and invalidations are not distributed. A new function InvalidationsTouchSharedCatalog() checks whether invalidation messages in the commit record concern shared catalogs by examining SHAREDINVALSMGR_ID messages (checking rlocator.dbOid == InvalidOid) and all other message types (checking cc.dbId == InvalidOid, since the dbId field is at the same union offset for all non-SMGR types).

Motivation: A single logical slot on db1 is unaffected by ANALYZE/catalog traffic in db2 that pins xmin via a long-running transaction, eliminating O(k²) reorder-buffer spill growth. The reproducer shows 19MB→0 spill reduction.

Verdict

waiting-on-author

Rationale: One BLOCKER finding (F1) survives verification. The parsed->msgs vs ReorderBufferGetInvalidations divergence is real but benign in the demonstrated scenario — the patch's design is correct. However, the author has not responded to Shlok Kyal's on-list review raising this concern. The repair (documentation comment explaining why parsed->msgs is the correct basis) is validated. A MAJOR test gap (F2) also requires a TAP test before this can advance.

Findings

Verification Summary

All five findings verified. F1's divergence is real but the impact is benign — the patch's design is correct. The repair is documentation. F2's test gap is addressed by a validated TAP test. F3–F5 are comment improvements.

Repair Validation Summary

Repair patches:

• repair_only.patch (F1/F3/F4/F5): comment-only changes to decode.c and snapbuild.c
• test_add.patch (F2): new TAP test + Makefile change

Both patches apply cleanly on top of the frozen patch. Full build and all test suites pass (contrib/test_decoding, src/test/recovery, src/test/subscription).

Security Review

No security vulnerability found. The shared-catalog detection invariant is complete and correct for all current SharedInvalidationMessage types. The union overlay is type-safe. The builder->snapshot == NULL guard prevents NULL-deref/UAF. No new trust-boundary, authorization, TOCTOU, or search-path surface is introduced.

pgsql-hackers Thread Ledger

Thread 655705: "logical decoding: skip unnecessary snapshot distribution."

Finding links:

• F1: already-raised — Shlok Kyal raised the divergence on-list; author has not replied
• F2–F5: new — harness findings not previously raised on-list

Changed-Code Coverage

Test set executed

Fresh disposable coverage build with --coverage at frozen HEAD. No stale/mismatched counters.

Metrics

Uncovered regions

1. decode.c:1377-1378 — SMGR invalidation branch (2 lines, 1 branch)

• Classification: infeasible-with-reason — SHAREDINVALSMGR_ID messages are non-transactional and never appear in commit WAL records. Dead defensive code. Waived.

1. snapbuild.c:1088 branch 2 — NULL-guard when distribute == false (1 branch)

• Classification: requires-special-harness-or-injection-point — requires distribute==false AND builder->snapshot==NULL simultaneously, which only occurs during slot initialization before any same-DB catalog commit. Test packet provided (TAP with cross-DB race or injection point).

Whole-function diagnostics

Non-instrumented changes

• src/include/replication/snapbuild.h: SnapBuildCommitTxn signature change (added bool distribute). Header not instrumented by gcov; consumed by instrumented decode.c and snapbuild.c.

Aggregated Test Plan

1. Existing: contrib/test_decoding (14 regression + 1 TAP), src/test/recovery (55 TAP), src/test/subscription (39 TAP)
2. Proposed (F2): src/test/subscription/t/039_snapshot_inval_xdb.pl — 4 scenarios covering cross-DB non-shared, cross-DB shared, rolled-back savepoint, and committed-with-rolled-back-sibling cases
3. Proposed (coverage): src/test/recovery/t/107_cross_db_snapbuild_null.pl — TAP test for NULL-guard branch coverage

Provenance

Residual Risk

• The parsed->msgs vs ReorderBufferGetInvalidations divergence (F1) is benign in the current code but could become a real bug if invalidation/rollback handling changes in the future. The documentation repair mitigates this risk.
• The NULL-guard branch (snapbuild.c:1088 branch 2) is uncovered by the current test set; it requires a cross-DB race during slot initialization or an injection point to exercise deterministically.
• The SMGR branch in InvalidationsTouchSharedCatalog is dead defensive code; future changes to sinval.h that add new message types with different layouts could break the union overlay assumption.