On Fri, Sep 1, 2023 at 6:34 PM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:
>
> Dear Dilip,
>
> Thank you for reviewing!
>
> >
> > 1.
> > + conn = connectToServer(&new_cluster, "template1");
> > +
> > + prep_status("Checking for logical replication slots");
> > +
> > + res = executeQueryOrDie(conn, "SELECT slot_name "
> > + "FROM pg_catalog.pg_replication_slots "
> > + "WHERE slot_type = 'logical' AND "
> > + "temporary IS FALSE;");
> >
> >
> > I think we should add some comment saying this query will only fetch
> > logical slots because the database name will always be NULL in the
> > physical slots. Otherwise looking at the query it is very confusing
> > how it is avoiding the physical slots.
>
> Hmm, the query you pointed out does not check the database of the slot...
> We are fetching only logical slots by the condition "slot_type = 'logical'",
> I think it is too trivial to describe in the comment.
> Just to confirm - pg_replication_slots can see alls the slots even if the database
> is not current one.
I think this is fine. Actually I posted comments based on v28 where
the query inside get_old_cluster_logical_slot_infos_per_db() function
was missing the condition on the slot_type = logical but while
commenting I quoted the wrong hunk from the code. Anyway the other
part of the code which I intended is also fixed from v29 so all good.
Thanks :)
> > 4. Looking at the above two comments I feel that now the order should be like
> > - Fetch all the db infos
> > get_db_infos()
> > - loop
> > get_rel_infos()
> > get_old_cluster_logical_slot_infos()
> >
> > -- and now print relation and slot information per database
> > print_db_infos()
>
> Fixed like that. It seems that we go back to old style...
> Now the debug prints are like below:
>
> ```
> source databases:
> Database: "template1"
> relname: "pg_catalog.pg_largeobject", reloid: 2613, reltblspace: ""
> relname: "pg_catalog.pg_largeobject_loid_pn_index", reloid: 2683, reltblspace: ""
> Database: "postgres"
> relname: "pg_catalog.pg_largeobject", reloid: 2613, reltblspace: ""
> relname: "pg_catalog.pg_largeobject_loid_pn_index", reloid: 2683, reltblspace: ""
> Logical replication slots within the database:
> slotname: "old1", plugin: "test_decoding", two_phase: 0
> slotname: "old2", plugin: "test_decoding", two_phase: 0
> slotname: "old3", plugin: "test_decoding", two_phase: 0
Yeah this looks good now.
--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com