Re: Add a new BGWORKER_BYPASS_ROLELOGINCHECK flag

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.

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

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

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

-#define BGWORKER_BYPASS_ALLOWCONN 1
+#define BGWORKER_BYPASS_ALLOWCONN 0x0001

Can be a change of its own as well.

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.

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