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. > 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. -- With Regards, Amit Kapila.
