Date: Thu, 3 Sep 2020 10:50:49 -0400
From: Alvaro Herrera <alvherre@2ndquadrant.com>
To: Michael Paquier <michael@paquier.xyz>
Cc: Daniel Gustafsson <daniel@yesql.se>,
	Robert Haas <robertmhaas@gmail.com>,
	Andres Freund <andres@anarazel.de>,
	Postgres hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: Switch to multi-inserts for pg_depend
Message-ID: <20200903145049.GA14188@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20200903053506.GC14963@paquier.xyz>
User-Agent: Mutt/1.10.1 (2018-07-13)
Precedence: bulk

I agree, this version looks much better, thanks.  Two very minor things:

On 2020-Sep-03, Michael Paquier wrote:

> @@ -76,11 +77,23 @@ recordMultipleDependencies(const ObjectAddress *depender,
>  
>  	dependDesc = table_open(DependRelationId, RowExclusiveLock);
>  
> +	/*
> +	 * Allocate the slots to use, but delay initialization until we know that
> +	 * they will be used.  The slot initialization is the costly part, and the
> +	 * exact number of dependencies inserted cannot be known in advance as it
> +	 * depends on what is pinned by the system.
> +	 */

I'm not sure you need the second sentence in this comment; keeping the
"delay initialization until ..." part seems sufficient.  If you really
want to highlight that initialization is costly, maybe just say "delay
costly initialization".

> +		/*
> +		 * Record the Dependency.  Note we don't bother to check for duplicate
> +		 * dependencies; there's no harm in them.
> +		 */

No need to uppercase "dependency".  (I know this is carried forward from
prior comment, but it was equally unnecessary there.)

>  	/*
>  	 * Allocate the slots to use, but delay initialization until we know that
> -	 * they will be used.
> +	 * they will be used.  A full scan of pg_shdepend is done to find all the
> +	 * dependencies from the template database to copy.  Their number is not
> +	 * known in advance and the slot initialization is the costly part.
>  	 */

As above, this change is not needed.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services