Re: pg_upgrade's interaction with pg_resetwal seems confusing

On Fri, Oct 13, 2023 at 12:00 AM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>
> On Thu, Oct 12, 2023 at 7:17 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> > Now, as mentioned in the first paragraph, it seems we anyway don't
> > need to reset the WAL at the end when setting the next OID for the new
> > cluster with the -o option. If that is true, then I think even without
> > slots work it will be helpful to have such an option in pg_resetwal.
> >
> > Thoughts?
>
> I wonder if we should instead provide a way to reset the OID counter
> with a function call inside the database, gated by IsBinaryUpgrade.
>

I think the challenge in doing so would be that when the server is
running, a concurrent checkpoint can also update the OID counter value
in the control file. See below code:

CreateCheckPoint()
{
...
LWLockAcquire(OidGenLock, LW_SHARED);
checkPoint.nextOid = ShmemVariableCache->nextOid;
if (!shutdown)
checkPoint.nextOid += ShmemVariableCache->oidCount;
LWLockRelease(OidGenLock);
...
UpdateControlFile()
...
}

Now, we can try to pass some startup options like checkpoint_timeout
with a large value to ensure that checkpoint won't interfere but not
sure if that would be bulletproof. Instead, how about allowing
pg_upgrade to update the control file of the new cluster (with the
required value of OID) following the same method as pg_resetwal does
in RewriteControlFile()?

> Having something like pg_resetwal --but-dont-actually-reset-the-wal
> seems both self-contradictory and vulnerable to abuse that we might be
> better off not inviting.
>

Fair point.

--
With Regards,
Amit Kapila.