Re: Deduplicate code updating ControleFile's DBState.

On Tue, Sep 21, 2021 at 9:43 PM Bossart, Nathan <bossartn(at)amazon(dot)com> wrote:
>
> On 9/20/21, 10:07 PM, "Amul Sul" <sulamul(at)gmail(dot)com> wrote:
> > 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:
> >> > 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.
>
> void
> UpdateControlFile(void)
> {
> + ControlFile->time = (pg_time_t) time(NULL);
> update_controlfile(DataDir, ControlFile, true);
> }
>
> Shouldn't we update the time in update_controlfile()?

If you see the callers of update_controlfile() except for
RewriteControlFile() no one else updates the timestamp before calling
it, I am not sure if that is intentional or not. That was the one
reason that was added in UpdateControlFile(). And another reason is
that if you look at all the deleting lines followed by
UpdateControlFile() & moving that to UpdateControlFile() wouldn't
change anything drastically.

IMO, anything going to change should update the timestamp as well,
that could be a bug then.

> Also, can you
> split this change into two patches (i.e., one for the timestamp change
> and another for the refactoring)? Otherwise, this looks reasonable to
> me.

Done, thanks for the review.

Regards,
Amul