Date: Mon, 9 Sep 2024 14:17:17 -0500
From: Nathan Bossart <nathandbossart@gmail.com>
To: Daniel Gustafsson <daniel@yesql.se>
Cc: Ilya Gladyshev <ilya.v.gladyshev@gmail.com>,
	Corey Huinker <corey.huinker@gmail.com>,
	Robert Haas <robertmhaas@gmail.com>, Jeff Davis <pgsql@j-davis.com>,
	PostgreSQL Hackers <pgsql-hackers@postgresql.org>
Subject: Re: optimizing pg_upgrade's once-in-each-database steps
Message-ID: <Zt9JvRyYcFBVhoAJ@nathan>
References: 
 <CADkLM=fc-wujxNtGpLSVrBAtWQgsW-8RYrMxVyQ1buJfQX9CHw@mail.gmail.com>
 <ZreEh54Xr6D3Qy8k@nathan>
 <ZreI0vpZoQfoocDq@nathan>
 <Zr4mycERiqJx09y_@nathan>
 <Zs-awIHJu_GGahdD@nathan>
 <3A3553B1-123D-4F11-9426-277F2E920AD7@gmail.com>
 <ZtTXEexwmlvsgsPs@nathan>
 <0fe740dd-cd23-45a6-b116-c777349acbf1@gmail.com>
 <Zte63Vz4E2P-VOwE@nathan>
 <3A4280AB-D57E-4F9E-B1DB-63C80C7E891F@yesql.se>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <3A4280AB-D57E-4F9E-B1DB-63C80C7E891F@yesql.se>
Archived-At: <https://www.postgresql.org/message-id/Zt9JvRyYcFBVhoAJ%40nathan>
Precedence: bulk

On Thu, Sep 05, 2024 at 01:32:34PM +0200, Daniel Gustafsson wrote:
> I've read and tested through the latest version of this patchset and I think
> it's ready to go in.

Thanks for reviewing.  I'm aiming to commit it later this week.

> The one concern I have is that tasks can exit(1) on libpq
> errors tearing down perfectly functional connections without graceful shutdown.
> Longer term I think it would make sense to add similar exit handler callbacks
> to the ones in pg_dump for graceful cleanup of connections.  However, in order
> to keep goalposts in clear view I don't think this patch need to have it, but
> it would be good to consider once in.

This did cross my mind.  I haven't noticed any problems in my testing, and
it looks like there are several existing places in pg_upgrade that call
pg_fatal() with open connections, so I'm inclined to agree that this is a
nice follow-up task that needn't hold up this patch set.

> Spotted a small typo in the comments:
> 
> +	 * nothing to process.  This is primarily intended for the inital step in
> s/inital/initial/

Will fix.

-- 
nathan