On Mon, Oct 02, 2023 at 10:53:22AM +0200, Drouvot, Bertrand wrote:
> On 10/2/23 10:17 AM, Michael Paquier wrote:
>> On Mon, Oct 02, 2023 at 10:01:04AM +0200, Drouvot, Bertrand wrote:
>>> I think that would make sense to have more flexibility in the worker_spi
>>> module. I think that could be done in a dedicated patch though. I
>>> think it makes more sense to have the current patch "focusing" on
>>> this new flag (while adding a test about it without too much
>>> refactoring). What about doing the worker_spi module re-factoring
>>> as a follow up of this one?
>>
>> I would do that first, as that's what I usually do,
>
> The reason I was thinking not doing that first is that there is no real use
> case in the current worker_spi module test.
As a template, improving and extending it seems worth it to me as long
as it can also improve tests.
> > but I see also
> > your point that this is not mandatory. If you want, I could give it a
> > shot tomorrow to see where it leads.
>
> Oh yeah that would be great (and maybe you already see a use case in the
> current test). Thanks!
Yes, while it shows a bit more what one can achieve with bgw_extra, it
also helps in improving the test coverage with starting dynamic
workers across defined databases and roles through a SQL function.
It took me a bit longer than I expected, but here is what I finish
with:
- 0001 extends worker_spi to be able to pass down database and role
IDs for the workers to start, through MyBgworkerEntry->bgw_extra.
Perhaps the two new arguments of worker_spi_launch() should use
InvalidOid as default, actually, to fall back to the database and
roles defined by the GUCs in these cases. That would be two lines to
change in worker_spi--1.0.sql.
- 0002 is your patch, on top of which I have added handling for the
flags in the launch() function with a text[]. The tests get much
simpler, and don't need restarts.
By the way, I am pretty sure that we are going to need a wait phase
after the startup of the worker in the case where "nologrole" is not
allowed to log in even with the original patch: the worker may not
have started at the point where we check the logs. That's true as
well when involving worker_spi_launch().
What do you think?
--
Michael