On Tue, Oct 24, 2023 at 11:32 AM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:
>
> > If we don't want to keep it generic then we should use something like
> > 'contains_decodable_change'. 'is_change_decodable' could have suited
> > here if we were checking a particular change.
>
> I kept the name for now. How does Bharath think?
No more bikeshedding from my side. +1 for processing_required as-is.
> > > 6. An optimization in count_old_cluster_logical_slots(void): Turn
> > > slot_count to a function static variable so that the for loop isn't
> > > required every time because the slot count is prepared in
> > > get_old_cluster_logical_slot_infos only once and won't change later
> > > on. Do you see any problem with the following? This saves a few CPU
> > > cycles when there are large number of replication slots.
> > > {
> > > static int slot_count = 0;
> > > static bool first_time = true;
> > >
> > > if (first_time)
> > > {
> > > for (int dbnum = 0; dbnum < old_cluster.dbarr.ndbs; dbnum++)
> > > slot_count += old_cluster.dbarr.dbs[dbnum].slot_arr.nslots;
> > >
> > > first_time = false;
> > > }
> > >
> > > return slot_count;
> > > }
> > >
> >
> > This may not be a problem but this is also not a function that will be
> > used frequently. I am not sure if adding such code optimizations is
> > worth it.
>
> Not addressed.
count_old_cluster_logical_slots is being called 3 times during
pg_upgrade and every time counting number of slots for all the
databases seems redundant IMV especially given the fact that the slot
count is computed once at the beginning and never changes. When the
replication slots on the cluster are on the higher side, every time
counting *may* prove costly. And, the use of static variables isn't a
huge change requiring a different set of infra or as such, it's a
simple pattern.
Having said above, if others don't see a merit in it, I'm okay to
withdraw my comment.
> Below commands are an example of the test.
>
> ```
> # test PG9.5 -> patched HEAD
> $ oldinstall=/home/hayato/older/pg95 make check PROVE_TESTS='t/003_upgrade_logical_replication_slots.pl'
Oh, I get it. Thanks.
> Also, based on a comment [2], the upgrade function was renamed to
> 'binary_upgrade_logical_slot_has_caught_up'.
+1.
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.
--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
