Dear Hou, Thank you for reviewing! PSA new version! PSA new version.
> Here are some comments:
>
> 1.
>
> bool reap_child(bool wait_for_child);
> +
> +XLogRecPtr strtoLSN(const char *str, bool *have_error);
>
> This function has be removed.
Removed.
> 2.
>
> + if (nslots_on_new)
> + {
> + if (nslots_on_new == 1)
> + pg_fatal("New cluster must not have logical replication
> slots but found a slot.");
> + else
> + pg_fatal("New cluster must not have logical replication
> slots but found %d slots.",
> + nslots_on_new);
>
> We could try ngettext() here:
> pg_log_warning(ngettext("New cluster must not have logical
> replication slots but found %d slot.",
> "New
> cluster must not have logical replication slots but found %d slots",
>
> nslots_on_new)
I agreed to use ngettext(), but I disagreed to change to warning.
Changed to use ngettext().
> 3.
> - create_script_for_old_cluster_deletion(&deletion_script_file_name);
> -
>
> Is there a reason for reordering this function ? Sorry If I missed some
> previous discussions.
We discussed to move create_logical_replication_slots(), but not for
create_script_for_old_cluster_deletion(). Restored.
> 4.
>
> @@ -610,6 +724,12 @@ free_db_and_rel_infos(DbInfoArr *db_arr)
> {
> free_rel_infos(&db_arr->dbs[dbnum].rel_arr);
> pg_free(db_arr->dbs[dbnum].db_name);
> +
> + /*
> + * Logical replication slots must not exist on the new cluster
> before
> + * create_logical_replication_slots().
> + */
> + Assert(db_arr->dbs[dbnum].slot_arr.nslots == 0);
>
>
> I think the assert is not necessary, as the patch will check the new cluster's
> slots in another function. Besides, this function is not only used for new
> cluster, but the comment only mentioned the new cluster which seems a bit
> inconsistent. So, how about removing it ?
Amit also pointed out, so removed the Assertion and comment.
> 5.
> (cluster == &new_cluster) ?
> - " -c synchronous_commit=off -c fsync=off -c
> full_page_writes=off" : "",
> + " -c synchronous_commit=off -c fsync=off -c
> full_page_writes=off" :
> + " -c max_slot_wal_keep_size=-1",
>
> I think we need to set max_slot_wal_keep_size on new cluster as well,
> otherwise
> it's possible that the new created slots get invalidated during upgrade, what
> do you think ?
Added.
> 6.
>
> + bool is_lost; /* Is the slot in 'lost'? */
> +} LogicalSlotInfo;
>
> Would it be better to use 'invalidated', as the same is used in error message
> of ReportSlotInvalidation() and logicaldecoding.sgml.
Per suggestion from Amit, changed to 'invalid'.
> 7.
> + for (dbnum = 0; dbnum < old_cluster.dbarr.ndbs; dbnum++)
> + {
> ...
> + if (script)
> + {
> + fclose(script);
> +
> + pg_log(PG_REPORT, "fatal");
> + pg_fatal("The source cluster contains one or more
> problematic logical replication slots.\n"
>
> I think we should do this pg_fatal out of the for() loop, otherwise we cannot
> collect all the problematic slots.
Yeah, agreed. Fixed.
Also, based on the discussion [1], I added an elog(ERROR) in
InvalidatePossiblyObsoleteSlot().
[1]:
https://www.postgresql.org/message-id/CAA4eK1%2BWBphnmvMpjrxceymzuoMuyV2_pMGaJq-zNODiJqAa7Q%40mail.gmail.com
Best Regards,
Hayato Kuroda
FUJITSU LIMITED
v34-0001-Persist-logical-slots-to-disk-during-a-shutdown-.patch
Description: v34-0001-Persist-logical-slots-to-disk-during-a-shutdown-.patch
v34-0002-pg_upgrade-Allow-to-replicate-logical-replicatio.patch
Description: v34-0002-pg_upgrade-Allow-to-replicate-logical-replicatio.patch
