Date: Mon, 10 Jul 2023 16:09:07 -0700
From: Nathan Bossart <nathandbossart@gmail.com>
To: Daniel Gustafsson <daniel@yesql.se>
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: Reducing connection overhead in pg_upgrade compat check phase
Message-ID: <20230710230907.GA481155@nathanxps13>
References: <20230218054613.GA3360104@nathanxps13>
 <542946CF-0D77-446E-9FA6-028982175320@yesql.se>
 <20230222192006.GA79448@nathanxps13>
 <235E7E38-6DCA-4E5F-812C-7F3033FC800A@yesql.se>
 <89A64E5D-D3DC-4806-B038-CD493F5F5510@yesql.se>
 <20230313182147.GA294021@nathanxps13>
 <20230704190839.GA3468243@nathanxps13>
 <9C9D8A09-9794-46DD-8074-8961924B99F1@yesql.se>
 <20230708214354.GA4044380@nathanxps13>
 <A31FC972-FCBE-47CE-85F0-182B58C29422@yesql.se>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <A31FC972-FCBE-47CE-85F0-182B58C29422@yesql.se>
Archived-At: <https://www.postgresql.org/message-id/20230710230907.GA481155%40nathanxps13>
Precedence: bulk

On Mon, Jul 10, 2023 at 04:43:23PM +0200, Daniel Gustafsson wrote:
>> On 8 Jul 2023, at 23:43, Nathan Bossart <nathandbossart@gmail.com> wrote:
>> Since "script" will be NULL and "db_used" will be false in the first
>> iteration of the loop, couldn't we move this stuff to before the loop?
> 
> We could.  It's done this way to match how all the other checks are performing
> the inner loop for consistency.  I think being consistent is better than micro
> optimizing in non-hot codepaths even though it adds some redundancy.
>
> [ ... ] 
> 
>> Won't "script" always be initialized here?  If I'm following this code
>> correctly, I think everything except the fclose() can be removed.
> 
> You are right that this check is superfluous.  This is again an artifact of
> modelling the code around how the other checks work for consistency.  At least
> I think that's a good characteristic of the code.

I can't say I agree with this, but I'm not going to hold up the patch over
it.  FWIW I was looking at this more from a code simplification/readability
standpoint.

>> +static int	n_data_types_usage_checks = 7;
>> 
>> Can we determine this programmatically so that folks don't need to remember
>> to update it?
> 
> Fair point, I've added a counter loop to the beginning of the check function to
> calculate it.

+	/* Gather number of checks to perform */
+	while (tmp->status != NULL)
+		n_data_types_usage_checks++;

I think we need to tmp++ somewhere here.

>> In fact, I wonder if we could just add the versions directly to
>> data_types_usage_checks so that we don't need the separate hook functions.
> 
> We could, but it would be sort of contrived I think since some check <= and
> some == while some check the catversion as well (and new ones may have other
> variants.  I think this is the least paint-ourselves-in-a-corner version, if we
> feel it's needlessly complicated and no other variants are added we can revisit
> this.

Makes sense.

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