On Thu, Jul 10, 2025 at 2:35 PM Amit Kapila <amit.kapil...@gmail.com> wrote: > > On Thu, Jul 10, 2025 at 2:23 PM vignesh C <vignes...@gmail.com> wrote: > > > > Few comments: > > 1) With the current approach invalidation will not happen for logical > > replication slots during upgrade operation, I felt we could retain > > this assertion just in case in the future it gets called from > > elsewhere, do you feel this assertion should be removed in the new > > approach too? > > - /* > > - * The logical replication slots shouldn't be invalidated > > as GUC > > - * max_slot_wal_keep_size is set to -1 and > > - * idle_replication_slot_timeout is set to 0 during the > > binary > > - * upgrade. See check_old_cluster_for_valid_slots() > > where we ensure > > - * that no invalidated before the upgrade. > > - */ > > - Assert(!(*invalidated && SlotIsLogical(s) && > > IsBinaryUpgrade)); > > > > I don't think we need this assertion. This is a static function, and > we skipped calling this function in its caller, so it doesn't make > sense to have this assertion.
I agree. > > 2) Documentation > > 2.a) Currently slot invalidation will not happen during upgrade > > because of idle_replication_slot_timeout, do you feel we should add a > > note in documentation or is it ok not to mention? > > 2.b) Currently WAL removal will not happen during upgrade because of > > max_slot_wal_keep_size, should we add a note about this. > > > > We skip or do a few special things in other parts of the code during > BinaryUpgrade, but don't mention those, so don't think we need to > mention this one as well. Make sense -- Regards, Dilip Kumar Google
