Re: pg_upgrade's interaction with pg_resetwal seems confusing

On Mon, 16 Oct 2023 at 10:33, Hayato Kuroda (Fujitsu)
<kuroda(dot)hayato(at)fujitsu(dot)com> wrote:
>
> Dear Vignesh,
>
> Thank you for reviewing! PSA new version.
>
> >
> > Few comments:
> > 1) Most of the code in binary_upgrade_set_next_oid is similar to
> > SetNextObjectId, but SetNextObjectId has the error handling to report
> > an error if an invalid nextOid is specified:
> > if (ShmemVariableCache->nextOid > nextOid)
> > elog(ERROR, "too late to advance OID counter to %u, it is now %u",
> > nextOid, ShmemVariableCache->nextOid);
> >
> > Is this check been left out from binary_upgrade_set_next_oid function
> > intentionally? Have you left this because it could be like a dead
> > code. If so, should we have an assert for this here?
>
> Yeah, they were removed intentionally, but I did rethink that they could be
> combined. ereport() would be skipped during the upgrade mode. Thought?
>
> Regarding the first ereport(ERROR), it just requires that we are doing initdb.
>
> As for the second ereport(ERROR), it requires that next OID is not rollbacked.
> The restriction seems OK during the initialization, but it is not appropriate for
> upgrading: wraparound of OID counter might be occurred on old cluster but we try
> to restore the counter anyway.
>
> > 2) How about changing issue_warnings_and_set_oid function name to
> > issue_warnings_and_set_next_oid?
> > void
> > -issue_warnings_and_set_wal_level(void)
> > +issue_warnings_and_set_oid(void)
> > {
>
> Fixed.
>
> > 3) We have removed these comments, is there any change to the rsync
> > instructions? If so we could update the comments accordingly.
> > - * We unconditionally start/stop the new server because
> > pg_resetwal -o set
> > - * wal_level to 'minimum'. If the user is upgrading standby
> > servers using
> > - * the rsync instructions, they will need pg_upgrade to write its final
> > - * WAL record showing wal_level as 'replica'.
> >
>
> Hmm, I thought comments for rsync seemed outdated so that removed. I still think
> this is not needed. Since controlfile->wal_level is not updated to 'minimal'
> anymore, the unconditional startup is not required for physical standby.

We can update the commit message with the details of the same, it will
help to understand that it is intentionally done.

There are couple of typos with the new patch:
1) "uprade logical replication slot" should be "upgrade logical
replication slot":
Previously, the OID counter is restored by invoking pg_resetwal with the -o
option, at the end of upgrade. This is not problematic for now, but WAL removals
are not happy if we want to uprade logical replication slot. Therefore, a new
upgrade function is introduced to reset next OID.
2) "becasue the value" should be "because the value":
Note that we only update the on-memory data to avoid concurrent update of
control with the chekpointer. It is harmless becasue the value would be set at
shutdown checkpoint.

Regards,
Vignesh