Horton

Re: [Patch] ALTER SYSTEM READ ONLY

Robert Haas <robertmhaas@gmail.com>

View thread View on postgr.es
From: Robert Haas <robertmhaas@gmail.com>
To: Amul Sul <sulamul@gmail.com>
Cc: Dilip Kumar <dilipbalaut@gmail.com>, Andres Freund <andres@anarazel.de>, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>, Ibrar Ahmed <ibrar.ahmad@gmail.com>, PostgreSQL-development <pgsql-hackers@postgresql.org>, Prabhat Sahu <prabhat.sahu@enterprisedb.com>
Date: 2021-09-15T14:32:10Z
Lists: pgsql-hackers 
On Wed, Sep 15, 2021 at 6:49 AM Amul Sul <sulamul@gmail.com> wrote:
>  Initially, I thought to
> use SharedRecoveryState which is always set to RECOVERY_STATE_ARCHIVE,
> if  the archive recovery requested. But there is another case where
> SharedRecoveryState could be RECOVERY_STATE_ARCHIVE irrespective of
> ArchiveRecoveryRequested value, that is the presence of a backup label
> file.

Right, there's a difference between whether archive recovery has been
*requested* and whether it is actually *happening*.

> If we want to use SharedRecoveryState, we need one more state
> which could differentiate between ArchiveRecoveryRequested and the
> backup label file presence case.  To move ahead, I have copied
> ArchiveRecoveryRequested into shared memory and it will be cleared
> once archive cleanup is finished. With all these changes, we could get
> rid of xlogaction argument and DetermineRecoveryXlogAction() function.
> Could move its logic to PerformRecoveryXLogAction() directly.

Putting these changes into 0001 seems to make no sense. It seems like
they should be part of 0003, or maybe a new 0004 patch.

> And for ThisTimeLineID, I don't think we need to do anything since this
> value is available with all the backend as per the following comment:
> "
> /*
>  * ThisTimeLineID will be same in all backends --- it identifies current
>  * WAL timeline for the database system.
>  */
> TimeLineID  ThisTimeLineID = 0;

I'm not sure I find that argument totally convincing. The two
variables aren't assigned at exactly the same places in the code,
nonwithstanding the comment. I'm not saying you're wrong. I'm just
saying I don't believe it just because the comment says so.

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

Commits

  1. Initialize variable to placate compiler.

  2. StartupXLOG: Don't repeatedly disable/enable local xlog insertion.

  3. StartupXLOG: Call CleanupAfterArchiveRecovery after XLogReportParameters.

  4. Postpone some end-of-recovery operations related to allowing WAL.

  5. Refactor some end-of-recovery code out of StartupXLOG().

  6. Re-enable contrib/bloom's TAP tests.

  7. Remove unnecessary call to ReadCheckpointRecord().

  8. Allow for error or refusal while absorbing a ProcSignalBarrier.

  9. Add comment to explain an unused function parameter

  10. Extend the ProcSignal mechanism to support barriers.

  11. At promotion, don't leave behind a partial segment on the old timeline.