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

On Fri, Sep 15, 2023 at 8:43 AM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:
>

Few comments:
1. Why is the FPI record (XLOG_FPI_FOR_HINT) not considered a record
to be ignored? This can be generated during reading system tables.

2.
+binary_upgrade_validate_wal_record_types_after_lsn(PG_FUNCTION_ARGS)
{
...
+ if (initial_record)
+ {
+ /* Initial record must be XLOG_CHECKPOINT_SHUTDOWN */
+ if (!CHECK_WAL_RECORD(rmid, info, RM_XLOG_ID,
+   XLOG_CHECKPOINT_SHUTDOWN))
+ result = false;
...
+ if (!CHECK_WAL_RECORD(rmid, info, RM_XLOG_ID, XLOG_CHECKPOINT_SHUTDOWN) &&
+ !CHECK_WAL_RECORD(rmid, info, RM_XLOG_ID, XLOG_CHECKPOINT_ONLINE) &&
+ !CHECK_WAL_RECORD(rmid, info, RM_STANDBY_ID, XLOG_RUNNING_XACTS) &&
+ !CHECK_WAL_RECORD(rmid, info, RM_HEAP2_ID, XLOG_HEAP2_PRUNE))
+ result = false;
...
}

Isn't it better to immediately return false if any unexpected WAL is
found? This will avoid reading unnecessary WAL

3.
+Datum
+binary_upgrade_validate_wal_record_types_after_lsn(PG_FUNCTION_ARGS)
+{
...
+
+ CHECK_IS_BINARY_UPGRADE;
+
+ /* Quick exit if the given lsn is larger than current one */
+ if (start_lsn >= curr_lsn)
+ PG_RETURN_BOOL(true);

Why do you return true here? My understanding was if the first record
is not a shutdown checkpoint record then it should fail, if that is
not true then I think we need to explain the same in comments.

--
With Regards,
Amit Kapila.