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

Dear Bharath,

Thank you for giving comments! Before addressing your comments,
I wanted to reply some of them.

> 4.
> + /*
> + * There is a possibility that following records may be generated
> + * during the upgrade.
> + */
> + is_valid = is_xlog_record_type(rmid, info, RM_XLOG_ID,
> XLOG_CHECKPOINT_SHUTDOWN) ||
> + is_xlog_record_type(rmid, info, RM_XLOG_ID,
> XLOG_CHECKPOINT_ONLINE) ||
> + is_xlog_record_type(rmid, info, RM_XLOG_ID, XLOG_SWITCH) ||
> + is_xlog_record_type(rmid, info, RM_XLOG_ID,
> XLOG_FPI_FOR_HINT) ||
> + is_xlog_record_type(rmid, info, RM_XLOG_ID,
> XLOG_PARAMETER_CHANGE) ||
> + is_xlog_record_type(rmid, info, RM_STANDBY_ID,
> XLOG_RUNNING_XACTS) ||
> + is_xlog_record_type(rmid, info, RM_HEAP2_ID,
> XLOG_HEAP2_PRUNE);
>
> What if we missed to capture the WAL records that may be generated
> during upgrade?

If such records are generated before calling binary_upgrade_validate_wal_logical_end(),
the upgrading would fail. Otherwise it would be succeeded. Anyway, we don't care
such records because those aren't required to be replicated. The main thing we
want to detect is that we don't miss any record generated before server shutdown.

>
> What happens if a custom WAL resource manager generates table/index AM
> WAL records during upgrade?

If such records are found, definitely we cannot distinguish whether it is acceptable.
We do not have a way to know the property of custom WALs. We didn't care as there
are other problems in the approach, if such a facility is invoked.
Please see the similar discussion [1].

>
> What happens if new WAL records are added that may be generated during
> the upgrade? Isn't keeping this code extensible and in sync with
> future changes a problem?

Actually, others also pointed out the similar point. Originally we just checked
confirmed_flush_lsn and "latest checkpoint lsn" reported by pg_controldata, but
found an issue what the upgrading cannot be passed if users do pg_upgrade --check
just before the actual upgrade. Then we discussed some idea but they have some
disadvantages, so we settled on the current idea. Here is a summary which
describes current situation it would be quite helpful [2]
(maybe you have already known).

> Or we'd better say that any custom WAL
> records are found after the slot's confirmed flush LSN, then the slot
> isn't upgraded?

After concluding how we ensure, we can add the sentence accordingly.

>
> 5. In continuation to the above comment:
>
> Why can't this logic be something like - if there's any WAL record
> seen after a slot's confirmed flush LSN is of type generated by WAL
> resource manager having the rm_decode function defined, then the slot
> can't be upgraded.

Thank you for giving new approach! We have never seen the approach before,
but at least XLOG and HEAP2 rmgr have a decode function. So that
XLOG_CHECKPOINT_SHUTDOWN, XLOG_CHECKPOINT_ONLINE, and XLOG_HEAP2_PRUNE cannot
be ignored the approach, seems not appropriate.
If you have another approach, I'm very happy if you post.

[1]: https://www.postgresql.org/message-id/ZNZ4AxUMIrnMgRbo%40momjian.us
[2]: https://www.postgresql.org/message-id/CAA4eK1JVKZGRHLOEotWi%2Be%2B09jucNedqpkkc-Do4dh5FTAU%2B5w%40mail.gmail.com

Best Regards,
Hayato Kuroda
FUJITSU LIMITED