Re: Add a new BGWORKER_BYPASS_ROLELOGINCHECK flag

[Michael Paquier]

On Sun, Oct 15, 2023 at 10:47:22PM -0400, Tom Lane wrote:
> I agree that that probably is the root cause, and we should fix it
> by bumping up max_worker_processes in this test.

Thanks.  I've fixed this one now.  Let's see if mamba is OK after
that.

> If there's actually a defensible reason for the C code to act
> like that, then all the call sites need to have checks for
> a null result.

We're just talking about a test module and an ERROR in the same
fashion as autoprewarm makes things more predictible for the TAP
script, IMO.
--
Michael


signature.asc
Description: PGP signature

Re: Add a new BGWORKER_BYPASS_ROLELOGINCHECK flag

[Tom Lane]

Michael Paquier  writes:
> The buildfarm has provided some feedback, and the new tests have been
> unstable on mamba:
> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=mamba=2023-10-15%2005%3A04%3A21

Yeah.  FWIW, I tried to reproduce this on mamba's host, but did not
see it again in 100 or so tries.

> In more details:
> # poll_query_until timed out executing this query:
> # SELECT datname, usename, wait_event FROM pg_stat_activity
> # WHERE backend_type = 'worker_spi dynamic' AND
> # pid = ;
> # with stderr:
> # ERROR:  syntax error at or near ";"
> # LINE 3: pid = ;

> So this looks like a hard failure in starting the worker that should
> bypass the role login check.  The logs don't offer much information,
> but I think I know what's going on here: at this stage of the tests,
> the number of workers created is 7, very close to the limit of
> max_worker_processes, at 8 by default.  So one parallel worker spawned
> by any of the other bgworkers would be enough to prevent the last one
> to start, and mamba has been slow enough in the startup of the static
> workers to show that this could be possible.

I agree that that probably is the root cause, and we should fix it
by bumping up max_worker_processes in this test.

But this failure is annoying in another way.  Evidently,
worker_spi_launch returned NULL, which must have been from here:

if (!RegisterDynamicBackgroundWorker(, ))
PG_RETURN_NULL();

Why in the world is that "return NULL", and not "ereport(ERROR)"
like all the other failure conditions in that function?

If there's actually a defensible reason for the C code to act
like that, then all the call sites need to have checks for
a null result.

