On 9/19/21, 11:07 PM, "Amul Sul" <sulamul@gmail.com> wrote:
> +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.
I see that the attached patch reorders the call to UpdateControlFile()
to before SharedRecoveryState is updated, which seems to go against
the intent of ebdf5bf. I'm not sure if this really creates that much
of a problem in practice, but it is a behavior change.
Also, I still think it might be better to include this patch in the
patch set where the exported function is needed. On its own, this is
a very small amount of refactoring that might not be totally
necessary.
> 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?
I see a few places where UpdateControlFile() is called without
updating ControlFile->time. I haven't found any obvious reason for
that, so perhaps it would be okay to move it to update_controlfile().
Nathan