On Fri, Mar 29, 2024 at 9:34 AM Hayato Kuroda (Fujitsu) <kuroda.hay...@fujitsu.com> wrote: > > Thanks for updating the patch! Here is a comment for it. > > ``` > + /* > + * By advancing the restart_lsn, confirmed_lsn, and xmin using > + * fast-forward logical decoding, we can verify whether a consistent > + * snapshot can be built. This process also involves saving necessary > + * snapshots to disk during decoding, ensuring that logical decoding > + * efficiently reaches a consistent point at the restart_lsn without > + * the potential loss of data during snapshot creation. > + */ > + pg_logical_replication_slot_advance(remote_slot->confirmed_lsn, > + found_consistent_point); > + ReplicationSlotsComputeRequiredLSN(); > + updated_lsn = true; > ``` > > You added them like pg_replication_slot_advance(), but the function also calls > ReplicationSlotsComputeRequiredXmin(false) at that time. According to the > related > commit b48df81 and discussions [1], I know it is needed only for physical > slots, > but it makes more consistent to call requiredXmin() as well, per [2]: >
Yeah, I also think it is okay to call for the sake of consistency with pg_replication_slot_advance(). -- With Regards, Amit Kapila.
