Mime-Version: 1.0
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset=UTF-8; format=Flowed
Date: Fri, 01 Dec 2023 10:31:19 -0600
Message-Id: <CXD52ST7330T.1J5XRORQO5BD3@neon.tech>
Cc: "pgsql-hackers" <pgsql-hackers@postgresql.org>, "Thomas Munro"
 <thomas.munro@gmail.com>
Subject: Re: Refactoring backend fork+exec code
To: "Heikki Linnakangas" <hlinnaka@iki.fi>, "Andres Freund"
 <andres@anarazel.de>
From: "Tristan Partin" <tristan@neon.tech>
References: <7a59b073-5b5b-151e-7ed3-8b01ff7ce9ef@iki.fi>
 <20230710225043.svl7fqxecwshwc7a@awork3.anarazel.de>
 <119551e6-60f6-49b2-ae04-a382a89f900c@iki.fi>
 <e0dcfabb-f59a-45e8-bd92-f8d706c92128@iki.fi>
 <CXCDA9YFDZS1.Y5JUZBHLLXYR@neon.tech>
 <3b02a5a1-81ea-44e4-a09e-c0a594a4cef8@iki.fi>
In-Reply-To: <3b02a5a1-81ea-44e4-a09e-c0a594a4cef8@iki.fi>
Archived-At: 
 <https://www.postgresql.org/message-id/CXD52ST7330T.1J5XRORQO5BD3%40neon.tech>
Precedence: bulk

On Fri Dec 1, 2023 at 6:10 AM CST, Heikki Linnakangas wrote:
> On 30/11/2023 20:44, Tristan Partin wrote:
> > Patches 1-3 seem committable as-is.
>
> Thanks for the review! I'm focusing on patches 1-3 now, and will come=20
> back to the rest after committing 1-3.
>
> There was one test failure with EXEC_BACKEND from patch 2, in=20
> 'test_shm_mq'. In restore_backend_variables() I checked if 'bgw_name' is=
=20
> empty to decide if the BackgroundWorker struct is filled in or not, but=
=20
> it turns out that 'test_shm_mq' doesn't fill in bgw_name. It probably=20
> should, I think that's an oversight in 'test_shm_mq', but that's a=20
> separate issue.
>
> I did some more refactoring of patch 2, to fix that and to improve it in=
=20
> general. The BackgroundWorker struct is now passed through the=20
> fork-related functions similarly to the Port struct. That seems more=20
> consistent.
>
> Attached is new version of these patches. For easier review, I made the=
=20
> new refactorings compared in a new commit 0003. I will squash that=20
> before pushing, but this makes it easier to see what changed.
>
> Barring any new feedback or issues, I will commit these.

My only thought is that perhaps has_bg_worker is a better name than=20
has_worker, but I agree that having a flag is better than checking=20
bgw_name.

--=20
Tristan Partin
Neon (https://neon.tech)