Re: Add a new BGWORKER_BYPASS_ROLELOGINCHECK flag

Hi,

On 10/3/23 11:21 AM, Bharath Rupireddy wrote:
> On Mon, Oct 2, 2023 at 4:58 PM Drouvot, Bertrand
> <bertranddrouvot(dot)pg(at)gmail(dot)com> wrote:
>>
>> On 9/29/23 8:19 AM, Michael Paquier wrote:
>>> On Thu, Sep 28, 2023 at 02:37:02PM +0200, Drouvot, Bertrand wrote:
>>>> This patch allows the role provided in BackgroundWorkerInitializeConnection()
>>>> and BackgroundWorkerInitializeConnectionByOid() to lack login authorization.
>>>
>>> Interesting. Yes, there would be use cases for that, I suppose.
>
> Correct. It allows the roles that don't have LOGIN capabilities to
> start and use bg workers.
>
>>> This may be more adapted with a bits32 for the flags.
>>
>> Done that way in v2 attached.
>
> While I like the idea of the flag to skip login checks for bg workers,
> I don't quite like the APIs being changes InitializeSessionUserId and
> InitPostgres (adding a new input parameter),
> BackgroundWorkerInitializeConnection and
> BackgroundWorkerInitializeConnectionByOid (changing of input parameter
> type) given that all of these functions are available for external
> modules and will break things for sure.
>
> What if BGWORKER_BYPASS_ROLELOGINCHECK be part of bgw_flags? With
> this, none of the API needs to be changed, so no compatibility
> problems as such for external modules and the InitializeSessionUserId
> can just do something like [1]. We might be tempted to add
> BGWORKER_BYPASS_ALLOWCONN also to bgw_flags, but I'd prefer not to do
> it for the same compatibility reasons.
>
> Thoughts?
>

Thanks for looking at it!

I did some research and BGWORKER_BYPASS_ALLOWCONN has been added in eed1ce72e1 and
at that time the bgw_flags did already exist.

In this the related thread [1], Tom mentioned:

"
We change exported APIs in new major versions all the time. As
long as it's just a question of an added parameter, people can deal
with it.
"

And I agree with that.

Now, I understand your point but it looks to me that bgw_flags is more
about the capabilities (Access to shared memory with BGWORKER_SHMEM_ACCESS
or ability to establish database connection with BGWORKER_BACKEND_DATABASE_CONNECTION),

While with BGWORKER_BYPASS_ROLELOGINCHECK (and BGWORKER_BYPASS_ALLOWCONN) it's more related to
the BGW behavior once the capability is in place.

So, I think I'm fine with the current proposal and don't see the need to move
BGWORKER_BYPASS_ROLELOGINCHECK in bgw_flags.

[1]: https://www.postgresql.org/message-id/22769.1519323861%40sss.pgh.pa.us

Regards,

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