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

Dear Vignesh,

Thank you for reviewing! PSA new version patch set.

> Few minor comments:
> 1) we could remove the variable slotname from the below code by using
> PQgetvalue directly in pg_log:
> +       for (i = 0; i < ntups; i++)
> +       {
> +               char       *slotname;
> +
> +               is_error = true;
> +
> +               slotname = PQgetvalue(res, i, i_slotname);
> +
> +               pg_log(PG_WARNING,
> +                          "\nWARNING: logical replication slot \"%s\"
> is not active",
> +                          slotname);
> +       }

Removed. Such codes were in two functions, and both of them were fixed.

> 2) This include "catalog/pg_control.h" should be after inclusion pg_collation.h
>  #include "catalog/pg_authid_d.h"
> +#include "catalog/pg_control.h"
>  #include "catalog/pg_collation.h"

Moved.

> 3) This spurious addition line change might not be required in this patch:
>  --- a/src/bin/pg_upgrade/t/003_logical_replication_slots.pl
> +++ b/src/bin/pg_upgrade/t/003_logical_replication_slots.pl
> @@ -85,11 +85,39 @@ $old_node->safe_psql(
>  ]);
> 
>  my $result = $old_node->safe_psql('postgres',
> -       "SELECT count (*) FROM
> pg_logical_slot_get_changes('test_slot', NULL, NULL)"
> +       "SELECT count(*) FROM
> pg_logical_slot_peek_changes('test_slot', NULL, NULL)"
>  );
> +
>  is($result, qq(12), 'ensure WALs are not consumed yet');
>  $old_node->stop;

I removed the line.
In the first place, what I wanted to check here was that pg_upgrade failed because
WALs were not consumed. So if pg_logical_slot_get_changes() was called here, all
of WALs were consumed here and the subsequent command was sucseeded. This was not
happy for us and that's why changed to pg_logical_slot_peek_changes().
But after considering more, I thought that calling the function was not the mandatory
because no one needed the output.So removed.

> 4) This inclusion "#include "access/xlogrecord.h" is not required:
>  #include "postgres_fe.h"
> 
> +#include "access/xlogrecord.h"
> +#include "access/xlog_internal.h"
>  #include "catalog/pg_authid_d.h"

Removed.

> 5)"thepublisher's" should be "the publisher's"
>  When a live check is requested, there is a possibility of additional changes
> occurring, which may cause the current WAL position to exceed the
> confirmed_flush_lsn
> of the slot. As a result, we check the confirmed_flush_lsn of each logical slot
> instead. This is sufficient as all the WAL records will be sent during
> thepublisher's
> shutdown.

Fixed.

Best Regards,
Hayato Kuroda
FUJITSU LIMITED