Re: Add a new BGWORKER_BYPASS_ROLELOGINCHECK flag - Mailing list pgsql-hackers

From Drouvot, Bertrand
Subject Re: Add a new BGWORKER_BYPASS_ROLELOGINCHECK flag
Date
Msg-id 26e7cd6a-6dd1-4b08-975b-62603eeb6f3f@gmail.com
Whole thread Raw
In response to Re: Add a new BGWORKER_BYPASS_ROLELOGINCHECK flag  (Michael Paquier <michael@paquier.xyz>)
Responses Re: Add a new BGWORKER_BYPASS_ROLELOGINCHECK flag
List pgsql-hackers
Hi,

On 10/5/23 7:10 AM, Michael Paquier wrote:
> On Wed, Oct 04, 2023 at 12:54:24PM +0200, Drouvot, Bertrand wrote:
>> Except the Nit that I mentioned in 0001, that looks all good to me (with the
>> new wait in 001_worker_spi.pl).
> 
> Thanks, I've applied the refactoring, including in it the stuff to be
> able to control the flags used when launching a dynamic worker.  0001
> also needed some indenting.  You'll notice that the diffs in
> worker_spi are minimal now.  worker_spi_main is no more, as an effect
> of..  Cough..  c8e318b1b.

Thanks!

> +# An error message should be issued.
> +my $nb_errors = 0;
> +for (my $i = 0; $i < 10 * $PostgreSQL::Test::Utils::timeout_default; $i++)
> +{
> +    if ($node->log_contains(
> +            "role \"nologrole\" is not permitted to log in", $log_start))
> +    {
> +        $nb_errors = 1;
> +        last;
> +    }
> +    usleep(100_000);
> +}
> 
> This can be switched to use $node->wait_for_log, making the test
> simpler.  No need for Time::HiRes, either.
> 

Oh, thanks, did not know about $node->wait_for_log, good to know!

> -extern void BackgroundWorkerInitializeConnection(const char *dbname,
> const char *username, uint32 flags);
> +extern void BackgroundWorkerInitializeConnection(const char *dbname,
> const char *username, bits32 flags);
> [...]
> -BackgroundWorkerInitializeConnectionByOid(Oid dboid, Oid useroid,
> uint32 flags)
> +BackgroundWorkerInitializeConnectionByOid(Oid dboid, Oid useroid,
> bits32 flags)
> 
> That's changing signatures just for the sake of breaking them.  I
> would leave that alone, I guess..

Ok, switched back to uint32 in v6-0002 attached (v6-0001 is your v5-0001
unchanged).

> 
> @@ -719,6 +719,7 @@ InitPostgres(const char *in_dbname, Oid dboid,
>                const char *username, Oid useroid,
>                bool load_session_libraries,
>                bool override_allow_connections,
> +             bool bypass_login_check,
>                char *out_dbname)
> 
> I was not paying much attention here, but load_session_libraries gives
> a good argument in favor of switching all these booleans to a single
> bits32 argument as that would make three of them now, with a different
> set of flags than the bgworker ones.  This can be refactored on its
> own.

Yeah good point, will work on it once the current one is committed.

> 
> -#define BGWORKER_BYPASS_ALLOWCONN 1
> +#define BGWORKER_BYPASS_ALLOWCONN 0x0001
> 
> Can be a change of its own as well.

Yeah, but I think it's simple enough to just keep this change here
(and I don't think it's "really" needed without introducing 0x0002)

> 
> While looking at the refactoring at worker_spi.  I've noticed that it
> would be simple and cheap to have some coverage for BYPASS_ALLOWCONN,
> something we've never done since this has been introduced.  Let me
> suggest 0001 to add some coverage.

Good idea! I looked at 0001 and it looks ok to me.

> 
> 0002 is your own patch, with the tests simplified a bit.

Thanks, LGTM.

Regards,

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

pgsql-hackers by date:

Previous
From: David Rowley
Date:
Subject: Re: pg16: XX000: could not find pathkey item to sort
Next
From: Andy Fan
Date:
Subject: Re: Extract numeric filed in JSONB more effectively