Re: Deduplicate code updating ControleFile's DBState.

On 9/15/21, 4:47 AM, "Amul Sul" <sulamul@gmail.com> wrote:
> On Wed, Sep 15, 2021 at 12:52 AM Bossart, Nathan <bossartn@amazon.com> wrote:
>> It looks like ebdf5bf intentionally made sure that we hold
>> ControlFileLock while updating SharedRecoveryInProgress (now
>> SharedRecoveryState after 4e87c48).  The thread for this change [0]
>> has some additional details.
>>
>
> Yeah, I saw that and ebdf5bf main intention was to minimize the gap
> between both of them which was quite big previously.  The comments
> added by the same commit also describe the case that backends can
> write WAL and the control file is still referring not in
> DB_IN_PRODUCTION and IIUC, which seems to be acceptable.
> Then the question is what would be wrong if a process can see an
> inconsistent shared memory view for a small window?  Might be
> wait-promoting might behave unexpectedly, that I have to test.

For your proposed change, I would either leave out this particular
call site or add a "WithLock" version of the function.

void
SetControlFileDBState(DBState state)
{
        LWLockAcquire(ControlFileLock, LW_EXCLUSIVE);
        SetControlFileDBStateWithLock(state);
        LWLockRelease(ControlFileLock);
}

void
SetControlFileDBStateWithLock(DBState state)
{
        Assert(LWLockHeldByMeInMode(ControlFileLock, LW_EXCLUSIVE));

        ControlFile->state = state;
        ControlFile->time = (pg_time_t) time(NULL);
        UpdateControlFile();
}

>> As far as the patch goes, I'm not sure why SetControlFileDBState()
>> needs to be exported, and TBH I don't know if this change is really a
>> worthwhile improvement.  ISTM the main benefit is that it could help
>> avoid cases where we update the state but not the time.  However,
>> there's still nothing preventing that, and I don't get the idea that
>> it was really a big problem to begin with.
>>
>
> Oh ok, I was working on a different patch[1] where I want to call this
> function from checkpointer, but I agree exporting function is not in
> the scope of this patch.

Ah, I was missing this context.  Perhaps this should be included in
the patch set for the other thread, especially if it will need to be
exported.

Nathan