Re: [Patch] ALTER SYSTEM READ ONLY

On Fri, May 7, 2021 at 1:23 AM Robert Haas <robertmhaas@gmail.com> wrote:
>
> On Mon, Apr 12, 2021 at 10:04 AM Amul Sul <sulamul@gmail.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 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.

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.

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

Ok, I will try in the next version.

Regards,
Amul