On Fri, Sep 15, 2023 at 8:43 AM Hayato Kuroda (Fujitsu) <kuroda.hay...@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.