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