On Wed, Mar 25, 2026 at 12:17 PM Bharath Rupireddy < [email protected]> wrote:
> Hi, > > On Tue, Mar 24, 2026 at 11:50 PM Masahiko Sawada <[email protected]> > wrote: > > > > > Please find the v3 patch for further review. > > > > Thank you for updating the patch. I think the patch is reasonably > > simple and can avoid unnecessary overheads well due to XID-based > > checks. Here are some comments: > > Thank you for reviewing the patch. > > > vacuum_get_cutoff() is also called by VACUUM FULL, CLUSTER, and > > REPACK. I'm not sure that users would expect the slot invalidation > > also in these commands. I think it's better to leave > > vacuum_get_cutoff() a pure cutoff computation function and we can try > > to invalidate slots in heap_vacuum_rel(). It requires additional > > ReadNextTransactionId() but we can live with it, or we can make > > vacuum_get_cutoffs() return the nextXID as well (stored in *cutoffs). > > +1. I chose to perform the slot invalidation in heap_vacuum_rel by > getting the next txn ID and calling vacuum_get_cutoffs again when a > slot gets invalidated. IMHO, this is simple than adding a flag and do > the invalidation selectively in vacuum_get_cutoffs. > > > if (TransactionIdPrecedes(oldestXmin, cutoffXID)) > > + { > > + invalidated = > InvalidateObsoleteReplicationSlots(RS_INVAL_XID_AGE, > > + 0, > > + InvalidOid, > > + > InvalidTransactionId, > > + nextXID); > > + } > > > > I think it's better to check the procArray->replication_slot_xmin and > > procArray->replication_slot_catalog_xmin before iterating over each > > slot. Otherwise, we would end up checking every slot even when a long > > running transaction holds the oldestxmin back. > > +1. Changed. > > > + if (!TransactionIdIsNormal(cutoffXID)) > > + cutoffXID = FirstNormalTransactionId; > > > > These codes have the same comment but are doing a slightly different > > thing. I guess the latter is missing '-'? > > Fixed the typo. > > I fixed a test error being reported in CI. > > Please find the attached v4 patch for further review. > InvalidateObsoleteReplicationSlots(uint32 possible_causes, XLogSegNo oldestSegno, Oid dboid, - TransactionId snapshotConflictHorizon) + TransactionId snapshotConflictHorizon, TransactionId nextXID) May be add TransactionId nextXID in a new line? Thinking loud, vacuum doesn't run on a hot_standby, that means this GUC is not applicable for hot_standby. Is this intended? Why not call during checkpoint/restorepoint itself like other slot invalidation checks? Thanks, Satya
