Re: pg_upgrade's interaction with pg_resetwal seems confusing

On Fri, 13 Oct 2023 at 17:15, Hayato Kuroda (Fujitsu)
<kuroda(dot)hayato(at)fujitsu(dot)com> wrote:
>
> Dear hackers,
>
> > >
> > > > > I mean instead of resetwal directly modifying the control file we
> > > > > will modify that value in the server using the binary_upgrade function
> > > > > and then have that value flush to the disk by shutdown checkpoint.
> > > > >
> > > >
> > > > True, the alternative to consider is to let pg_upgrade update the
> > > > control file by itself with the required value of OID. The point I am
> > > > slightly worried about doing via server-side function is that some
> > > > online and or shutdown checkpoint can update other values in the
> > > > control file as well whereas if we do via pg_upgrade, we can have
> > > > better control over just updating the OID.
> > >
> > > Yeah, thats a valid point.
> > >
> >
> > But OTOH, just updating the control file via pg_upgrade may not be
> > sufficient because we should keep the shutdown checkpoint record also
> > updated with that value. See how pg_resetwal achieves it via
> > RewriteControlFile() and WriteEmptyXLOG(). So, considering both the
> > approaches it seems better to go with a server-side function as Robert
> > suggested.
>
> Based on these discussion, I implemented a server-side approach. An attached patch
> adds a upgrade function which sets ShmemVariableCache->nextOid. It is called at
> the end of the upgrade. Comments and name of issue_warnings_and_set_wal_level()
> is also updated because they become outdated.

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?

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)
{

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'.

Regards,
Vignesh