Re: Deduplicate code updating ControleFile's DBState.

On Thu, Sep 16, 2021 at 4:19 AM Bossart, Nathan <bossartn(at)amazon(dot)com> wrote:
>
> On 9/15/21, 4:47 AM, "Amul Sul" <sulamul(at)gmail(dot)com> wrote:
> > On Wed, Sep 15, 2021 at 12:52 AM Bossart, Nathan <bossartn(at)amazon(dot)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();
> }
>

+1, since skipping ControlFileLock for the DBState update is not the
right thing, let's have two different functions as per your suggestion
-- did the same in the attached version, thanks.

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

Ok, reverted those changes in the attached version.

I have one additional concern about the way we update the control
file, at every place where doing the update, we need to set control
file update time explicitly, why can't the time update line be moved
to UpdateControlFile() so that time gets automatically updated?

Regards,
Amul