Message-ID: <73de543e-712b-4468-8ffa-74a686235597@gmail.com>
Date: Tue, 3 Oct 2023 14:15:48 +0200
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: Add a new BGWORKER_BYPASS_ROLELOGINCHECK flag
To: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Cc: Michael Paquier <michael@paquier.xyz>,
 PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
References: <bcc36259-7850-4882-97ef-d6b905d2fc51@gmail.com>
 <ZRZsg2pqrCQ9dlsn@paquier.xyz>
 <eea89922-70e2-4a50-b990-4c6a9757a840@gmail.com>
 <CALj2ACWZ1a69K9GMsUpCY+kyMqqeHMWVu+UUAe7=gBCkk1Xp8Q@mail.gmail.com>
Content-Language: en-US
From: "Drouvot, Bertrand" <bertranddrouvot.pg@gmail.com>
In-Reply-To: <CALj2ACWZ1a69K9GMsUpCY+kyMqqeHMWVu+UUAe7=gBCkk1Xp8Q@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Archived-At: <https://www.postgresql.org/message-id/73de543e-712b-4468-8ffa-74a686235597%40gmail.com>
Precedence: bulk

Hi,

On 10/3/23 11:21 AM, Bharath Rupireddy wrote:
> On Mon, Oct 2, 2023 at 4:58 PM Drouvot, Bertrand
> <bertranddrouvot.pg@gmail.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