Re: [Patch] ALTER SYSTEM READ ONLY

On Sun, May 9, 2021 at 1:26 AM Amul Sul <sulamul@gmail.com> wrote:
> The state in the control file also gets cleared. Though, after
> clearing in memory the state patch doesn't really do the immediate
> change to the control file, it relies on the next UpdateControlFile()
> to do that.

But when will that happen? If you're relying on some very nearby code,
that might be OK, but perhaps a comment is in order. If you're just
thinking it's going to happen eventually, I think that's not good
enough.

> Regarding log message I think I have skipped that intentionally, to
> avoid confusing log as "system is now read write" when we do start as
> hot-standby which is not really read-write.

I think the message should not be phrased that way. In fact, I think
now that we've moved to calling this pg_prohibit_wal() rather than
ALTER SYSTEM READ ONLY, a lot of messages need to be rethought, and
maybe some comments and function names as well. Perhaps something
like:

system is read only -> WAL is now prohibited
system is read write -> WAL is no longer prohibited

And then for this particular case, maybe something like:

clearing WAL prohibition because the system is in archive recovery

> > The second part of this proposal was:
> >
> > "2. Create a new function with a name like XLogAcceptWrites(). Move the
> > following things from StartupXLOG() into that function: (1) the call
> > to UpdateFullPageWrites(), (2) the following block of code that does
> > either CreateEndOfRecoveryRecord() or RequestCheckpoint() or
> > CreateCheckPoint(), (3) the next block of code that runs
> > recovery_end_command, (4) the call to XLogReportParameters(), and (5)
> > the call to CompleteCommitTsInitialization(). Call the new function
> > from the place where we now call XLogReportParameters(). This would
> > mean that (1)-(3) happen later than they do now, which might require
> > some adjustments."
> >
> > Now you moved that code, but you also moved (6)
> > CompleteCommitTsInitialization(), (7) setting the control file to
> > DB_IN_PRODUCTION, (8) setting the state to RECOVERY_STATE_DONE, and
> > (9) requesting a checkpoint if we were just promoted. That's not what
> > was proposed. One result of this is that the server now thinks it's in
> > recovery even after the startup process has exited.
> > RecoveryInProgress() is still returning true everywhere. But that is
> > inconsistent with what Andres and I were recommending in
> > http://postgr.es/m/CA+TgmoZYQN=rcYE-iXWnjdvMAoH+7Jaqsif3U2k8xqXipBaS7A@mail.gmail.com
>
> Regarding modified approach, I tried to explain that why I did
> this in http://postgr.es/m/CAAJ_b96Yb4jaW6oU1bVYEBaf=TQ-QL+mMT1ExfwvNZEr7XRyoQ@mail.gmail.com

I am not able to understand what problem you are seeing there. If
we're in crash recovery, then nobody can connect to the database, so
there can't be any concurrent activity. If we're in archive recovery,
we now clear the WAL-is-prohibited flag so that we will go read-write
directly at the end of recovery. We can and should refuse any effort
to call pg_prohibit_wal() during recovery. If we reached the end of
crash recovery and are now permitting read-only connections, why would
anyone be able to write WAL before the system has been changed to
read-write? If that can happen, it's a bug, not a reason to change the
design.

Maybe your concern here is about ordering: the process that is going
to run XLogAcceptWrites() needs to allow xlog writes locally before we
tell other backends that they also can xlog writes; otherwise, some
other records could slip in before UpdateFullPageWrites() and similar
have run, which we probably don't want. But that's why
LocalSetXLogInsertAllowed() was invented, and if it doesn't quite do
what we need in this situation, we should be able to tweak it so it
does.

If your concern is something else, can you spell it out for me again
because I'm not getting it?

-- 
Robert Haas
EDB: http://www.enterprisedb.com