Re: Add a new BGWORKER_BYPASS_ROLELOGINCHECK flag

Hi,

On 10/4/23 8:20 AM, Michael Paquier wrote:
> 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.
> 

Yeah right.

> It took me a bit longer than I expected, 

Thanks for having looked at it!

> 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. 

I'm fine with the way it's currently done in 0001 and that sounds
more logical to me. I mean we don't "really" want InvalidOid but to fall
back to the GUCs.

Just a remark here:

+       if (!OidIsValid(roleoid))
+       {
+               /*
+                * worker_spi_role is NULL by default, so just pass down an invalid
+                * OID to let the main() routine do its connection work.
+                */
+               if (worker_spi_role)
+                       roleoid = get_role_oid(worker_spi_role, false);
+               else
+                       roleoid = InvalidOid;

the

+               else
+                       roleoid = InvalidOid;

I think it is not needed as we're already in "!OidIsValid(roleoid)".

> - 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.
> 

Yeah, agree that's better.

> 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.

I agree and it's now failing on my side.
I added this "wait" in v4-0002 attach and it's now working fine.

Please note there is more changes than adding this wait in 001_worker_spi.pl (as compare
to v3-0002) as I ran pgperltidy on it.
FWIW, the new "wait" is just the part related to "nb_errors".

> What do you think?

Except the Nit that I mentioned in 0001, that looks all good to me (with the
new wait in 001_worker_spi.pl).

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com