On Thu, Aug 31, 2023 at 7:56 PM Hayato Kuroda (Fujitsu) <kuroda.hay...@fujitsu.com> wrote:
> Thanks for giving comments! Thanks > > Some comments in 0002 > > > > 1. > > + res = executeQueryOrDie(conn, "SELECT slot_name " > > + "FROM pg_catalog.pg_replication_slots " > > + "WHERE slot_type = 'logical' AND " > > + "temporary IS FALSE;"); > > > > What is the reason we are ignoring temporary slots here? I think we > > better explain in the comments. > > The temporary slots were expressly ignored while checking because such slots > cannot exist after the upgrade. Before doing pg_upgrade, both old and new > cluster > must be turned off, and they start/stop several times during the upgrade. > > How do you think? LGTM > > > 2. > > + res = executeQueryOrDie(conn, "SELECT slot_name " > > + "FROM pg_catalog.pg_replication_slots " > > + "WHERE slot_type = 'logical' AND " > > + "temporary IS FALSE;"); > > + > > + if (PQntuples(res)) > > + pg_fatal("New cluster must not have logical replication slots but > > found \"%s\"", > > + PQgetvalue(res, 0, 0)); > > > > It looks a bit odd to me that first it is fetching all the logical > > slots from the new cluster and then printing the name of one of the > > slots. If it is printing the name of the slots then shouldn't it be > > printing all the slots' names or it should just say that there > > existing slots on the new cluster without giving any names? And if we > > are planning for option 2 i.e. not printing the name then better to > > put LIMIT 1 at the end of the query. > > I'm planning to change that the number of slots are reported by using > count(*). Yeah, that seems a better option. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com