On Wed, Sep 15, 2021 at 12:52 AM Bossart, Nathan <bossartn@amazon.com> wrote:
>
> On 9/13/21, 11:06 PM, "Amul Sul" <sulamul@gmail.com> wrote:
> > The patch is straightforward but the only concern is that in
> > StartupXLOG(), SharedRecoveryState now gets updated only with spin
> > lock; earlier it also had ControlFileLock in addition to that. AFAICU,
> > I don't see any problem there, since until the startup process exists
> > other backends could not connect and write a WAL record.
>
> 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.
> 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.
Regards,
Amul
1] https://postgr.es/m/CAAJ_b97KZzdJsffwRK7w0XU5HnXkcgKgTR69t8cOZztsyXjkQw@mail.gmail.com