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

Dear Amit,

Thanks for reviewing! The patch could be available at [1].

> 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?

Agreed for verifying the max_replication_slots. There are several ways to add it,
so I chose the simplest one - store the #slots to global variable and compare
between it and max_replication_slots.
As for the max_wal_senders, I don't think it should be. As you said, there is a
possibility user-defined background worker uses the slot and consumes WALs.

> 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?

OK, I can tell the example here. Should it be described on the source?

Assuming that there is a valid logical replication slot as follows:

```
postgres=# select slot_name, plugin, restart_lsn, wal_status, two_phase from pg_replication_slots;
slot_name | plugin | restart_lsn | wal_status | two_phase
-----------+---------------+-------------+------------+-----------
test | test_decoding | 0/15665A8 | reserved | f
(1 row)

postgres=# select * from pg_current_wal_lsn();
pg_current_wal_lsn
--------------------
0/15665E0
(1 row)
```

And here let's execute the pg_resetwal to the pg server.
The existing wal segment file is purged and moved to next seg.

```
$ pg_ctl stop -D data_N1/
waiting for server to shut down.... done
server stopped
$ pg_resetwal -l 000000010000000000000002 data_N1/
Write-ahead log reset
$ pg_ctl start -D data_N1/ -l N1.log
waiting for server to start.... done
server started
```

After that the logical slot cannot move foward anymore because the required WALs
are removed, whereas the wal_status is still "reserved".

```
postgres=# select slot_name, plugin, restart_lsn, wal_status, two_phase from pg_replication_slots;
slot_name | plugin | restart_lsn | wal_status | two_phase
-----------+---------------+-------------+------------+-----------
test | test_decoding | 0/15665A8 | reserved | f
(1 row)

postgres=# select * from pg_current_wal_lsn();
pg_current_wal_lsn
--------------------
0/2028328
(1 row)

postgres=# select * from pg_logical_slot_get_changes('test', NULL, NULL);
ERROR: requested WAL segment pg_wal/000000010000000000000001 has already been removed
```

pg_upgrade runs pg_dump and then pg_resetwal, so dumping slots must be done
separately to avoid above error.

> 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?

Currently I'm not sure it should be considered. Do you have in mind?

candidate_restart_lsn for the slot is set ony when XLOG_RUNNING_XACTS is decoded
(LogicalIncreaseRestartDecodingForSlot()), and is set as restart_lsn later. So
there are few timings to update the value and we cannot determine the accepatble
boundary.

Furthermore, I think restart point is not affect the result for replicating
changes on subscriber because it is always behind confimed_flush.

[1]: https://www.postgresql.org/message-id/TYAPR01MB5866E9ED5B8C5AD7F7AC062FF539A%40TYAPR01MB5866.jpnprd01.prod.outlook.com

Best Regards,
Hayato Kuroda
FUJITSU LIMITED