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
