Thread: Slight refactoring of state check in pg_upgrade check_ function

Slight refactoring of state check in pg_upgrade check_ function

From
Daniel Gustafsson
Date:
I noticed that the pg_upgrade check_ functions were determining failures found
in a few different ways.  Some keep a boolen flag variable, and some (like
check_for_incompatible_polymorphics) check the state of the script filehandle
which is guaranteed to be set (with the error message referring to the path of
said file).  Others like check_loadable_libraries only check the flag variable
and fclose the handle assuming it was opened.

The attached diff changes the functions to do it consistently in one way, by
checking the state of the filehandle.  Since we are referring to the file by
path in the printed error message it seemed the cleanest approach, and it saves
a few lines of code without IMO reducing readability.

There is no change in functionality, just code consistency.

--
Daniel Gustafsson        https://vmware.com/


Attachment

Re: Slight refactoring of state check in pg_upgrade check_ function

From
Nathan Bossart
Date:
On Sun, Aug 28, 2022 at 10:42:24PM +0200, Daniel Gustafsson wrote:
> I noticed that the pg_upgrade check_ functions were determining failures found
> in a few different ways.  Some keep a boolen flag variable, and some (like
> check_for_incompatible_polymorphics) check the state of the script filehandle
> which is guaranteed to be set (with the error message referring to the path of
> said file).  Others like check_loadable_libraries only check the flag variable
> and fclose the handle assuming it was opened.
> 
> The attached diff changes the functions to do it consistently in one way, by
> checking the state of the filehandle.  Since we are referring to the file by
> path in the printed error message it seemed the cleanest approach, and it saves
> a few lines of code without IMO reducing readability.
> 
> There is no change in functionality, just code consistency.

The patch looks reasonable to me.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com



Re: Slight refactoring of state check in pg_upgrade check_ function

From
Bruce Momjian
Date:
On Sun, Aug 28, 2022 at 03:06:09PM -0700, Nathan Bossart wrote:
> On Sun, Aug 28, 2022 at 10:42:24PM +0200, Daniel Gustafsson wrote:
> > I noticed that the pg_upgrade check_ functions were determining failures found
> > in a few different ways.  Some keep a boolen flag variable, and some (like
> > check_for_incompatible_polymorphics) check the state of the script filehandle
> > which is guaranteed to be set (with the error message referring to the path of
> > said file).  Others like check_loadable_libraries only check the flag variable
> > and fclose the handle assuming it was opened.
> > 
> > The attached diff changes the functions to do it consistently in one way, by
> > checking the state of the filehandle.  Since we are referring to the file by
> > path in the printed error message it seemed the cleanest approach, and it saves
> > a few lines of code without IMO reducing readability.
> > 
> > There is no change in functionality, just code consistency.
> 
> The patch looks reasonable to me.

+1.  Those checks have accumulated over time with different authors,
hence the stylistic differences.

-- 
  Bruce Momjian  <bruce@momjian.us>        https://momjian.us
  EDB                                      https://enterprisedb.com

  Indecision is a decision.  Inaction is an action.  Mark Batterson




Re: Slight refactoring of state check in pg_upgrade check_ function

From
Daniel Gustafsson
Date:
> On 30 Aug 2022, at 23:08, Bruce Momjian <bruce@momjian.us> wrote:
> On Sun, Aug 28, 2022 at 03:06:09PM -0700, Nathan Bossart wrote:

>> The patch looks reasonable to me.
> 
> +1. Those checks have accumulated over time with different authors,
> hence the stylistic differences.

Pushed, thanks for review!

--
Daniel Gustafsson        https://vmware.com/