RE: [PoC] pg_upgrade: allow to upgrade publisher node

Dear Vignesh,

Thank you for reviewing! New version will be attached the next post.

> Few comments
> 1) check_for_parameter_settings, check_for_confirmed_flush_lsn and
> check_are_logical_slots_active functions all have the same messages,
> we can keep it unique so that it is easy for user to interpret:
> +check_for_parameter_settings(ClusterInfo *new_cluster)
> +{
> +       PGresult   *res;
> +       PGconn     *conn = connectToServer(new_cluster, "template1");
> +       int                     max_replication_slots;
> +       char       *wal_level;
> +
> +       prep_status("Checking for logical replication slots");
> +
> +       res = executeQueryOrDie(conn, "SHOW max_replication_slots;");
> 
> +check_for_confirmed_flush_lsn(ClusterInfo *cluster)
> +{
> +       int                     i,
> +                               ntups,
> +                               i_slotname;
> +       bool            is_error = false;
> +       PGresult   *res;
> +       DbInfo     *active_db = &cluster->dbarr.dbs[0];
> +       PGconn     *conn = connectToServer(cluster, active_db->db_name);
> +
> +       Assert(user_opts.include_logical_slots);
> +
> +       /* --include-logical-replication-slots can be used since PG16. */
> +       if (GET_MAJOR_VERSION(cluster->major_version) <= 1500)
> +               return;
> +
> +       prep_status("Checking for logical replication slots");
> 
> +check_are_logical_slots_active(ClusterInfo *cluster)
> +{
> +       int                     i,
> +                               ntups,
> +                               i_slotname;
> +       bool            is_error = false;
> +       PGresult   *res;
> +       DbInfo     *active_db = &cluster->dbarr.dbs[0];
> +       PGconn     *conn = connectToServer(cluster, active_db->db_name);
> +
> +       Assert(user_opts.include_logical_slots);
> +
> +       /* --include-logical-replication-slots can be used since PG16. */
> +       if (GET_MAJOR_VERSION(cluster->major_version) <= 1500)
> +               return;
> +
> +       prep_status("Checking for logical replication slots");

Changed. How do you think?

> 2) This function can be placed above get_logical_slot_infos and the
> prototype from this file can be removed:
> +/*
> + * get_logical_slot_infos_per_db()
> + *
> + * gets the LogicalSlotInfos for all the logical replication slots of
> the database
> + * referred to by "dbinfo".
> + */
> +static void
> +get_logical_slot_infos_per_db(ClusterInfo *cluster, DbInfo *dbinfo)
> +{
> +       PGconn     *conn = connectToServer(cluster,
> +
>     dbinfo->db_name);

Removed.

> 3) LogicalReplicationSlotInfo should be placed after LogicalRepWorker
> to keep the order consistent:
>  LogicalRepCommitPreparedTxnData
>  LogicalRepCtxStruct
> +LogicalReplicationSlotInfo
>  LogicalRepMsgType

Indeed, fixed.

> 4) "existence of slots" be changed to "existence of slots."
> +               /*
> +                * If --include-logical-replication-slots is required, check the
> +                * existence of slots
> +                */

The comma was added.

> 5) This comment can be removed:
> + *
> + * XXX: add more attributes if needed
> + */

Removed. Additionally, another XXX which mentioned about physical slots was also removed.

Best Regards,
Hayato Kuroda
FUJITSU LIMITED