On Wed, Oct 25, 2023 at 1:39 PM Bharath Rupireddy <bharath.rupireddyforpostg...@gmail.com> wrote: > > On Wed, Oct 25, 2023 at 11:39 AM Amit Kapila <amit.kapil...@gmail.com> wrote: > > > > On Tue, Oct 24, 2023 at 1:20 PM Bharath Rupireddy > > <bharath.rupireddyforpostg...@gmail.com> wrote: > > > > > > > > > I spent some time on the v57 patch and it looks good to me - tests are > > > passing, no complaints from pgindent and pgperltidy. I turned the CF > > > entry https://commitfest.postgresql.org/45/4273/ to RfC. > > > > > > > Thanks, the patch looks mostly good to me but I am not convinced of > > keeping the tests across versions in this form. I don't think they are > > tested in BF, only one can manually create a setup to test. Shall we > > remove it for now and then consider it separately? > > I think we can retain the test_upgrade_from_pre_PG17 because it is not > only possible to trigger it manually but also one can write a CI > workflow to trigger it. >
It would be better to gauge its value separately and add it once the main patch is committed. I am slightly unhappy even with the hack used for pre-version testing in previous patch which is as follows: +# XXX: Older PG version had different rules for the inter-dependency of +# 'max_wal_senders' and 'max_connections', so assign values which will work for +# all PG versions. If Cluster.pm is fixed this code is not needed. +$old_publisher->append_conf( + 'postgresql.conf', qq[ +max_wal_senders = 5 +max_connections = 10 +]); There should be a way to avoid this but we can decide it afterwards. I don't want to hold the main patch for this point. What do you think? > > Apart from that, I have made minor modifications in the docs to adjust > > the order of various prerequisites. > > + <para> > + <application>pg_upgrade</application> attempts to migrate logical > + replication slots. This helps avoid the need for manually defining the > + same replication slots on the new publisher. Migration of logical > + replication slots is only supported when the old cluster is version 17.0 > + or later. Logical replication slots on clusters before version 17.0 will > + silently be ignored. > + </para> > > + The new cluster must not have permanent logical replication slots, > i.e., > > How about using "logical slots" in place of "logical replication > slots" to be more generic? We agreed and changed the function name to > Yeah, I am fine with that and I can take care of it before committing unless there is more to change. -- With Regards, Amit Kapila.