Re: [Patch] ALTER SYSTEM READ ONLY

On Fri, Feb 19, 2021 at 5:43 PM Amul Sul <sulamul@gmail.com> wrote:
>
> In the attached version I have made the changes accordingly what Robert has
> summarised in his previous mail[1].
>
> In addition to that, I also move the code that updates the control file to
> XLogAcceptWrites() which will also get skipped when the system is read-only (wal
> prohibited).  The system will be in the crash recovery, and that will
> change once we do the end-of-recovery checkpoint and the WAL writes operation
> which we were skipping from startup.  The benefit of keeping the system in
> recovery mode is that it fixes my concern[2] where other backends could connect
> and write wal records while we were changing the system to read-write. Now, no
> other backends allow a wal write; UpdateFullPageWrites(), end-of-recovery
> checkpoint, and XLogReportParameters() operations will be performed in the same
> sequence as it is in the startup while changing the system to read-write.

I was looking into the changes espcially recovery related problem,  I
have a few questions

1.
+static bool
+XLogAcceptWrites(bool needChkpt, bool bgwriterLaunched,
+                 bool localPromoteIsTriggered, XLogReaderState *xlogreader,
+                 bool archiveRecoveryRequested, TimeLineID endOfLogTLI,
+                 XLogRecPtr endOfLog, TimeLineID thisTimeLineID)
+{
+    bool        promoted = false;
+
+    /*
.....
+            if (localPromoteIsTriggered)
             {
-                checkPointLoc = ControlFile->checkPoint;
+                XLogRecord *record;

...
+                record = ReadCheckpointRecord(xlogreader,
+                                              ControlFile->checkPoint,
+                                              1, false);
                 if (record != NULL)
                 {
                     promoted = true;
                    ...
                    CreateEndOfRecoveryRecord();
                }

Why do we need to move promote related code in XLogAcceptWrites?
IMHO, this promote related handling should be in StartupXLOG only.
That will look cleaner.

>
> 1] http://postgr.es/m/CA+TgmoZ=CCTbAXxMTYZoGXEgqzOz9smkBWrDpsacpjvFcGCuaw@mail.gmail.com
> 2] http://postgr.es/m/CAAJ_b97xX-nqRyM_uXzecpH9aSgoMROrDNhrg1N51fDCDwoy2g@mail.gmail.com

2.
I did not clearly understand your concern in point [2], because of
which you have to postpone RECOVERY_STATE_DONE untill system is set
back to read-write.  Can you explain this?


-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com