On Tue, Jan 21, 2025 at 8:26 AM Peter Smith <smithpb2...@gmail.com> wrote: > > Some review comments for patch v61-0002 > > ====== > src/backend/replication/slot.c > > 1. > * Whether a slot needs to be invalidated depends on the cause. A slot is > - * removed if it: > + * invalidated if it: > * - RS_INVAL_WAL_REMOVED: requires a LSN older than the given segment > * - RS_INVAL_HORIZON: requires a snapshot <= the given horizon in the given > * db; dboid may be InvalidOid for shared relations > * - RS_INVAL_WAL_LEVEL: is logical > + * - RS_INVAL_IDLE_TIMEOUT: idle slot timeout has occurred > > 1a. > Firstly the wording seems odd. "Is invalidated if it:" (missing words?) > > ~ > > 1b. > Secondly, is this comment strictly correct? IIUC it's not *always* > going to be invalidated just because the cause is one of those listed. > e.g. the code calls InvalidatePossiblyObsoleteSlot but it might not > end up invalidating the slot having a cause RS_INVAL_IDLE_TIMEOUT. >
I feel the phrase "A slot is invalidated if it:" is supposed to be read alongside the respective cause description, such as: "A slot is invalidated if it requires an LSN older than…" "A slot is invalidated if it requires a snapshot <= the…" "A slot is invalidated if it is logical" IIUC, each listed cause specifies a clear condition under which the slot should *always* be invalidated for that cause. To maintain consistency with the header line "A slot is invalidated if it:", I’ve modified the description/condition for RS_INVAL_IDLE_TIMEOUT accordingly. Also, corrected the RS_INVAL_WAL_LEVEL description. Attached the v62 patch set with above mentioned changes and also addressed comments at [1]. [1] https://www.postgresql.org/message-id/CAHut%2BPvC4uPabeGMvDuTQ4S%2B5eX66Y6%2BtU5QMRmB2jDw-Cj2Cw%40mail.gmail.com -- Thanks, Nisha
v62-0001-Enhance-replication-slot-error-handling-slot-inv.patch
Description: Binary data
v62-0002-Introduce-inactive_timeout-based-replication-slo.patch
Description: Binary data
