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

Dear Amit,

Thank you for reviewing!

> Few comments:
> ==============
> 1.
> +       <link
> linkend="view-pg-replication-slots">pg_replication_slots</link>.<structfield>c
> onfirmed_flush_lsn</structfield>
> +       of all slots on the old cluster must be the same as the latest
> +       checkpoint location.
> 
> We can add something like: "This ensures that all the data has been
> replicated before the upgrade." to make it clear why this test is
> important.

Added.

> 2. Move the wal_level related restriction before max_replication_slots.
> 
> 3.
> + /* Is the slot still usable? */
> + if (slot->invalid)
> + {
> + if (script == NULL &&
> + (script = fopen_priv(output_path, "w")) == NULL)
> + pg_fatal("could not open file \"%s\": %s",
> + output_path, strerror(errno));
> +
> + fprintf(script,
> + "slotname :%s\tproblem: The slot is unusable\n",
> + slot->slotname);
> + }
> +
> + /*
> + * Do additional checks to ensure that confirmed_flush LSN of all
> + * the slots is the same as the latest checkpoint location.
> + *
> + * Note: This can be satisfied only when the old cluster has been
> + * shut down, so we skip this for live checks.
> + */
> + if (!live_check && !slot->caught_up)
> 
> Isn't it better to continue for the next slot once we find that slot
> is invalid instead of checking other conditions?

Right, fixed.

> 4.
> +
> + fprintf(script,
> + "slotname :%s\tproblem: The slot is unusable\n",
> + slot->slotname);
> 
> Let's keep it as one string and change the message to: "The slot
> "\"%s\" is invalid"

Changed.

> + fprintf(script,
> + "slotname :%s\tproblem: The slot has not consumed WALs yet\n",
> + slot->slotname);
> + }
> 
> On a similar line, we can change this to: "The slot "\"%s\" has not
> consumed the WAL yet"

Changed.

> 5.
> + snprintf(output_path, sizeof(output_path), "%s/%s",
> + log_opts.basedir,
> + "problematic_logical_relication_slots.txt");
> 
> I think we can name this file as "invalid_logical_replication_slots"
> or simply "logical_replication_slots"

The latter one seems too general for me, "invalid_..." was chosen.

> 6.
> + pg_fatal("The source cluster contains one or more problematic
> logical replication slots.\n"
> + "The needed workaround depends on the problem.\n"
> + "1) If the problem is \"The slot is unusable,\" You can drop such
> replication slots.\n"
> + "2) If the problem is \"The slot has not consumed WALs yet,\" you
> can consume all remaining WALs.\n"
> + "Then, you can restart the upgrade.\n"
> + "A list of problematic logical replication slots is in the file:\n"
> + "    %s", output_path);
> 
> This doesn't match the similar existing comments. So, let's change it
> to something like:
> 
> "Your installation contains invalid logical replication slots.  These
> slots can't be copied so this cluster cannot currently be upgraded.
> Consider either removing such slots or consuming the pending WAL if
> any and then restart the upgrade.  A list of invalid logical
> replication slots is in the file:"

Basically changed to your suggestion, but slightly reworded based on
what Grammarly said.

> Apart from the above, I have edited a few other comments in the patch.
> See attached.

Thanks for attaching! Included.

Best Regards,
Hayato Kuroda
FUJITSU LIMITED