Re: Deduplicate code updating ControleFile's DBState.

On Tue, Sep 21, 2021 at 4:44 AM Bossart, Nathan <bossartn(at)amazon(dot)com> wrote:
>
> On 9/19/21, 11:07 PM, "Amul Sul" <sulamul(at)gmail(dot)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.
>

I had to have a thought on the same and didn't see any problem and
test suits also fine but that doesn't mean the change is perfect, the
issue might be hard to reproduce if there are any. Let's see what
others think and for now, to be safe I have reverted this 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.
>

Well, the other patch set is quite big and complex. In my experience,
usually, people avoid downloading big sets due to lack of time and
such small refactoring patches usually don't get much detailed
attention.

Also, even though this patch is small, it is independent and has
nothing to do with other patch set whether it gets committed or not.
Still, proposing some improvement might not be a big one but nice to
have.

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

Ok, thanks, did the same in the attached version.

Regards,
Amul Sul