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

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