Re: [Patch] ALTER SYSTEM READ ONLY

On Mon, Apr 12, 2021 at 10:04 AM Amul Sul <sulamul(at)gmail(dot)com> wrote:
> Rebased again.

I started to look at this today, and didn't get very far, but I have a
few comments. The main one is that I don't think this patch implements
the design proposed in
https://www.postgresql.org/message-id/CA+TgmoZ=CCTbAXxMTYZoGXEgqzOz9smkBWrDpsacpjvFcGCuaw@mail.gmail.com

The first part of that proposal said this:

"1. If the server starts up and is read-only and
ArchiveRecoveryRequested, clear the read-only state in memory and also
in the control file, log a message saying that this has been done, and
proceed. This makes some other cases simpler to deal with."

As I read it, the patch clears the read-only state in memory, does not
clear it in the control file, and does not log a message.

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

I also noticed that 0001 does not compile without 0002, so the
separation into multiple patches is not clean. I would actually
suggest that the first patch in the series should just create
XLogAcceptWrites() with the minimum amount of adjustment to make that
work. That would potentially let us commit that change independently,
which would be good, because then if we accidentally break something,
it'll be easier to pin down to that particular change instead of being
mixed with everything else this needs to change.

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