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

On Thu, Jun 8, 2023 at 9:24 AM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:
>

Few comments/questions
====================
1.
+check_for_parameter_settings(ClusterInfo *new_cluster)
{
...
+
+ res = executeQueryOrDie(conn, "SHOW max_replication_slots;");
+ max_replication_slots = atoi(PQgetvalue(res, 0, 0));
+
+ if (max_replication_slots == 0)
+ pg_fatal("max_replication_slots must be greater than 0");
...
}

Won't it be better to verify that the value of "max_replication_slots"
is greater than the number of logical slots we are planning to copy
from old on the new cluster? Similar to this, I thought whether we
need to check the value of max_wal_senders? But, I guess one can
simply decode from slots by using APIs, so not sure about that. What
do you think?

2.
+ /*
+ * Dump logical replication slots if needed.
+ *
+ * XXX We cannot dump replication slots at the same time as the schema
+ * dump because we need to separate the timing of restoring
+ * replication slots and other objects. Replication slots, in
+ * particular, should not be restored before executing the pg_resetwal
+ * command because it will remove WALs that are required by the slots.
+ */
+ if (user_opts.include_logical_slots)

Can you explain this point a bit more with some example scenarios?
Basically, if we had sent all the WAL before the upgrade then why do
we need to worry about the timing of pg_resetwal?

3. I see that you are trying to ensure that all the WAL has been
consumed for a slot except for shutdown_checkpoint in patch 0003 but
do we need to think of any interaction with restart_lsn
(MyReplicationSlot->data.restart_lsn) which is the start point to read
WAL for decoding by walsender?

--
With Regards,
Amit Kapila.