On Mon, Apr 12, 2021 at 10:04 AM Amul Sul <sula...@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=cctbaxxmtyzogxegqzoz9smkbwrdpsacpjvfcgc...@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+7jaqsif3u2k8xqxipba...@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