(cc'ing Robert, as this coding originated at 090d0f205.)

regards, tom lane

Re: Add a new BGWORKER_BYPASS_ROLELOGINCHECK flag

[Michael Paquier]

On Thu, Oct 12, 2023 at 07:42:28AM +0200, Drouvot, Bertrand wrote:
> On 10/12/23 2:26 AM, Michael Paquier wrote:
>> I have tweaked a few comments, and applied that.  Thanks.
> 
> Oh and you also closed the CF entry, thanks!

The buildfarm has provided some feedback, and the new tests have been
unstable on mamba:
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=mamba=2023-10-15%2005%3A04%3A21

In more details:
# poll_query_until timed out executing this query:
# SELECT datname, usename, wait_event FROM pg_stat_activity
# WHERE backend_type = 'worker_spi dynamic' AND
# pid = ;
# expecting this output:
# mydb|nologrole|WorkerSpiMain
# last actual query output:
#
# with stderr:
# ERROR:  syntax error at or near ";"
# LINE 3: pid = ;

So this looks like a hard failure in starting the worker that should
bypass the role login check.  The logs don't offer much information,
but I think I know what's going on here: at this stage of the tests,
the number of workers created is 7, very close to the limit of
max_worker_processes, at 8 by default.  So one parallel worker spawned
by any of the other bgworkers would be enough to prevent the last one
to start, and mamba has been slow enough in the startup of the static
workers to show that this could be possible.

I think that we should just bump up max_worker_processes, like in the
attached.
--
Michael
diff --git a/src/test/modules/worker_spi/t/001_worker_spi.pl b/src/test/modules/worker_spi/t/001_worker_spi.pl
index 85b8934f4e..ba1d281e81 100644
--- a/src/test/modules/worker_spi/t/001_worker_spi.pl
+++ b/src/test/modules/worker_spi/t/001_worker_spi.pl
@@ -63,11 +63,14 @@ $node->safe_psql('postgres', q(CREATE ROLE myrole SUPERUSER LOGIN;));
 $node->safe_psql('mydb', 'CREATE EXTENSION worker_spi;');
 
 # Now load the module as a shared library.
+# Update max_worker_processes to make room for enough bgworkers, including
+# parallel workers these may spawn.
 $node->append_conf(
 	'postgresql.conf', q{
 shared_preload_libraries = 'worker_spi'
 worker_spi.database = 'mydb'
 worker_spi.total_workers = 3
+max_worker_processes = 32
 });
 $node->restart;
 


signature.asc
Description: PGP signature

Re: Add a new BGWORKER_BYPASS_ROLELOGINCHECK flag

[Drouvot, Bertrand]

Hi,

On 10/12/23 2:26 AM, Michael Paquier wrote:

On Wed, Oct 11, 2023 at 08:48:04AM +0200, Drouvot, Bertrand wrote:

Yeah, agree. Changed in v12 attached.


I have tweaked a few comments, and applied that.  Thanks.


Oh and you also closed the CF entry, thanks!

Regards,

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

Re: Add a new BGWORKER_BYPASS_ROLELOGINCHECK flag

[Michael Paquier]

On Wed, Oct 11, 2023 at 08:48:04AM +0200, Drouvot, Bertrand wrote:
> Yeah, agree. Changed in v12 attached.

I have tweaked a few comments, and applied that.  Thanks.
--
Michael


signature.asc
Description: PGP signature

Re: Add a new BGWORKER_BYPASS_ROLELOGINCHECK flag

[Drouvot, Bertrand]

Hi,

On 10/11/23 5:40 AM, Michael Paquier wrote:

On Wed, Oct 11, 2023 at 08:26:42AM +0900, Michael Paquier wrote:

  /* flags for InitPostgres() */
  #define INIT_PG_LOAD_SESSION_LIBS  0x0001
  #define INIT_PG_OVERRIDE_ALLOW_CONNS   0x0002
+#define INIT_PG_BYPASS_ROLE_LOGIN  0x0004

In 0002, I am not sure that this is the best name for this new flag.
There is consistency with the bgworker part, for sure, but shouldn't
we name that OVERRIDE_ROLE_LOGIN instead in miscadmin.h?


Yeah, agree. Changed in v12 attached.

Regards,

--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.comFrom 176362155b1d92b5b10699484d0cbd8e770ecda7 Mon Sep 17 00:00:00 2001
From: Michael Paquier 
Date: Tue, 10 Oct 2023 14:56:36 +0900
Subject: [PATCH v12] Allow background workers to bypass login check

This adds a new flag value (BGWORKER_BYPASS_ROLELOGINCHECK) to the flags
being used in BackgroundWorkerInitializeConnection() and
BackgroundWorkerInitializeConnectionByOid().  This flag allows the
background workers to bypass the login check done in
InitializeSessionUserId().
---
 doc/src/sgml/bgworker.sgml|  4 ++-
 src/backend/postmaster/postmaster.c   |  6 
 src/backend/utils/init/miscinit.c |  4 +--
 src/backend/utils/init/postinit.c |  6 ++--
 src/include/miscadmin.h   |  4 ++-
 src/include/postmaster/bgworker.h |  6 ++--
 .../modules/worker_spi/t/001_worker_spi.pl| 31 +++
 src/test/modules/worker_spi/worker_spi.c  |  2 ++
 8 files changed, 55 insertions(+), 8 deletions(-)
  11.1% doc/src/sgml/
   8.7% src/backend/postmaster/
  19.5% src/backend/utils/init/
  10.7% src/include/postmaster/
   3.4% src/include/
  42.8% src/test/modules/worker_spi/t/
   3.5% src/test/modules/worker_spi/

diff --git a/doc/src/sgml/bgworker.sgml b/doc/src/sgml/bgworker.sgml
index 9ad1146ba0..8b61cf1c55 100644
--- a/doc/src/sgml/bgworker.sgml
+++ b/doc/src/sgml/bgworker.sgml
@@ -200,7 +200,9 @@ typedef struct BackgroundWorker
InvalidOid, the process will run as the superuser created
during initdb. If 
BGWORKER_BYPASS_ALLOWCONN
is specified as flags it is possible to bypass the 
restriction
-   to connect to databases not allowing user connections.
+   to connect to databases not allowing user connections. If 
BGWORKER_BYPASS_ROLELOGINCHECK
+   is specified as flags it is possible to bypass the login 
check for the
+   role used to connect to databases.
A background worker can only call one of these two functions, and only
once.  It is not possible to switch databases.
   
diff --git a/src/backend/postmaster/postmaster.c 
b/src/backend/postmaster/postmaster.c
index 3d7fec995a..c711933f26 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -5567,6 +5567,9 @@ BackgroundWorkerInitializeConnection(const char *dbname, 
const char *username, u
/* ignore datallowconn? */
if (flags & BGWORKER_BYPASS_ALLOWCONN)
init_flags |= INIT_PG_OVERRIDE_ALLOW_CONNS;
+   /* bypass login check? */
+   if (flags & BGWORKER_BYPASS_ROLELOGINCHECK)
+   init_flags |= INIT_PG_OVERRIDE_ROLE_LOGIN;
 
/* XXX is this the right errcode? */
if (!(worker->bgw_flags & BGWORKER_BACKEND_DATABASE_CONNECTION))
@@ -5598,6 +5601,9 @@ BackgroundWorkerInitializeConnectionByOid(Oid dboid, Oid 
useroid, uint32 flags)
/* ignore datallowconn? */
if (flags & BGWORKER_BYPASS_ALLOWCONN)
init_flags |= INIT_PG_OVERRIDE_ALLOW_CONNS;
+   /* bypass login check? */
+   if (flags & BGWORKER_BYPASS_ROLELOGINCHECK)
+   init_flags |= INIT_PG_OVERRIDE_ROLE_LOGIN;
 
/* XXX is this the right errcode? */
if (!(worker->bgw_flags & BGWORKER_BACKEND_DATABASE_CONNECTION))
diff --git a/src/backend/utils/init/miscinit.c 
b/src/backend/utils/init/miscinit.c
index 1e671c560c..182d666852 100644
--- a/src/backend/utils/init/miscinit.c
+++ b/src/backend/utils/init/miscinit.c
@@ -725,7 +725,7 @@ has_rolreplication(Oid roleid)
  * Initialize user identity during normal backend startup
  */
 void
-InitializeSessionUserId(const char *rolename, Oid roleid)
+InitializeSessionUserId(const char *rolename, Oid roleid, bool 
bypass_login_check)
 {
HeapTuple   roleTup;
Form_pg_authid rform;
@@ -789,7 +789,7 @@ InitializeSessionUserId(const char *rolename, Oid roleid)
/*
 * Is role allowed to login at all?
 */
-   if (!rform->rolcanlogin)
+   if (!bypass_login_check && !rform->rolcanlogin)
ereport(FATAL,

(errcode(ERRCODE_INVALID_AUTHORIZATION_SPECIFICATION),
 errmsg("role \"%s\" is not permitted 
to log in",
diff --git a/src/backend/utils/init/postinit.c 

Re: Add a new BGWORKER_BYPASS_ROLELOGINCHECK flag

[Michael Paquier]

On Wed, Oct 11, 2023 at 08:26:42AM +0900, Michael Paquier wrote:
> On Tue, Oct 10, 2023 at 09:12:49AM +0200, Drouvot, Bertrand wrote:
> > On 10/10/23 7:58 AM, Michael Paquier wrote:
> >> I was looking at v8 just before you sent this v9, and still got
> >> annoyed by the extra boolean argument added to InitPostgres().
> > 
> > Arf, I did not look at it as I had in mind to look at it once
> > this one is in.
> 
> No problem.  I'm OK to do it.

Applied 0001 for now.

> I am not sure that this is necessary in the code paths of
> BackgroundWorkerInitializeConnectionByOid() and
> BackgroundWorkerInitializeConnection() as datallowconn is handled a
> few lines down.

 /* flags for InitPostgres() */
 #define INIT_PG_LOAD_SESSION_LIBS  0x0001
 #define INIT_PG_OVERRIDE_ALLOW_CONNS   0x0002
+#define INIT_PG_BYPASS_ROLE_LOGIN  0x0004

In 0002, I am not sure that this is the best name for this new flag.
There is consistency with the bgworker part, for sure, but shouldn't
we name that OVERRIDE_ROLE_LOGIN instead in miscadmin.h?
--
Michael
From 550019a0ec84fcde8588bb9c2ae4ab08ad4d5fa5 Mon Sep 17 00:00:00 2001
From: Michael Paquier 
Date: Tue, 10 Oct 2023 14:56:36 +0900
Subject: [PATCH v11] Allow background workers to bypass login check

This adds a new flag value (BGWORKER_BYPASS_ROLELOGINCHECK) to the flags
being used in BackgroundWorkerInitializeConnection() and
BackgroundWorkerInitializeConnectionByOid().  This flag allows the
background workers to bypass the login check done in
InitializeSessionUserId().
---
 src/include/miscadmin.h   |  4 ++-
 src/include/postmaster/bgworker.h |  6 ++--
 src/backend/postmaster/postmaster.c   |  6 
 src/backend/utils/init/miscinit.c |  4 +--
 src/backend/utils/init/postinit.c |  6 ++--
 .../modules/worker_spi/t/001_worker_spi.pl| 31 +++
 src/test/modules/worker_spi/worker_spi.c  |  2 ++
 doc/src/sgml/bgworker.sgml|  4 ++-
 8 files changed, 55 insertions(+), 8 deletions(-)

diff --git a/src/include/miscadmin.h b/src/include/miscadmin.h
index 1cc3712c0f..e280b8192d 100644
--- a/src/include/miscadmin.h
+++ b/src/include/miscadmin.h
@@ -364,7 +364,8 @@ extern bool InSecurityRestrictedOperation(void);
 extern bool InNoForceRLSOperation(void);
 extern void GetUserIdAndContext(Oid *userid, bool *sec_def_context);
 extern void SetUserIdAndContext(Oid userid, bool sec_def_context);
-extern void InitializeSessionUserId(const char *rolename, Oid roleid);
+extern void InitializeSessionUserId(const char *rolename, Oid roleid,
+	bool bypass_login_check);
 extern void InitializeSessionUserIdStandalone(void);
 extern void SetSessionAuthorization(Oid userid, bool is_superuser);
 extern Oid	GetCurrentRoleId(void);
@@ -466,6 +467,7 @@ extern PGDLLIMPORT AuxProcType MyAuxProcType;
 /* flags for InitPostgres() */
 #define INIT_PG_LOAD_SESSION_LIBS		0x0001
 #define INIT_PG_OVERRIDE_ALLOW_CONNS	0x0002
+#define INIT_PG_BYPASS_ROLE_LOGIN		0x0004
 extern void pg_split_opts(char **argv, int *argcp, const char *optstr);
 extern void InitializeMaxBackends(void);
 extern void InitPostgres(const char *in_dbname, Oid dboid,
diff --git a/src/include/postmaster/bgworker.h b/src/include/postmaster/bgworker.h
index 7815507e3d..d7a5c1a946 100644
--- a/src/include/postmaster/bgworker.h
+++ b/src/include/postmaster/bgworker.h
@@ -150,9 +150,11 @@ extern void BackgroundWorkerInitializeConnectionByOid(Oid dboid, Oid useroid, ui
  * Flags to BackgroundWorkerInitializeConnection et al
  *
  *
- * Allow bypassing datallowconn restrictions when connecting to database
+ * Allow bypassing datallowconn restrictions and login check when connecting
+ * to database
  */
-#define BGWORKER_BYPASS_ALLOWCONN 1
+#define BGWORKER_BYPASS_ALLOWCONN 0x0001
+#define BGWORKER_BYPASS_ROLELOGINCHECK 0x0002
 
 
 /* Block/unblock signals in a background worker process */
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index 3d7fec995a..b1cabbd0d6 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -5567,6 +5567,9 @@ BackgroundWorkerInitializeConnection(const char *dbname, const char *username, u
 	/* ignore datallowconn? */
 	if (flags & BGWORKER_BYPASS_ALLOWCONN)
 		init_flags |= INIT_PG_OVERRIDE_ALLOW_CONNS;
+	/* bypass login check? */
+	if (flags & BGWORKER_BYPASS_ROLELOGINCHECK)
+		init_flags |= INIT_PG_BYPASS_ROLE_LOGIN;
 
 	/* XXX is this the right errcode? */
 	if (!(worker->bgw_flags & BGWORKER_BACKEND_DATABASE_CONNECTION))
@@ -5598,6 +5601,9 @@ BackgroundWorkerInitializeConnectionByOid(Oid dboid, Oid useroid, uint32 flags)
 	/* ignore datallowconn? */
 	if (flags & BGWORKER_BYPASS_ALLOWCONN)
 		init_flags |= INIT_PG_OVERRIDE_ALLOW_CONNS;
+	/* bypass login check? */
+	if (flags & BGWORKER_BYPASS_ROLELOGINCHECK)
+		init_flags |= INIT_PG_BYPASS_ROLE_LOGIN;
 
 	/* XXX is this the right errcode? */
 	if (!(worker->bgw_flags & 

Re: Add a new BGWORKER_BYPASS_ROLELOGINCHECK flag

[Michael Paquier]

On Tue, Oct 10, 2023 at 09:12:49AM +0200, Drouvot, Bertrand wrote:
> On 10/10/23 7:58 AM, Michael Paquier wrote:
>> I was looking at v8 just before you sent this v9, and still got
>> annoyed by the extra boolean argument added to InitPostgres().
> 
> Arf, I did not look at it as I had in mind to look at it once
> this one is in.

No problem.  I'm OK to do it.

>> So
>> please let me propose to bite the bullet and refactor that, as of the
>> 0001 attached that means less diff footprints in all the callers of
>> InitPostgres() (I am not wedded to the flag names).
> 
> Thanks for having looked at it!
> 
> +   bits32  init_flags = 0; /* never honor 
> session_preload_libraries */
> 
> Also a few word about datallowconn in the comment? (as the flag deals with 
> both).

I am not sure that this is necessary in the code paths of
BackgroundWorkerInitializeConnectionByOid() and
BackgroundWorkerInitializeConnection() as datallowconn is handled a
few lines down.
--
Michael


signature.asc
Description: PGP signature

Re: Add a new BGWORKER_BYPASS_ROLELOGINCHECK flag

[Drouvot, Bertrand]

On 10/10/23 9:21 AM, Michael Paquier wrote:

On Tue, Oct 10, 2023 at 09:18:04AM +0200, Drouvot, Bertrand wrote:

Please forget about it ;-)


That's called an ENOCOFFEE :D


Exactly, good one! ;-)

Regards,

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

Re: Add a new BGWORKER_BYPASS_ROLELOGINCHECK flag

[Michael Paquier]

On Tue, Oct 10, 2023 at 09:18:04AM +0200, Drouvot, Bertrand wrote:
> Please forget about it ;-)

That's called an ENOCOFFEE :D
--
Michael


signature.asc
Description: PGP signature

Re: Add a new BGWORKER_BYPASS_ROLELOGINCHECK flag

[Drouvot, Bertrand]

On 10/10/23 9:12 AM, Drouvot, Bertrand wrote:

Hi,
Any reason why INIT_PG_BYPASS_ROLE_LOGIN is not 0x0003?



Please forget about it ;-)

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

Re: Add a new BGWORKER_BYPASS_ROLELOGINCHECK flag

[Drouvot, Bertrand]

Hi,

On 10/10/23 7:58 AM, Michael Paquier wrote:

On Tue, Oct 10, 2023 at 06:57:05AM +0200, Drouvot, Bertrand wrote:

Please find attached v9 (v8 rebase due to f483b2090).


I was looking at v8 just before you sent this v9, and still got
annoyed by the extra boolean argument added to InitPostgres(). 


Arf, I did not look at it as I had in mind to look at it once
this one is in.


So
please let me propose to bite the bullet and refactor that, as of the
0001 attached that means less diff footprints in all the callers of
InitPostgres() (I am not wedded to the flag names).


Thanks for having looked at it!

+   bits32  init_flags = 0; /* never honor 
session_preload_libraries */

Also a few word about datallowconn in the comment? (as the flag deals with 
both).



It looks like 0002 had the same issues as f483b209: the worker that
could not be started because of the login restriction could be
detected as stopped by worker_spi_launch(), causing the script to fail
hard.

0002 is basically your v9, able to work with the refactoring from
0001.


Thanks!

 #define INIT_PG_OVERRIDE_ALLOW_CONNS   0x0002
+#define INIT_PG_BYPASS_ROLE_LOGIN  0x0004

Any reason why INIT_PG_BYPASS_ROLE_LOGIN is not 0x0003?

Except that it does look good to me.

Regards,

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

Re: Add a new BGWORKER_BYPASS_ROLELOGINCHECK flag

[Michael Paquier]

On Tue, Oct 10, 2023 at 06:57:05AM +0200, Drouvot, Bertrand wrote:
> Please find attached v9 (v8 rebase due to f483b2090).

I was looking at v8 just before you sent this v9, and still got
annoyed by the extra boolean argument added to InitPostgres().  So
please let me propose to bite the bullet and refactor that, as of the
0001 attached that means less diff footprints in all the callers of
InitPostgres() (I am not wedded to the flag names).

It looks like 0002 had the same issues as f483b209: the worker that
could not be started because of the login restriction could be
detected as stopped by worker_spi_launch(), causing the script to fail
hard.

0002 is basically your v9, able to work with the refactoring from
0001.  
--
Michael
From 6e094435789a0255ddd4961bd43cfae4bf54e135 Mon Sep 17 00:00:00 2001
From: Michael Paquier 
Date: Tue, 10 Oct 2023 14:41:56 +0900
Subject: [PATCH v10 1/2] Refactor InitPostgres() with flags

---
 src/include/miscadmin.h |  6 --
 src/backend/bootstrap/bootstrap.c   |  2 +-
 src/backend/postmaster/autovacuum.c |  5 ++---
 src/backend/postmaster/postmaster.c | 16 
 src/backend/tcop/postgres.c |  5 +++--
 src/backend/utils/init/postinit.c   | 17 +
 6 files changed, 31 insertions(+), 20 deletions(-)

diff --git a/src/include/miscadmin.h b/src/include/miscadmin.h
index 14bd574fc2..1cc3712c0f 100644
--- a/src/include/miscadmin.h
+++ b/src/include/miscadmin.h
@@ -463,12 +463,14 @@ extern PGDLLIMPORT AuxProcType MyAuxProcType;
  */
 
 /* in utils/init/postinit.c */
+/* flags for InitPostgres() */
+#define INIT_PG_LOAD_SESSION_LIBS		0x0001
+#define INIT_PG_OVERRIDE_ALLOW_CONNS	0x0002
 extern void pg_split_opts(char **argv, int *argcp, const char *optstr);
 extern void InitializeMaxBackends(void);
 extern void InitPostgres(const char *in_dbname, Oid dboid,
 		 const char *username, Oid useroid,
-		 bool load_session_libraries,
-		 bool override_allow_connections,
+		 bits32 flags,
 		 char *out_dbname);
 extern void BaseInit(void);
 
diff --git a/src/backend/bootstrap/bootstrap.c b/src/backend/bootstrap/bootstrap.c
index 5810f8825e..e01dca9b7c 100644
--- a/src/backend/bootstrap/bootstrap.c
+++ b/src/backend/bootstrap/bootstrap.c
@@ -345,7 +345,7 @@ BootstrapModeMain(int argc, char *argv[], bool check_only)
 	if (pg_link_canary_is_frontend())
 		elog(ERROR, "backend is incorrectly linked to frontend functions");
 
-	InitPostgres(NULL, InvalidOid, NULL, InvalidOid, false, false, NULL);
+	InitPostgres(NULL, InvalidOid, NULL, InvalidOid, 0, NULL);
 
 	/* Initialize stuff for bootstrap-file processing */
 	for (i = 0; i < MAXATTR; i++)
diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c
index ae9be9b911..327ea0d45a 100644
--- a/src/backend/postmaster/autovacuum.c
+++ b/src/backend/postmaster/autovacuum.c
@@ -488,7 +488,7 @@ AutoVacLauncherMain(int argc, char *argv[])
 	/* Early initialization */
 	BaseInit();
 
-	InitPostgres(NULL, InvalidOid, NULL, InvalidOid, false, false, NULL);
+	InitPostgres(NULL, InvalidOid, NULL, InvalidOid, 0, NULL);
 
 	SetProcessingMode(NormalProcessing);
 
@@ -1706,8 +1706,7 @@ AutoVacWorkerMain(int argc, char *argv[])
 		 * Note: if we have selected a just-deleted database (due to using
 		 * stale stats info), we'll fail and exit here.
 		 */
-		InitPostgres(NULL, dbid, NULL, InvalidOid, false, false,
-	 dbname);
+		InitPostgres(NULL, dbid, NULL, InvalidOid, 0, dbname);
 		SetProcessingMode(NormalProcessing);
 		set_ps_display(dbname);
 		ereport(DEBUG1,
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index 0761b38bf8..3d7fec995a 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -5562,6 +5562,11 @@ void
 BackgroundWorkerInitializeConnection(const char *dbname, const char *username, uint32 flags)
 {
 	BackgroundWorker *worker = MyBgworkerEntry;
+	bits32		init_flags = 0; /* never honor session_preload_libraries */
+
+	/* ignore datallowconn? */
+	if (flags & BGWORKER_BYPASS_ALLOWCONN)
+		init_flags |= INIT_PG_OVERRIDE_ALLOW_CONNS;
 
 	/* XXX is this the right errcode? */
 	if (!(worker->bgw_flags & BGWORKER_BACKEND_DATABASE_CONNECTION))
@@ -5571,8 +5576,7 @@ BackgroundWorkerInitializeConnection(const char *dbname, const char *username, u
 
 	InitPostgres(dbname, InvalidOid,	/* database to connect to */
  username, InvalidOid,	/* role to connect as */
- false,			/* never honor session_preload_libraries */
- (flags & BGWORKER_BYPASS_ALLOWCONN) != 0,	/* ignore datallowconn? */
+ init_flags,
  NULL);			/* no out_dbname */
 
 	/* it had better not gotten out of "init" mode yet */
@@ -5589,6 +5593,11 @@ void
 BackgroundWorkerInitializeConnectionByOid(Oid dboid, Oid useroid, uint32 flags)
 {
 	BackgroundWorker *worker = MyBgworkerEntry;
+	bits32		init_flags = 0; /* never honor 

Re: Add a new BGWORKER_BYPASS_ROLELOGINCHECK flag

[Drouvot, Bertrand]

Hi,

On 10/6/23 8:48 AM, Drouvot, Bertrand wrote:

Hi,

On 10/5/23 6:23 PM, Bharath Rupireddy wrote:

On Thu, Oct 5, 2023 at 9:32 PM Drouvot, Bertrand
 wrote:


+  CREATE ROLE nologrole with nologin;
+  GRANT CREATE ON DATABASE mydb TO nologrole;


A few nit-picks:

1. s/with/WITH
2. s/nologin/NOLOGIN


done in v8 attached.


3. +   is specified as flags it is possible to
bypass the login check to connect to databases.
How about "it is possible to bypass the login check for the role used
to connect to databases."?



"for the role used" sounds implicit to me but I don't have a strong opinion
about it so re-worded as per your proposal in v8.



Please find attached v9 (v8 rebase due to f483b2090).

Regards,

--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.comFrom e3a75359b2f3a57d7312ff4ab3f77ae78f44f221 Mon Sep 17 00:00:00 2001
From: Michael Paquier 
Date: Thu, 5 Oct 2023 14:06:37 +0900
Subject: [PATCH v9] Allow background workers to bypass login check

This adds a new flag value (BGWORKER_BYPASS_ROLELOGINCHECK) to the flags
being used in BackgroundWorkerInitializeConnection() and
BackgroundWorkerInitializeConnectionByOid().  This flag allows the
background workers to bypass the login check done in
InitializeSessionUserId().
---
 doc/src/sgml/bgworker.sgml|  4 ++-
 src/backend/bootstrap/bootstrap.c |  2 +-
 src/backend/postmaster/autovacuum.c   |  5 ++-
 src/backend/postmaster/postmaster.c   |  2 ++
 src/backend/tcop/postgres.c   |  1 +
 src/backend/utils/init/miscinit.c |  4 +--
 src/backend/utils/init/postinit.c |  5 +--
 src/include/miscadmin.h   |  4 ++-
 src/include/postmaster/bgworker.h |  6 ++--
 .../modules/worker_spi/t/001_worker_spi.pl| 36 +++
 src/test/modules/worker_spi/worker_spi.c  |  2 ++
 11 files changed, 59 insertions(+), 12 deletions(-)
  10.0% doc/src/sgml/
  10.9% src/backend/postmaster/
  15.4% src/backend/utils/init/
   9.6% src/include/postmaster/
  45.8% src/test/modules/worker_spi/t/
   3.1% src/test/modules/worker_spi/
   4.8% src/

diff --git a/doc/src/sgml/bgworker.sgml b/doc/src/sgml/bgworker.sgml
index 9ad1146ba0..8b61cf1c55 100644
--- a/doc/src/sgml/bgworker.sgml
+++ b/doc/src/sgml/bgworker.sgml
@@ -200,7 +200,9 @@ typedef struct BackgroundWorker
InvalidOid, the process will run as the superuser created
during initdb. If 
BGWORKER_BYPASS_ALLOWCONN
is specified as flags it is possible to bypass the 
restriction
-   to connect to databases not allowing user connections.
+   to connect to databases not allowing user connections. If 
BGWORKER_BYPASS_ROLELOGINCHECK
+   is specified as flags it is possible to bypass the login 
check for the
+   role used to connect to databases.
A background worker can only call one of these two functions, and only
once.  It is not possible to switch databases.
   
diff --git a/src/backend/bootstrap/bootstrap.c 
b/src/backend/bootstrap/bootstrap.c
index 5810f8825e..bdd005b66a 100644
--- a/src/backend/bootstrap/bootstrap.c
+++ b/src/backend/bootstrap/bootstrap.c
@@ -345,7 +345,7 @@ BootstrapModeMain(int argc, char *argv[], bool check_only)
if (pg_link_canary_is_frontend())
elog(ERROR, "backend is incorrectly linked to frontend 
functions");
 
-   InitPostgres(NULL, InvalidOid, NULL, InvalidOid, false, false, NULL);
+   InitPostgres(NULL, InvalidOid, NULL, InvalidOid, false, false, false, 
NULL);
 
/* Initialize stuff for bootstrap-file processing */
for (i = 0; i < MAXATTR; i++)
diff --git a/src/backend/postmaster/autovacuum.c 
b/src/backend/postmaster/autovacuum.c
index ae9be9b911..4686b81f68 100644
--- a/src/backend/postmaster/autovacuum.c
+++ b/src/backend/postmaster/autovacuum.c
@@ -488,7 +488,7 @@ AutoVacLauncherMain(int argc, char *argv[])
/* Early initialization */
BaseInit();
 
-   InitPostgres(NULL, InvalidOid, NULL, InvalidOid, false, false, NULL);
+   InitPostgres(NULL, InvalidOid, NULL, InvalidOid, false, false, false, 
NULL);
 
SetProcessingMode(NormalProcessing);
 
@@ -1706,8 +1706,7 @@ AutoVacWorkerMain(int argc, char *argv[])
 * Note: if we have selected a just-deleted database (due to 
using
 * stale stats info), we'll fail and exit here.
 */
-   InitPostgres(NULL, dbid, NULL, InvalidOid, false, false,
-dbname);
+   InitPostgres(NULL, dbid, NULL, InvalidOid, false, false, false, 
dbname);
SetProcessingMode(NormalProcessing);
set_ps_display(dbname);
ereport(DEBUG1,
diff --git a/src/backend/postmaster/postmaster.c 
b/src/backend/postmaster/postmaster.c
index 0761b38bf8..0502588013 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c

Re: Add a new BGWORKER_BYPASS_ROLELOGINCHECK flag

[Michael Paquier]

On Mon, Oct 09, 2023 at 11:11:58PM -0400, Tom Lane wrote:
> Michael Paquier  writes:
>> Saying that, I'm OK with just dropping this query, as it could also be
>> possible that one decides that calling pgstat_bestart() before the
>> datallowconn check is a good idea for a reason or another.
> 
> Not sure if that's a likely change or not.  However, if we're in
> agreement that this test step isn't buying much, let's just drop
> it and save the test cycles.

No problem here.  f483b2090 has removed the query entirely, relying
now only on a wait_for_log() when the worker startup fails.
--
Michael


signature.asc
Description: PGP signature

Re: Add a new BGWORKER_BYPASS_ROLELOGINCHECK flag

[Tom Lane]

Michael Paquier  writes:
> On Mon, Oct 09, 2023 at 12:20:18PM -0400, Tom Lane wrote:
>> There will be a window where the worker has logged "database
>> "noconndb" is not currently accepting connections" but hasn't yet
>> exited, so that conceivably this query could see a positive count.

> I don't think that's possible here.  The check on datallowconn is done
> before a backend calls pgstat_bestart() which would make its backend
> entry reported to pg_stat_activity.  So there is no window where a
> backend would be in pg_stat_activity if this check fails.

Ah, right.  I complained after seeing that we set MyProc->databaseId
before doing CheckMyDatabase, but you're right that it doesn't
matter for pg_stat_activity until pgstat_bestart. 

> Saying that, I'm OK with just dropping this query, as it could also be
> possible that one decides that calling pgstat_bestart() before the
> datallowconn check is a good idea for a reason or another.

Not sure if that's a likely change or not.  However, if we're in
agreement that this test step isn't buying much, let's just drop
it and save the test cycles.

regards, tom lane

Re: Add a new BGWORKER_BYPASS_ROLELOGINCHECK flag

[Michael Paquier]

On Mon, Oct 09, 2023 at 12:20:18PM -0400, Tom Lane wrote:
> There will be a window where the worker has logged "database
> "noconndb" is not currently accepting connections" but hasn't yet
> exited, so that conceivably this query could see a positive count.

I don't think that's possible here.  The check on datallowconn is done
before a backend calls pgstat_bestart() which would make its backend
entry reported to pg_stat_activity.  So there is no window where a
backend would be in pg_stat_activity if this check fails.

> We could just drop this test, reasoning that the appearance of
> the error message is sufficient evidence that the right thing
> happened.  (If the failed worker is still around, it won't break
> the remaining tests AFAICS.)  Or we could convert this to a
> poll_query_until() loop.

Saying that, I'm OK with just dropping this query, as it could also be
possible that one decides that calling pgstat_bestart() before the
datallowconn check is a good idea for a reason or another.
--
Michael


signature.asc
Description: PGP signature

Re: Add a new BGWORKER_BYPASS_ROLELOGINCHECK flag

[Tom Lane]

Michael Paquier  writes:
> Another thing is that we cannot rely on the PID returned by launch()
> as it could fail, so $worker3_pid needs to disappear.  If we do that,
> I'd rather just switch to a specific database for the tests with
> ALLOWCONN rather than reuse "mydb" that could have other workers.

Right.

> The
> attached fixes the issue for me.

Hmm.  This passed a few dozen test cycles on mamba's host,
but it seems to me there's still a race condition here:

$result = $node->safe_psql('postgres',
"SELECT count(*) FROM pg_stat_activity WHERE datname = 'noconndb';");
is($result, '0', 'dynamic bgworker without BYPASS_ALLOWCONN not started');

There will be a window where the worker has logged "database
"noconndb" is not currently accepting connections" but hasn't yet
exited, so that conceivably this query could see a positive count.

We could just drop this test, reasoning that the appearance of
the error message is sufficient evidence that the right thing
happened.  (If the failed worker is still around, it won't break
the remaining tests AFAICS.)  Or we could convert this to a
poll_query_until() loop.

regards, tom lane

Re: Add a new BGWORKER_BYPASS_ROLELOGINCHECK flag

[Michael Paquier]

On Sun, Oct 08, 2023 at 05:48:55PM -0400, Tom Lane wrote:
> There have been intermittent failures on various buildfarm machines
> since this went in.  After seeing one on my own animal mamba [1],
> I tried to reproduce it manually on that machine, and it does
> indeed fail about one time in two.  The buildfarm script is not
> managing to capture the relevant log files, but what I see in a
> manual run is that 001_worker_spi.pl logs this:

Thanks for the logs, I've noticed the failure but could not make any
sense of it based on the lack of information provided from the
buildfarm.  Serinus has complained once, for instance. 

> Since this only seems to happen on slow machines, I'd call it a timing
> problem or race condition.  Unless you want to argue that the race
> should not happen, probably the fix is to make the test script cope
> with this worker_spi_launch() call failing.  As long as we see the
> expected result from wait_for_log, we can be pretty sure the right
> thing happened.

The trick to reproduce the failure is to slow down worker_spi_launch()
before WaitForBackgroundWorkerStartup() with a worker already
registered so as the worker has the time to start and exit because of
the ALLOW_CONNECTIONS restriction.  (SendPostmasterSignal() in
RegisterDynamicBackgroundWorker() interrupts a hardcoded sleep, so
I've just used an on-disk flag.)

Another thing is that we cannot rely on the PID returned by launch()
as it could fail, so $worker3_pid needs to disappear.  If we do that,
I'd rather just switch to a specific database for the tests with
ALLOWCONN rather than reuse "mydb" that could have other workers.  The
attached fixes the issue for me.
--
Michael
diff --git a/src/test/modules/worker_spi/t/001_worker_spi.pl b/src/test/modules/worker_spi/t/001_worker_spi.pl
index 06bb73816f..044e208812 100644
--- a/src/test/modules/worker_spi/t/001_worker_spi.pl
+++ b/src/test/modules/worker_spi/t/001_worker_spi.pl
@@ -105,30 +105,31 @@ postgres|myrole|WorkerSpiMain]),
 ) or die "Timed out while waiting for dynamic bgworkers to be launched";
 
 # Check BGWORKER_BYPASS_ALLOWCONN.
-$node->safe_psql('postgres', q(ALTER DATABASE mydb ALLOW_CONNECTIONS false;));
+$node->safe_psql('postgres', q(CREATE DATABASE noconndb ALLOW_CONNECTIONS false;));
+my $noconndb_id = $node->safe_psql('mydb',
+	"SELECT oid FROM pg_database where datname = 'noconndb';");
 my $log_offset = -s $node->logfile;
 
-# bgworker cannot be launched with connection restriction.
-my $worker3_pid = $node->safe_psql('postgres',
-	qq[SELECT worker_spi_launch(12, $mydb_id, $myrole_id);]);
+# worker_spi_launch() may be able to detect that the worker has been
+# stopped, so do not rely on psql_safe().
+$node->psql('postgres',
+	qq[SELECT worker_spi_launch(12, $noconndb_id, $myrole_id);]);
 $node->wait_for_log(
-	qr/database "mydb" is not currently accepting connections/, $log_offset);
+	qr/database "noconndb" is not currently accepting connections/, $log_offset);
 
 $result = $node->safe_psql('postgres',
-	"SELECT count(*) FROM pg_stat_activity WHERE pid = $worker3_pid;");
+	"SELECT count(*) FROM pg_stat_activity WHERE datname = 'noconndb';");
 is($result, '0', 'dynamic bgworker without BYPASS_ALLOWCONN not started');
 
 # bgworker bypasses the connection check, and can be launched.
 my $worker4_pid = $node->safe_psql('postgres',
-	qq[SELECT worker_spi_launch(12, $mydb_id, $myrole_id, '{"ALLOWCONN"}');]);
+	qq[SELECT worker_spi_launch(12, $noconndb_id, $myrole_id, '{"ALLOWCONN"}');]);
 ok( $node->poll_query_until(
 		'postgres',
 		qq[SELECT datname, usename, wait_event FROM pg_stat_activity
 WHERE backend_type = 'worker_spi dynamic' AND
 pid IN ($worker4_pid) ORDER BY datname;],
-		qq[mydb|myrole|WorkerSpiMain]),
+		qq[noconndb|myrole|WorkerSpiMain]),
 	'dynamic bgworker with BYPASS_ALLOWCONN started');
 
-$node->safe_psql('postgres', q(ALTER DATABASE mydb ALLOW_CONNECTIONS true;));
-
 done_testing();


signature.asc
Description: PGP signature

Re: Add a new BGWORKER_BYPASS_ROLELOGINCHECK flag

[Tom Lane]

Michael Paquier  writes:
> On Fri, Oct 06, 2023 at 03:29:28PM +0900, Michael Paquier wrote:
>> Andres, are there logs for this TAP test on serinus?  Or perhaps there
>> is a core file that could be looked at?  The other animals are not
>> showing anything for the moment.

> Well, it looks OK.  Still that's itching a bit.

There have been intermittent failures on various buildfarm machines
since this went in.  After seeing one on my own animal mamba [1],
I tried to reproduce it manually on that machine, and it does
indeed fail about one time in two.  The buildfarm script is not
managing to capture the relevant log files, but what I see in a
manual run is that 001_worker_spi.pl logs this:

...
# Postmaster PID for node "mynode" is 21897
[01:19:53.931](2.663s) ok 5 - bgworkers all launched
[01:19:54.711](0.780s) ok 6 - dynamic bgworkers all launched
error running SQL: 'psql::1: ERROR:  could not start background process
HINT:  More details may be available in the server log.'
while running 'psql -XAtq -d port=56393 host=/tmp/PETPK0Stwi dbname='postgres' 
-f - -v ON_ERROR_STOP=1' with sql 'SELECT worker_spi_launch(12, 16394, 16395);' 
at 
/home/tgl/pgsql/src/test/modules/worker_spi/../../../../src/test/perl/PostgreSQL/Test/Cluster.pm
 line 2009.
# Postmaster PID for node "mynode" is 21897
### Stopping node "mynode" using mode immediate
# Running: pg_ctl -D 
/home/tgl/pgsql/src/test/modules/worker_spi/tmp_check/t_001_worker_spi_mynode_data/pgdata
 -m immediate stop
waiting for server to shut down done
server stopped
# No postmaster PID for node "mynode"
[01:19:55.032](0.321s) # Tests were run but no plan was declared and 
done_testing() was not seen.
[01:19:55.035](0.002s) # Looks like your test exited with 29 just after 6.

and in the postmaster log

2023-10-08 01:19:54.265 EDT [5820] LOG:  worker_spi dynamic worker 10 
initialized with schema10.counted
2023-10-08 01:19:54.378 EDT [27776] 001_worker_spi.pl LOG:  statement: SELECT 
worker_spi_launch(11, 5, 16395);
2023-10-08 01:19:54.476 EDT [18120] 001_worker_spi.pl LOG:  statement: SELECT 
datname, usename, wait_event FROM pg_stat_activity
WHERE backend_type = 'worker_spi dynamic' AND
pid IN (5820, 428) ORDER BY datname;
2023-10-08 01:19:54.548 EDT [428] LOG:  worker_spi dynamic worker 11 
initialized with schema11.counted
2023-10-08 01:19:54.680 EDT [152] 001_worker_spi.pl LOG:  statement: SELECT 
datname, usename, wait_event FROM pg_stat_activity
WHERE backend_type = 'worker_spi dynamic' AND
pid IN (5820, 428) ORDER BY datname;
2023-10-08 01:19:54.779 EDT [1675] 001_worker_spi.pl LOG:  statement: ALTER 
DATABASE mydb ALLOW_CONNECTIONS false;
2023-10-08 01:19:54.854 EDT [26562] 001_worker_spi.pl LOG:  statement: SELECT 
worker_spi_launch(12, 16394, 16395);
2023-10-08 01:19:54.878 EDT [23636] FATAL:  database "mydb" is not currently 
accepting connections
2023-10-08 01:19:54.888 EDT [21897] LOG:  background worker "worker_spi 
dynamic" (PID 23636) exited with exit code 1
2023-10-08 01:19:54.888 EDT [26562] 001_worker_spi.pl ERROR:  could not start 
background process
2023-10-08 01:19:54.888 EDT [26562] 001_worker_spi.pl HINT:  More details may 
be available in the server log.
2023-10-08 01:19:54.888 EDT [26562] 001_worker_spi.pl STATEMENT:  SELECT 
worker_spi_launch(12, 16394, 16395);
2023-10-08 01:19:54.912 EDT [21897] LOG:  received immediate shutdown request
2023-10-08 01:19:55.014 EDT [21897] LOG:  database system is shut down

What it looks like to me is that there is a code path by which "could
not start background process" is reported as a failure of the SELECT
worker_spi_launch() query itself.  The test script is not expecting
that, because it executes that query with

# bgworker cannot be launched with connection restriction.
my $worker3_pid = $node->safe_psql('postgres',
   qq[SELECT worker_spi_launch(12, $mydb_id, $myrole_id);]);
$node->wait_for_log(
   qr/database "mydb" is not currently accepting connections/, $log_offset);

so safe_psql bails out and we get no further.

Since this only seems to happen on slow machines, I'd call it a timing
problem or race condition.  Unless you want to argue that the race
should not happen, probably the fix is to make the test script cope
with this worker_spi_launch() call failing.  As long as we see the
expected result from wait_for_log, we can be pretty sure the right
thing happened.

regards, tom lane

[1] 
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=mamba=2023-10-08%2001%3A00%3A22

Re: Add a new BGWORKER_BYPASS_ROLELOGINCHECK flag

[Michael Paquier]

On Fri, Oct 06, 2023 at 03:29:28PM +0900, Michael Paquier wrote:
> Andres, are there logs for this TAP test on serinus?  Or perhaps there
> is a core file that could be looked at?  The other animals are not
> showing anything for the moment.

serinus has reported back once again, and just returned with a green
state, twice in a row:
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=serinus=2023-10-06%2007%3A42%3A53
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=serinus=2023-10-06%2007%3A28%3A02

Well, it looks OK.  Still that's itching a bit.
--
Michael


signature.asc
Description: PGP signature

Re: Add a new BGWORKER_BYPASS_ROLELOGINCHECK flag

[Drouvot, Bertrand]

Hi,

On 10/5/23 6:23 PM, Bharath Rupireddy wrote:

On Thu, Oct 5, 2023 at 9:32 PM Drouvot, Bertrand
 wrote:


+  CREATE ROLE nologrole with nologin;
+  GRANT CREATE ON DATABASE mydb TO nologrole;


A few nit-picks:

1. s/with/WITH
2. s/nologin/NOLOGIN


done in v8 attached.


3. +   is specified as flags it is possible to
bypass the login check to connect to databases.
How about "it is possible to bypass the login check for the role used
to connect to databases."?



"for the role used" sounds implicit to me but I don't have a strong opinion
about it so re-worded as per your proposal in v8.

Regards,

--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.comFrom 741cd631822f13e101966f06dbb56b3a141af8df Mon Sep 17 00:00:00 2001
From: Michael Paquier 
Date: Thu, 5 Oct 2023 14:06:37 +0900
Subject: [PATCH v8] Allow background workers to bypass login check

This adds a new flag value (BGWORKER_BYPASS_ROLELOGINCHECK) to the flags
being used in BackgroundWorkerInitializeConnection() and
BackgroundWorkerInitializeConnectionByOid().  This flag allows the
background workers to bypass the login check done in
InitializeSessionUserId().
---
 doc/src/sgml/bgworker.sgml|  4 ++-
 src/backend/bootstrap/bootstrap.c |  2 +-
 src/backend/postmaster/autovacuum.c   |  5 ++-
 src/backend/postmaster/postmaster.c   |  2 ++
 src/backend/tcop/postgres.c   |  1 +
 src/backend/utils/init/miscinit.c |  4 +--
 src/backend/utils/init/postinit.c |  5 +--
 src/include/miscadmin.h   |  4 ++-
 src/include/postmaster/bgworker.h |  6 ++--
 .../modules/worker_spi/t/001_worker_spi.pl| 36 +++
 src/test/modules/worker_spi/worker_spi.c  |  2 ++
 11 files changed, 59 insertions(+), 12 deletions(-)
  10.0% doc/src/sgml/
  10.9% src/backend/postmaster/
  15.4% src/backend/utils/init/
   9.6% src/include/postmaster/
  45.8% src/test/modules/worker_spi/t/
   3.1% src/test/modules/worker_spi/
   4.8% src/

diff --git a/doc/src/sgml/bgworker.sgml b/doc/src/sgml/bgworker.sgml
index 9ad1146ba0..8b61cf1c55 100644
--- a/doc/src/sgml/bgworker.sgml
+++ b/doc/src/sgml/bgworker.sgml
@@ -200,7 +200,9 @@ typedef struct BackgroundWorker
InvalidOid, the process will run as the superuser created
during initdb. If 
BGWORKER_BYPASS_ALLOWCONN
is specified as flags it is possible to bypass the 
restriction
-   to connect to databases not allowing user connections.
+   to connect to databases not allowing user connections. If 
BGWORKER_BYPASS_ROLELOGINCHECK
+   is specified as flags it is possible to bypass the login 
check for the
+   role used to connect to databases.
A background worker can only call one of these two functions, and only
once.  It is not possible to switch databases.
   
diff --git a/src/backend/bootstrap/bootstrap.c 
b/src/backend/bootstrap/bootstrap.c
index 5810f8825e..bdd005b66a 100644
--- a/src/backend/bootstrap/bootstrap.c
+++ b/src/backend/bootstrap/bootstrap.c
@@ -345,7 +345,7 @@ BootstrapModeMain(int argc, char *argv[], bool check_only)
if (pg_link_canary_is_frontend())
elog(ERROR, "backend is incorrectly linked to frontend 
functions");
 
-   InitPostgres(NULL, InvalidOid, NULL, InvalidOid, false, false, NULL);
+   InitPostgres(NULL, InvalidOid, NULL, InvalidOid, false, false, false, 
NULL);
 
/* Initialize stuff for bootstrap-file processing */
for (i = 0; i < MAXATTR; i++)
diff --git a/src/backend/postmaster/autovacuum.c 
b/src/backend/postmaster/autovacuum.c
index ae9be9b911..4686b81f68 100644
--- a/src/backend/postmaster/autovacuum.c
+++ b/src/backend/postmaster/autovacuum.c
@@ -488,7 +488,7 @@ AutoVacLauncherMain(int argc, char *argv[])
/* Early initialization */
BaseInit();
 
-   InitPostgres(NULL, InvalidOid, NULL, InvalidOid, false, false, NULL);
+   InitPostgres(NULL, InvalidOid, NULL, InvalidOid, false, false, false, 
NULL);
 
SetProcessingMode(NormalProcessing);
 
@@ -1706,8 +1706,7 @@ AutoVacWorkerMain(int argc, char *argv[])
 * Note: if we have selected a just-deleted database (due to 
using
 * stale stats info), we'll fail and exit here.
 */
-   InitPostgres(NULL, dbid, NULL, InvalidOid, false, false,
-dbname);
+   InitPostgres(NULL, dbid, NULL, InvalidOid, false, false, false, 
dbname);
SetProcessingMode(NormalProcessing);
set_ps_display(dbname);
ereport(DEBUG1,
diff --git a/src/backend/postmaster/postmaster.c 
b/src/backend/postmaster/postmaster.c
index 0d876c61fd..a308ffa1c5 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -5572,6 +5572,7 @@ BackgroundWorkerInitializeConnection(const char *dbname, 
const char *username, u
 

Re: Add a new BGWORKER_BYPASS_ROLELOGINCHECK flag

[Michael Paquier]

On Fri, Oct 06, 2023 at 09:09:10AM +0900, Michael Paquier wrote:
> I am not completely sure what you mean here.  We've never supported
> load_session_libraries for background workers, and I'm not quite sure
> that there is a case for it.  FWIW, my idea around that would be to
> have two separate sets of flags: one set for the bgworkers and one set
> for PostgresInit() as an effect of load_session_libraries which looks
> like a fuzzy concept for bgworkers.

I have applied v1 a few hours ago as of 991bb0f9653c.  Then, the
buildfarm has quickly complained that a bgworkers able to start with 
BYPASS_ALLOWCONN while the database has its access restricted could
spawn workers that themselves try to connect to the database
restricted, causing the test to fail.  I have applied fd4d93d269c0 as
a quick way to avoid the spawn of workers in this case, and the
buildfarm has turned back to green.

Now, there's been a second type failure on serinus even after all
that:
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=serinus=2023-10-06%2001%3A04%3A05

The step running a `make check` on worker_spi in the run has worked:
https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=serinus=2023-10-06%2001%3A04%3A05=module-worker_spi-check

But the follow-up step doing an installcheck with worker_spi has not.
And this looks like a crash:
https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=serinus=2023-10-06%2001%3A04%3A05=testmodules-install-check-C

The logs reported by this step are not really helpful, as they contain
only information about the sql/ tests in the modules.  It is the first
time that this stuff is tested, so this could be a race condition
that's been around for some time but we've never seen it until now, or
it could be an issue in the test I fail to see.

Andres, are there logs for this TAP test on serinus?  Or perhaps there
is a core file that could be looked at?  The other animals are not
showing anything for the moment.
--
Michael


signature.asc
Description: PGP signature

Re: Add a new BGWORKER_BYPASS_ROLELOGINCHECK flag

[Michael Paquier]

On Thu, Oct 05, 2023 at 05:51:31PM +0530, Bharath Rupireddy wrote:
> I think changing BGWORKER_BYPASS_ALLOWCONN to 0x0001 and having bits32
> for InitPostgres inputs load_session_libraries and
> override_allow_connections can be 0001 in this patch series so that it
> can go first, no? This avoids the new code being in the old format and
> changing things right after it commits.

I am not completely sure what you mean here.  We've never supported
load_session_libraries for background workers, and I'm not quite sure
that there is a case for it.  FWIW, my idea around that would be to
have two separate sets of flags: one set for the bgworkers and one set
for PostgresInit() as an effect of load_session_libraries which looks
like a fuzzy concept for bgworkers.
--
Michael


signature.asc
Description: PGP signature

Re: Add a new BGWORKER_BYPASS_ROLELOGINCHECK flag

[Nathan Bossart]

On Thu, Oct 05, 2023 at 05:51:31PM +0530, Bharath Rupireddy wrote:
> I think changing BGWORKER_BYPASS_ALLOWCONN to 0x0001 and having bits32
> for InitPostgres inputs load_session_libraries and
> override_allow_connections can be 0001 in this patch series so that it
> can go first, no? This avoids the new code being in the old format and
> changing things right after it commits.

+1

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

Re: Add a new BGWORKER_BYPASS_ROLELOGINCHECK flag

[Bharath Rupireddy]

On Thu, Oct 5, 2023 at 9:32 PM Drouvot, Bertrand
 wrote:
>
> +  CREATE ROLE nologrole with nologin;
> +  GRANT CREATE ON DATABASE mydb TO nologrole;

A few nit-picks:

1. s/with/WITH
2. s/nologin/NOLOGIN
3. +   is specified as flags it is possible to
bypass the login check to connect to databases.
How about "it is possible to bypass the login check for the role used
to connect to databases."?

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

Re: Add a new BGWORKER_BYPASS_ROLELOGINCHECK flag

[Drouvot, Bertrand]

Hi,

On 10/5/23 2:21 PM, Bharath Rupireddy wrote:

On Thu, Oct 5, 2023 at 12:22 PM Drouvot, Bertrand
 wrote:



A comment on v6-0002:
1.
+  CREATE ROLE nologrole with nologin;
+  ALTER ROLE nologrole with superuser;
+]);
We don't need superuser privileges here, do we? Or do we need it for
the worker_spi to access pg_catalog and stuff in worker_spi_main? If
not, can we remove it to showcase non-superusers requesting bg
workers?


superuser is not needed here.
I removed it but had to change it in v7 attached to:

+  CREATE ROLE nologrole with nologin;
+  GRANT CREATE ON DATABASE mydb TO nologrole;

To avoid things like:

"
2023-10-05 15:59:39.189 UTC [2830732] LOG:  worker_spi dynamic worker 13 
initialized with schema13.counted
2023-10-05 15:59:39.191 UTC [2830732] ERROR:  permission denied for database 
mydb
2023-10-05 15:59:39.191 UTC [2830732] CONTEXT:  SQL statement "CREATE SCHEMA "schema13" 
CREATE TABLE "counted"
"

Regards,
 
--

Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.comFrom 6dd6cdb14646cb32f4aedc1905229a8b5c1d215c Mon Sep 17 00:00:00 2001
From: Michael Paquier 
Date: Thu, 5 Oct 2023 14:06:37 +0900
Subject: [PATCH v7 2/2] Allow background workers to bypass login check

This adds a new flag value (BGWORKER_BYPASS_ROLELOGINCHECK) to the flags
being used in BackgroundWorkerInitializeConnection() and
BackgroundWorkerInitializeConnectionByOid().  This flag allows the
background workers to bypass the login check done in
InitializeSessionUserId().
---
 doc/src/sgml/bgworker.sgml|  3 +-
 src/backend/bootstrap/bootstrap.c |  2 +-
 src/backend/postmaster/autovacuum.c   |  5 +--
 src/backend/postmaster/postmaster.c   |  2 +
 src/backend/tcop/postgres.c   |  1 +
 src/backend/utils/init/miscinit.c |  4 +-
 src/backend/utils/init/postinit.c |  5 ++-
 src/include/miscadmin.h   |  4 +-
 src/include/postmaster/bgworker.h |  6 ++-
 .../modules/worker_spi/t/001_worker_spi.pl| 37 +++
 src/test/modules/worker_spi/worker_spi.c  |  2 +
 11 files changed, 59 insertions(+), 12 deletions(-)
   9.1% doc/src/sgml/
  10.7% src/backend/postmaster/
  15.1% src/backend/utils/init/
   9.4% src/include/postmaster/
  47.5% src/test/modules/worker_spi/t/
   3.1% src/test/modules/worker_spi/
   4.8% src/

diff --git a/doc/src/sgml/bgworker.sgml b/doc/src/sgml/bgworker.sgml
index 9ad1146ba0..7335c02ed8 100644
--- a/doc/src/sgml/bgworker.sgml
+++ b/doc/src/sgml/bgworker.sgml
@@ -200,7 +200,8 @@ typedef struct BackgroundWorker
InvalidOid, the process will run as the superuser created
during initdb. If 
BGWORKER_BYPASS_ALLOWCONN
is specified as flags it is possible to bypass the 
restriction
-   to connect to databases not allowing user connections.
+   to connect to databases not allowing user connections. If 
BGWORKER_BYPASS_ROLELOGINCHECK
+   is specified as flags it is possible to bypass the login 
check to connect to databases.
A background worker can only call one of these two functions, and only
once.  It is not possible to switch databases.
   
diff --git a/src/backend/bootstrap/bootstrap.c 
b/src/backend/bootstrap/bootstrap.c
index 5810f8825e..bdd005b66a 100644
--- a/src/backend/bootstrap/bootstrap.c
+++ b/src/backend/bootstrap/bootstrap.c
@@ -345,7 +345,7 @@ BootstrapModeMain(int argc, char *argv[], bool check_only)
if (pg_link_canary_is_frontend())
elog(ERROR, "backend is incorrectly linked to frontend 
functions");
 
-   InitPostgres(NULL, InvalidOid, NULL, InvalidOid, false, false, NULL);
+   InitPostgres(NULL, InvalidOid, NULL, InvalidOid, false, false, false, 
NULL);
 
/* Initialize stuff for bootstrap-file processing */
for (i = 0; i < MAXATTR; i++)
diff --git a/src/backend/postmaster/autovacuum.c 
b/src/backend/postmaster/autovacuum.c
index ae9be9b911..4686b81f68 100644
--- a/src/backend/postmaster/autovacuum.c
+++ b/src/backend/postmaster/autovacuum.c
@@ -488,7 +488,7 @@ AutoVacLauncherMain(int argc, char *argv[])
/* Early initialization */
BaseInit();
 
-   InitPostgres(NULL, InvalidOid, NULL, InvalidOid, false, false, NULL);
+   InitPostgres(NULL, InvalidOid, NULL, InvalidOid, false, false, false, 
NULL);
 
SetProcessingMode(NormalProcessing);
 
@@ -1706,8 +1706,7 @@ AutoVacWorkerMain(int argc, char *argv[])
 * Note: if we have selected a just-deleted database (due to 
using
 * stale stats info), we'll fail and exit here.
 */
-   InitPostgres(NULL, dbid, NULL, InvalidOid, false, false,
-dbname);
+   InitPostgres(NULL, dbid, NULL, InvalidOid, false, false, false, 
dbname);
SetProcessingMode(NormalProcessing);
set_ps_display(dbname);
ereport(DEBUG1,

Re: Add a new BGWORKER_BYPASS_ROLELOGINCHECK flag

[Bharath Rupireddy]

On Thu, Oct 5, 2023 at 12:22 PM Drouvot, Bertrand
 wrote:
>
> >
> > @@ -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)

I think changing BGWORKER_BYPASS_ALLOWCONN to 0x0001 and having bits32
for InitPostgres inputs load_session_libraries and
override_allow_connections can be 0001 in this patch series so that it
can go first, no? This avoids the new code being in the old format and
changing things right after it commits.

v6-0001 LGTM.

A comment on v6-0002:
1.
+  CREATE ROLE nologrole with nologin;
+  ALTER ROLE nologrole with superuser;
+]);
We don't need superuser privileges here, do we? Or do we need it for
the worker_spi to access pg_catalog and stuff in worker_spi_main? If
not, can we remove it to showcase non-superusers requesting bg
workers?

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

Re: Add a new BGWORKER_BYPASS_ROLELOGINCHECK flag

[Drouvot, Bertrand]

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.comFrom 5ebe8aa15f22851ae7771137f58366ea9aa21087 Mon Sep 17 00:00:00 2001
From: Michael Paquier 
Date: Thu, 5 Oct 2023 14:06:37 +0900
Subject: [PATCH v6 2/2] Allow background workers to bypass login check

This adds a new flag value (BGWORKER_BYPASS_ROLELOGINCHECK) to the flags
being used in BackgroundWorkerInitializeConnection() and
BackgroundWorkerInitializeConnectionByOid().  This flag allows the
background workers to bypass the login check done in
InitializeSessionUserId().
---
 doc/src/sgml/bgworker.sgml|  3 +-
 src/backend/bootstrap/bootstrap.c |  2 +-
 src/backend/postmaster/autovacuum.c   |  5 +--
 src/backend/postmaster/postmaster.c   |  2 +
 src/backend/tcop/postgres.c   |  1 +
 src/backend/utils/init/miscinit.c |  4 +-
 src/backend/utils/init/postinit.c |  5 ++-
 src/include/miscadmin.h   |  4 +-
 src/include/postmaster/bgworker.h |  6 ++-
 .../modules/worker_spi/t/001_worker_spi.pl| 37 +++
 src/test/modules/worker_spi/worker_spi.c  |  2 +
 11 files changed, 59 insertions(+), 12 deletions(-)
   9.2% doc/src/sgml/
  10.7% src/backend/postmaster/
  15.1% src/backend/utils/init/
   9.5% src/include/postmaster/
  47.4% src/test/modules/worker_spi/t/
   3.1% src/test/modules/worker_spi/
   4.8% src/

diff --git a/doc/src/sgml/bgworker.sgml b/doc/src/sgml/bgworker.sgml
index 9ad1146ba0..7335c02ed8 100644
--- a/doc/src/sgml/bgworker.sgml
+++ b/doc/src/sgml/bgworker.sgml
@@ -200,7 +200,8 @@ typedef struct BackgroundWorker
InvalidOid, the process will run as the superuser created
during initdb. If 
BGWORKER_BYPASS_ALLOWCONN
is specified as flags it is possible to bypass the 
restriction
-   to connect to databases not allowing user connections.
+   to connect to databases not allowing user connections. If 

Re: Add a new BGWORKER_BYPASS_ROLELOGINCHECK flag

[Michael Paquier]

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
From fe8373992d2e2083d53e75ecd954ba2b71e454a1 Mon Sep 17 00:00:00 2001
From: Michael Paquier 
Date: Thu, 5 Oct 2023 13:02:14 +0900
Subject: [PATCH v5 1/2] worker_spi: Add tests for BGWORKER_BYPASS_ALLOWCONN

---
 .../modules/worker_spi/t/001_worker_spi.pl| 25 +++
 1 file changed, 25 insertions(+)

diff --git a/src/test/modules/worker_spi/t/001_worker_spi.pl b/src/test/modules/worker_spi/t/001_worker_spi.pl
index 4b46b1336b..7e5a4b1402 100644
--- a/src/test/modules/worker_spi/t/001_worker_spi.pl
+++ b/src/test/modules/worker_spi/t/001_worker_spi.pl
@@ -104,4 +104,29 @@ postgres|myrole|WorkerSpiMain]),
 	'dynamic bgworkers all launched'
 ) or die "Timed out while waiting for dynamic bgworkers to be launched";
 
+# Check BGWORKER_BYPASS_ALLOWCONN.
+$node->safe_psql('postgres', q(ALTER DATABASE mydb ALLOW_CONNECTIONS false;));
+my $log_offset = -s $node->logfile;
+
+# bgworker cannot be launched with connection restriction.
+my $worker3_pid = $node->safe_psql('postgres',
+	qq[SELECT worker_spi_launch(12, $mydb_id, $myrole_id);]);
+$node->wait_for_log(
+	qr/database "mydb" is not currently accepting connections/, $log_offset);
+
+$result = $node->safe_psql('postgres',
+	"SELECT count(*) FROM pg_stat_activity WHERE pid = $worker3_pid;");
+is($result, '0', 'dynamic bgworker without BYPASS_ALLOWCONN not started');
+
+# bgworker bypasses the connection check, and can be launched.
+my $worker4_pid = $node->safe_psql('postgres',
+	qq[SELECT worker_spi_launch(12, $mydb_id, $myrole_id, '{"ALLOWCONN"}');]);
+ok( $node->poll_query_until(
+		'postgres',
+		qq[SELECT datname, usename, wait_event FROM pg_stat_activity
+WHERE backend_type = 'worker_spi dynamic' AND
+pid IN ($worker4_pid) ORDER BY datname;],
+		qq[mydb|myrole|WorkerSpiMain]),
+	'dynamic bgworker with BYPASS_ALLOWCONN started');
+
 done_testing();
-- 
2.42.0

From 59b1e8827c20a3e801e64b3b089efb3d06d71f42 Mon Sep 17 00:00:00 2001
From: Michael Paquier 
Date: Thu, 5 Oct 2023 14:06:37 +0900
Subject: [PATCH v5 2/2] Allow background workers to bypass login check

This adds a new flag value (BGWORKER_BYPASS_ROLELOGINCHECK) to the flags
being used in BackgroundWorkerInitializeConnection() and
BackgroundWorkerInitializeConnectionByOid().  This flag allows the
background workers to bypass the login check done in
InitializeSessionUserId().
---
 src/include/miscadmin.h   |  4 +-
 src/include/postmaster/bgworker.h | 10 +++--
 src/backend/bootstrap/bootstrap.c

Re: Add a new BGWORKER_BYPASS_ROLELOGINCHECK flag

[Drouvot, Bertrand]

Hi,

On 10/4/23 8:20 AM, Michael Paquier wrote:

On Mon, Oct 02, 2023 at 10:53:22AM +0200, Drouvot, Bertrand wrote:

On 10/2/23 10:17 AM, Michael Paquier wrote:

On Mon, Oct 02, 2023 at 10:01:04AM +0200, Drouvot, Bertrand wrote:

I think that would make sense to have more flexibility in the worker_spi
module. I think that could be done in a dedicated patch though. I
think it makes more sense to have the current patch "focusing" on
this new flag (while adding a test about it without too much
refactoring). What about doing the worker_spi module  re-factoring
as a follow up of this one?


I would do that first, as that's what I usually do,


The reason I was thinking not doing that first is that there is no real use
case in the current worker_spi module test.


As a template, improving and extending it seems worth it to me as long
as it can also improve tests.


but I see also
your point that this is not mandatory.  If you want, I could give it a
shot tomorrow to see where it leads.


Oh yeah that would be great (and maybe you already see a use case in the
current test). Thanks!


Yes, while it shows a bit more what one can achieve with bgw_extra, it
also helps in improving the test coverage with starting dynamic
workers across defined databases and roles through a SQL function.



Yeah right.

It took me a bit longer than I expected, 


Thanks for having looked at it!


but here is what I finish
with:
- 0001 extends worker_spi to be able to pass down database and role
IDs for the workers to start, through MyBgworkerEntry->bgw_extra.
Perhaps the two new arguments of worker_spi_launch() should use
InvalidOid as default, actually, to fall back to the database and
roles defined by the GUCs in these cases. 


I'm fine with the way it's currently done in 0001 and that sounds
more logical to me. I mean we don't "really" want InvalidOid but to fall
back to the GUCs.

Just a remark here:

+   if (!OidIsValid(roleoid))
+   {
+   /*
+* worker_spi_role is NULL by default, so just pass down an 
invalid
+* OID to let the main() routine do its connection work.
+*/
+   if (worker_spi_role)
+   roleoid = get_role_oid(worker_spi_role, false);
+   else
+   roleoid = InvalidOid;

the

+   else
+   roleoid = InvalidOid;

I think it is not needed as we're already in "!OidIsValid(roleoid)".


- 0002 is your patch, on top of which I have added handling for the
flags in the launch() function with a text[].  The tests get much
simpler, and don't need restarts.



Yeah, agree that's better.


By the way, I am pretty sure that we are going to need a wait phase
after the startup of the worker in the case where "nologrole" is not
allowed to log in even with the original patch: the worker may not
have started at the point where we check the logs.


I agree and it's now failing on my side.
I added this "wait" in v4-0002 attach and it's now working fine.

Please note there is more changes than adding this wait in 001_worker_spi.pl 
(as compare
to v3-0002) as I ran pgperltidy on it.
FWIW, the new "wait" is just the part related to "nb_errors".


What do you think?


Except the Nit that I mentioned in 0001, that looks all good to me (with the
new wait in 001_worker_spi.pl).

Regards,

--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.comFrom 64465c2b2d54ba1c316efe347f346687415d01e0 Mon Sep 17 00:00:00 2001
From: Michael Paquier 
Date: Wed, 4 Oct 2023 15:06:25 +0900
Subject: [PATCH v4 2/2] Allow background workers to bypass login check

This adds a new flag value (BGWORKER_BYPASS_ROLELOGINCHECK) to the flags
being used in BackgroundWorkerInitializeConnection() and
BackgroundWorkerInitializeConnectionByOid().  This flag allows the
background workers to bypass the login check done in
InitializeSessionUserId().
---
 doc/src/sgml/bgworker.sgml|  3 +-
 src/backend/bootstrap/bootstrap.c |  2 +-
 src/backend/postmaster/autovacuum.c   |  5 +-
 src/backend/postmaster/postmaster.c   |  6 +-
 src/backend/tcop/postgres.c   |  1 +
 src/backend/utils/init/miscinit.c |  4 +-
 src/backend/utils/init/postinit.c |  5 +-
 src/include/miscadmin.h   |  4 +-
 src/include/postmaster/bgworker.h | 10 +--
 .../modules/worker_spi/t/001_worker_spi.pl| 68 +--
 .../modules/worker_spi/worker_spi--1.0.sql|  3 +-
 src/test/modules/worker_spi/worker_spi.c  | 49 +++--
 12 files changed, 134 insertions(+), 26 deletions(-)
   4.6% doc/src/sgml/
   6.8% src/backend/postmaster/
   7.6% src/backend/utils/init/
   7.1% src/include/postmaster/
  41.4% src/test/modules/worker_spi/t/
  29.8% src/test/modules/worker_spi/

diff --git a/doc/src/sgml/bgworker.sgml b/doc/src/sgml/bgworker.sgml
index 

Re: Add a new BGWORKER_BYPASS_ROLELOGINCHECK flag

[Michael Paquier]

On Mon, Oct 02, 2023 at 10:53:22AM +0200, Drouvot, Bertrand wrote:
> On 10/2/23 10:17 AM, Michael Paquier wrote:
>> On Mon, Oct 02, 2023 at 10:01:04AM +0200, Drouvot, Bertrand wrote:
>>> I think that would make sense to have more flexibility in the worker_spi
>>> module. I think that could be done in a dedicated patch though. I
>>> think it makes more sense to have the current patch "focusing" on
>>> this new flag (while adding a test about it without too much
>>> refactoring). What about doing the worker_spi module  re-factoring
>>> as a follow up of this one?
>> 
>> I would do that first, as that's what I usually do,
> 
> The reason I was thinking not doing that first is that there is no real use
> case in the current worker_spi module test.

As a template, improving and extending it seems worth it to me as long
as it can also improve tests.

> > but I see also
> > your point that this is not mandatory.  If you want, I could give it a
> > shot tomorrow to see where it leads.
> 
> Oh yeah that would be great (and maybe you already see a use case in the
> current test). Thanks!

Yes, while it shows a bit more what one can achieve with bgw_extra, it
also helps in improving the test coverage with starting dynamic
workers across defined databases and roles through a SQL function.

It took me a bit longer than I expected, but here is what I finish
with:
- 0001 extends worker_spi to be able to pass down database and role
IDs for the workers to start, through MyBgworkerEntry->bgw_extra.
Perhaps the two new arguments of worker_spi_launch() should use
InvalidOid as default, actually, to fall back to the database and
roles defined by the GUCs in these cases.  That would be two lines to
change in worker_spi--1.0.sql.
- 0002 is your patch, on top of which I have added handling for the
flags in the launch() function with a text[].  The tests get much
simpler, and don't need restarts.

By the way, I am pretty sure that we are going to need a wait phase
after the startup of the worker in the case where "nologrole" is not
allowed to log in even with the original patch: the worker may not
have started at the point where we check the logs.  That's true as
well when involving worker_spi_launch().

What do you think?
--
Michael
From b37dd684da3ff6f9c00719c27b89f1e39f6961a4 Mon Sep 17 00:00:00 2001
From: Michael Paquier 
Date: Wed, 4 Oct 2023 14:28:20 +0900
Subject: [PATCH v3 1/2] Redesign database and role handling in worker_spi

worker_spi_launch gains two arguments for a database ID and a role ID.
If these are InvalidOid, the dynamic worker falls back to the GUCs
defined to launch the workers.  Tests are refactored in consequence.

bgw_extra is used to pass down these two OIDs to the main bgworker
routine.  Workers loaded with shared_preload_libraries just rely on the
GUCs.
---
 .../modules/worker_spi/t/001_worker_spi.pl| 21 +---
 .../modules/worker_spi/worker_spi--1.0.sql|  2 +-
 src/test/modules/worker_spi/worker_spi.c  | 54 ++-
 3 files changed, 67 insertions(+), 10 deletions(-)

diff --git a/src/test/modules/worker_spi/t/001_worker_spi.pl b/src/test/modules/worker_spi/t/001_worker_spi.pl
index 2965acd789..a026042c65 100644
--- a/src/test/modules/worker_spi/t/001_worker_spi.pl
+++ b/src/test/modules/worker_spi/t/001_worker_spi.pl
@@ -20,9 +20,11 @@ $node->safe_psql('postgres', 'CREATE EXTENSION worker_spi;');
 # This consists in making sure that a table name "counted" is created
 # on a new schema whose name includes the index defined in input argument
 # of worker_spi_launch().
-# By default, dynamic bgworkers connect to the "postgres" database.
+# By default, dynamic bgworkers connect to the "postgres" database with
+# an undefined role, falling back to the GUC defaults (InvalidOid for
+# worker_spi_launch).
 my $result =
-  $node->safe_psql('postgres', 'SELECT worker_spi_launch(4) IS NOT NULL;');
+  $node->safe_psql('postgres', 'SELECT worker_spi_launch(4, 0, 0) IS NOT NULL;');
 is($result, 't', "dynamic bgworker launched");
 $node->poll_query_until(
 	'postgres',
@@ -58,6 +60,7 @@ note "testing bgworkers loaded with shared_preload_libraries";
 # Create the database first so as the workers can connect to it when
 # the library is loaded.
 $node->safe_psql('postgres', q(CREATE DATABASE mydb;));
+$node->safe_psql('postgres', q(CREATE ROLE myrole SUPERUSER LOGIN;));
 $node->safe_psql('mydb', 'CREATE EXTENSION worker_spi;');
 
 # Now load the module as a shared library.
@@ -80,16 +83,18 @@ ok( $node->poll_query_until(
 
 # Ask worker_spi to launch dynamic bgworkers with the library loaded, then
 # check their existence.  Use IDs that do not overlap with the schemas created
-# by the previous workers.
-my $worker1_pid = $node->safe_psql('mydb', 'SELECT worker_spi_launch(10);');
-my $worker2_pid = $node->safe_psql('mydb', 'SELECT worker_spi_launch(11);');
+# by the previous workers.  These use a specific role.
+my $myrole_id = $node->safe_psql('mydb', "SELECT oid FROM pg_roles where rolname 

Re: Add a new BGWORKER_BYPASS_ROLELOGINCHECK flag

[Michael Paquier]

On Tue, Oct 03, 2023 at 07:02:11PM +0530, Bharath Rupireddy wrote:
> On Tue, Oct 3, 2023 at 5:45 PM Drouvot, Bertrand
>  wrote:
>> 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.
>> "
> 
> It doesn't have to be always/all the time. If the case here is okay to
> change the bgw and other core functions API, I honestly feel that we
> must move BGWORKER_BYPASS_ALLOWCONN to bgw_flags.

I don't agree with this point.  BackgroundWorkerInitializeConnection()
and its other friend are designed to be called at the beginning of the
main routine of a background worker, where bgw_flags is not accessible.
There is much more happening before a bgworker initializes its
connection, like signal handling and definitions of other states that
depend on the GUCs loaded for the bgworker.

>> 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.
> 
> I look at the new flag as a capability of the bgw to connect with a
> role without login access. IMV, all are the same.

Bertrand is arguing that the current code with its current split is
OK, because both are different concepts:
- bgw_flags is used by the postmaster to control how to launch the
bgworkers.
- The BGWORKER_* flags are used by the bgworkers themselves, once
things are set up by the postmaster based on bgw_flags.
--
Michael


signature.asc
Description: PGP signature

Re: Add a new BGWORKER_BYPASS_ROLELOGINCHECK flag

[Bharath Rupireddy]

On Tue, Oct 3, 2023 at 5:45 PM Drouvot, Bertrand
 wrote:
>
> > 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.
> "

It doesn't have to be always/all the time. If the case here is okay to
change the bgw and other core functions API, I honestly feel that we
must move BGWORKER_BYPASS_ALLOWCONN to bgw_flags.

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

I look at the new flag as a capability of the bgw to connect with a
role without login access. IMV, all are the same.

> 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

I prefer to have it as bgw_flag, however, let's hear from others.

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

Re: Add a new BGWORKER_BYPASS_ROLELOGINCHECK flag

[Drouvot, Bertrand]

Hi,

On 10/3/23 11:21 AM, Bharath Rupireddy wrote:

On Mon, Oct 2, 2023 at 4:58 PM Drouvot, Bertrand
 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

Re: Add a new BGWORKER_BYPASS_ROLELOGINCHECK flag

[Bharath Rupireddy]

On Mon, Oct 2, 2023 at 4:58 PM Drouvot, Bertrand
 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?

[1]
diff --git a/src/backend/utils/init/miscinit.c
b/src/backend/utils/init/miscinit.c
index 1e671c560c..27dcf052ab 100644
--- a/src/backend/utils/init/miscinit.c
+++ b/src/backend/utils/init/miscinit.c
@@ -786,10 +786,17 @@ InitializeSessionUserId(const char *rolename, Oid roleid)
 */
if (IsUnderPostmaster)
{
+   boolskip_check = false;
+
+   /* If asked, skip the role login check for background
workers. */
+   if (IsBackgroundWorker &&
+   (MyBgworkerEntry->bgw_flags &
BGWORKER_BYPASS_ROLELOGINCHECK) != 0)
+   skip_check = true;
+
/*
 * Is role allowed to login at all?
 */
-   if (!rform->rolcanlogin)
+   if (!skip_check && !rform->rolcanlogin)
ereport(FATAL,

(errcode(ERRCODE_INVALID_AUTHORIZATION_SPECIFICATION),
 errmsg("role \"%s\" is not
permitted to log in",

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

Re: Add a new BGWORKER_BYPASS_ROLELOGINCHECK flag

[Drouvot, Bertrand]

Hi,

On 10/2/23 10:17 AM, Michael Paquier wrote:

On Mon, Oct 02, 2023 at 10:01:04AM +0200, Drouvot, Bertrand wrote:

I think that would make sense to have more flexibility in the worker_spi
module. I think that could be done in a dedicated patch though. I
think it makes more sense to have the current patch "focusing" on
this new flag (while adding a test about it without too much
refactoring). What about doing the worker_spi module  re-factoring
as a follow up of this one?


I would do that first, as that's what I usually do,


The reason I was thinking not doing that first is that there is no real use
case in the current worker_spi module test.


but I see also
your point that this is not mandatory.  If you want, I could give it a
shot tomorrow to see where it leads.


Oh yeah that would be great (and maybe you already see a use case in the
current test). Thanks!


Oh right, worth to modify 019_replslot_limit.pl, 002_corrupted.pl and
001_pg_controldata.pl in a separate patch for consistency? (they are using
"(stat $node->logfile)[7]" or "(stat($pg_control))[7]").


Indeed, that's strange.  Let's remove the dependency to stat here.
The other solution is slightly more elegant IMO, as we don't rely on
the position of the result from stat().


Agree, I will propose a new patch for this.

Regards,

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

Re: Add a new BGWORKER_BYPASS_ROLELOGINCHECK flag

[Michael Paquier]

On Mon, Oct 02, 2023 at 10:01:04AM +0200, Drouvot, Bertrand wrote:
> I think that would make sense to have more flexibility in the worker_spi
> module. I think that could be done in a dedicated patch though. I
> think it makes more sense to have the current patch "focusing" on
> this new flag (while adding a test about it without too much
> refactoring). What about doing the worker_spi module  re-factoring
> as a follow up of this one?

I would do that first, as that's what I usually do, but I see also
your point that this is not mandatory.  If you want, I could give it a
shot tomorrow to see where it leads.

> Oh right, worth to modify 019_replslot_limit.pl, 002_corrupted.pl and
> 001_pg_controldata.pl in a separate patch for consistency? (they are using
> "(stat $node->logfile)[7]" or "(stat($pg_control))[7]").

Indeed, that's strange.  Let's remove the dependency to stat here.
The other solution is slightly more elegant IMO, as we don't rely on
the position of the result from stat().
--
Michael


signature.asc
Description: PGP signature

Re: Add a new BGWORKER_BYPASS_ROLELOGINCHECK flag

[Drouvot, Bertrand]

Hi,

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.


Thanks for looking at it!




+uint32 flags,
 char *out_dbname)
  {


This may be more adapted with a bits32 for the flags.


Done that way in v2 attached.




+# Ask the background workers to connect with this role with the flag in place.
+$node->append_conf(
+'postgresql.conf', q{
+worker_spi.role = 'nologrole'
+worker_spi.bypass_login_check = true
+});
+$node->restart;
+
+# An error message should not be issued.
+ok( !$node->log_contains(
+"role \"nologrole\" is not permitted to log in", $log_start),
+"nologrole allowed to connect if BGWORKER_BYPASS_ROLELOGINCHECK is set");
+
  done_testing();


It would be cheaper to use a dynamic background worker for such tests.
Something that I've been tempted to do in this module is to extend the
amount of data that's given to bgw_main_arg when launching a worker
with worker_spi_launch().  How about extending the SQL function so as
it is possible to give in input a role name (or a regrole), a database
name (or a database OID) and a text[] for the flags?  This would
require a bit more refactoring, but this would be benefitial to show
or one can pass down a full structure from the registration to the
main() routine.  On top of that, it would make the addition of the new
GUCs worker_spi.bypass_login_check and worker_spi.role unnecessary.



I think that would make sense to have more flexibility in the worker_spi
module. I think that could be done in a dedicated patch though. I think it makes
more sense to have the current patch "focusing" on this new flag (while adding 
a test
about it without too much refactoring). What about doing the worker_spi module
re-factoring as a follow up of this one?


+# return the size of logfile of $node in bytes
+sub get_log_size
+{
+my ($node) = @_;
+
+return (stat $node->logfile)[7];
+}


Just use -s here.


Done in v2 attached.


See other tests that want to check the contents of
the logs from an offset.



Oh right, worth to modify 019_replslot_limit.pl, 002_corrupted.pl and
001_pg_controldata.pl in a separate patch for consistency? (they are using
"(stat $node->logfile)[7]" or "(stat($pg_control))[7]").


- * Allow bypassing datallowconn restrictions when connecting to database
+ * Allow bypassing datallowconn restrictions and login check when connecting
+ * to database
   */
-#define BGWORKER_BYPASS_ALLOWCONN 1
+#define BGWORKER_BYPASS_ALLOWCONN 0x0001
+#define BGWORKER_BYPASS_ROLELOGINCHECK 0x0002


The structure of the patch is inconsistent.  These flags are in
bgworker.h, but they are used also by InitPostgres().  Perhaps a
second boolean flag would be OK rather than a second set of flags for
InitPostgres() mapping with the bgworker set.


I did not want initially to add an extra parameter to InitPostgres() but
I agree it would make more sense. So done that way in v2.

Regards,

--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.comFrom 59de41f3c305b009babfa6d05c054d8ae9e4d730 Mon Sep 17 00:00:00 2001
From: Bertrand Drouvot 
Date: Wed, 20 Sep 2023 08:28:59 +
Subject: [PATCH v2] Allow background workers to bypass login check

This adds a new flag value (BGWORKER_BYPASS_ROLELOGINCHECK) to the flags being
used in BackgroundWorkerInitializeConnection() and 
BackgroundWorkerInitializeConnectionByOid().

This flag allows the background workers to bypass the login check done in 
InitializeSessionUserId().
---
 doc/src/sgml/bgworker.sgml|  3 +-
 src/backend/bootstrap/bootstrap.c |  2 +-
 src/backend/postmaster/autovacuum.c   |  5 +--
 src/backend/postmaster/postmaster.c   |  6 ++-
 src/backend/tcop/postgres.c   |  1 +
 src/backend/utils/init/miscinit.c |  4 +-
 src/backend/utils/init/postinit.c |  5 ++-
 src/include/miscadmin.h   |  4 +-
 src/include/postmaster/bgworker.h | 10 +++--
 .../modules/worker_spi/t/001_worker_spi.pl| 40 +++
 src/test/modules/worker_spi/worker_spi.c  | 25 +++-
 11 files changed, 88 insertions(+), 17 deletions(-)
   7.6% doc/src/sgml/
  11.4% src/backend/postmaster/
  12.6% src/backend/utils/init/
  11.8% src/include/postmaster/
  29.2% src/test/modules/worker_spi/t/
  23.2% src/test/modules/worker_spi/
   4.0% src/

diff --git a/doc/src/sgml/bgworker.sgml b/doc/src/sgml/bgworker.sgml
index 9ad1146ba0..7335c02ed8 100644
--- a/doc/src/sgml/bgworker.sgml
+++ b/doc/src/sgml/bgworker.sgml
@@ -200,7 +200,8 @@ typedef struct BackgroundWorker
InvalidOid, the process will run as the superuser 

Re: Add a new BGWORKER_BYPASS_ROLELOGINCHECK flag

[Michael Paquier]

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.

> +  uint32 flags,
>char *out_dbname)
>  {

This may be more adapted with a bits32 for the flags.

> +# Ask the background workers to connect with this role with the flag in 
> place.
> +$node->append_conf(
> +'postgresql.conf', q{
> +worker_spi.role = 'nologrole'
> +worker_spi.bypass_login_check = true
> +});
> +$node->restart;
> +
> +# An error message should not be issued.
> +ok( !$node->log_contains(
> +"role \"nologrole\" is not permitted to log in", $log_start),
> +"nologrole allowed to connect if BGWORKER_BYPASS_ROLELOGINCHECK is set");
> +
>  done_testing();

It would be cheaper to use a dynamic background worker for such tests.
Something that I've been tempted to do in this module is to extend the
amount of data that's given to bgw_main_arg when launching a worker
with worker_spi_launch().  How about extending the SQL function so as
it is possible to give in input a role name (or a regrole), a database
name (or a database OID) and a text[] for the flags?  This would
require a bit more refactoring, but this would be benefitial to show
or one can pass down a full structure from the registration to the
main() routine.  On top of that, it would make the addition of the new
GUCs worker_spi.bypass_login_check and worker_spi.role unnecessary.

> +# return the size of logfile of $node in bytes
> +sub get_log_size
> +{
> +my ($node) = @_;
> +
> +return (stat $node->logfile)[7];
> +}

Just use -s here.  See other tests that want to check the contents of
the logs from an offset.

> - * Allow bypassing datallowconn restrictions when connecting to database
> + * Allow bypassing datallowconn restrictions and login check when connecting
> + * to database
>   */
> -#define BGWORKER_BYPASS_ALLOWCONN 1
> +#define BGWORKER_BYPASS_ALLOWCONN 0x0001
> +#define BGWORKER_BYPASS_ROLELOGINCHECK 0x0002

The structure of the patch is inconsistent.  These flags are in
bgworker.h, but they are used also by InitPostgres().  Perhaps a
second boolean flag would be OK rather than a second set of flags for
InitPostgres() mapping with the bgworker set.
--
Michael


signature.asc
Description: PGP signature

Add a new BGWORKER_BYPASS_ROLELOGINCHECK flag

[Drouvot, Bertrand]

Hi hackers,

Please find attached a patch proposal to $SUBJECT.

This patch allows the role provided in BackgroundWorkerInitializeConnection()
and BackgroundWorkerInitializeConnectionByOid() to lack login authorization.

In InitPostgres(), in case of a background worker, authentication is not 
performed
(PerformAuthentication() is not called), so having the role used to connect to 
the database
lacking login authorization seems to make sense.

With this new flag in place, one could give "high" privileges to the role used 
to initialize
the background workers connections without any risk of seeing this role being 
used by a
"normal user" to login.

The attached patch:

- adds the new flag
- adds documentation
- adds testing

Looking forward to your feedback,

Regards,

--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.comFrom 82ea53a81a5e9f5e8b680ec6de849a48a7222463 Mon Sep 17 00:00:00 2001
From: Bertrand Drouvot 
Date: Wed, 20 Sep 2023 08:28:59 +
Subject: [PATCH v1] Allow background workers to bypass login check

This adds a new flag value (BGWORKER_BYPASS_ROLELOGINCHECK) to the flags being
used in BackgroundWorkerInitializeConnection() and 
BackgroundWorkerInitializeConnectionByOid().

This flag allows the background workers to bypass the login check done in 
InitializeSessionUserId().
---
 doc/src/sgml/bgworker.sgml|  3 +-
 src/backend/bootstrap/bootstrap.c |  2 +-
 src/backend/postmaster/autovacuum.c   |  4 +-
 src/backend/postmaster/postmaster.c   |  4 +-
 src/backend/tcop/postgres.c   |  2 +-
 src/backend/utils/init/miscinit.c |  4 +-
 src/backend/utils/init/postinit.c |  9 ++--
 src/include/miscadmin.h   |  5 +-
 src/include/postmaster/bgworker.h |  6 ++-
 .../modules/worker_spi/t/001_worker_spi.pl| 48 +++
 src/test/modules/worker_spi/worker_spi.c  | 25 +-
 11 files changed, 95 insertions(+), 17 deletions(-)
   6.9% doc/src/sgml/
   3.4% src/backend/bootstrap/
  10.6% src/backend/postmaster/
  16.8% src/backend/utils/init/
   7.2% src/include/postmaster/
  29.7% src/test/modules/worker_spi/t/
  21.1% src/test/modules/worker_spi/
   4.0% src/

diff --git a/doc/src/sgml/bgworker.sgml b/doc/src/sgml/bgworker.sgml
index 9ad1146ba0..7335c02ed8 100644
--- a/doc/src/sgml/bgworker.sgml
+++ b/doc/src/sgml/bgworker.sgml
@@ -200,7 +200,8 @@ typedef struct BackgroundWorker
InvalidOid, the process will run as the superuser created
during initdb. If 
BGWORKER_BYPASS_ALLOWCONN
is specified as flags it is possible to bypass the 
restriction
-   to connect to databases not allowing user connections.
+   to connect to databases not allowing user connections. If 
BGWORKER_BYPASS_ROLELOGINCHECK
+   is specified as flags it is possible to bypass the login 
check to connect to databases.
A background worker can only call one of these two functions, and only
once.  It is not possible to switch databases.
   
diff --git a/src/backend/bootstrap/bootstrap.c 
b/src/backend/bootstrap/bootstrap.c
index 5810f8825e..d4b6425585 100644
--- a/src/backend/bootstrap/bootstrap.c
+++ b/src/backend/bootstrap/bootstrap.c
@@ -345,7 +345,7 @@ BootstrapModeMain(int argc, char *argv[], bool check_only)
if (pg_link_canary_is_frontend())
elog(ERROR, "backend is incorrectly linked to frontend 
functions");
 
-   InitPostgres(NULL, InvalidOid, NULL, InvalidOid, false, false, NULL);
+   InitPostgres(NULL, InvalidOid, NULL, InvalidOid, false, 0, NULL);
 
/* Initialize stuff for bootstrap-file processing */
for (i = 0; i < MAXATTR; i++)
diff --git a/src/backend/postmaster/autovacuum.c 
b/src/backend/postmaster/autovacuum.c
index ae9be9b911..dde83e76fb 100644
--- a/src/backend/postmaster/autovacuum.c
+++ b/src/backend/postmaster/autovacuum.c
@@ -488,7 +488,7 @@ AutoVacLauncherMain(int argc, char *argv[])
/* Early initialization */
BaseInit();
 
-   InitPostgres(NULL, InvalidOid, NULL, InvalidOid, false, false, NULL);
+   InitPostgres(NULL, InvalidOid, NULL, InvalidOid, false, 0, NULL);
 
SetProcessingMode(NormalProcessing);
 
@@ -1706,7 +1706,7 @@ AutoVacWorkerMain(int argc, char *argv[])
 * Note: if we have selected a just-deleted database (due to 
using
 * stale stats info), we'll fail and exit here.
 */
-   InitPostgres(NULL, dbid, NULL, InvalidOid, false, false,
+   InitPostgres(NULL, dbid, NULL, InvalidOid, false, 0,
 dbname);
SetProcessingMode(NormalProcessing);
set_ps_display(dbname);
diff --git a/src/backend/postmaster/postmaster.c 
b/src/backend/postmaster/postmaster.c
index 54e9bfb8c4..443b1c4725 100644
--- a/src/backend/postmaster/postmaster.c
+++