Add a new BGWORKER_BYPASS_ROLELOGINCHECK flag

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

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

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

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

On Mon, Oct 2, 2023 at 4:58 PM Drouvot, Bertrand
<bertranddrouvot.pg@gmail.com> wrote:

On 9/29/23 8:19 AM, Michael Paquier wrote:

On Thu, Sep 28, 2023 at 02:37:02PM +0200, Drouvot, Bertrand wrote:

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

Interesting. Yes, there would be use cases for that, I suppose.

Correct. It allows the roles that don't have LOGIN capabilities to
start and use bg workers.

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

Done that way in v2 attached.

While I like the idea of the flag to skip login checks for bg workers,
I don't quite like the APIs being changes InitializeSessionUserId and
InitPostgres (adding a new input parameter),
BackgroundWorkerInitializeConnection and
BackgroundWorkerInitializeConnectionByOid (changing of input parameter
type) given that all of these functions are available for external
modules and will break things for sure.

What if BGWORKER_BYPASS_ROLELOGINCHECK be part of bgw_flags? With
this, none of the API needs to be changed, so no compatibility
problems as such for external modules and the InitializeSessionUserId
can just do something like [1]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) { + bool skip_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,. 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)
        {
+               bool    skip_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

Hi,

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

On Mon, Oct 2, 2023 at 4:58 PM Drouvot, Bertrand
<bertranddrouvot.pg@gmail.com> wrote:

On 9/29/23 8:19 AM, Michael Paquier wrote:

On Thu, Sep 28, 2023 at 02:37:02PM +0200, Drouvot, Bertrand wrote:

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

Interesting. Yes, there would be use cases for that, I suppose.

Correct. It allows the roles that don't have LOGIN capabilities to
start and use bg workers.

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

Done that way in v2 attached.

While I like the idea of the flag to skip login checks for bg workers,
I don't quite like the APIs being changes InitializeSessionUserId and
InitPostgres (adding a new input parameter),
BackgroundWorkerInitializeConnection and
BackgroundWorkerInitializeConnectionByOid (changing of input parameter
type) given that all of these functions are available for external
modules and will break things for sure.

What if BGWORKER_BYPASS_ROLELOGINCHECK be part of bgw_flags? With
this, none of the API needs to be changed, so no compatibility
problems as such for external modules and the InitializeSessionUserId
can just do something like [1]. We might be tempted to add
BGWORKER_BYPASS_ALLOWCONN also to bgw_flags, but I'd prefer not to do
it for the same compatibility reasons.

Thoughts?

Thanks for looking at it!

I did some research and BGWORKER_BYPASS_ALLOWCONN has been added in eed1ce72e1 and
at that time the bgw_flags did already exist.

In this the related thread [1]/messages/by-id/22769.1519323861@sss.pgh.pa.us, 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]: /messages/by-id/22769.1519323861@sss.pgh.pa.us

Regards,

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

On Tue, Oct 3, 2023 at 5:45 PM Drouvot, Bertrand
<bertranddrouvot.pg@gmail.com> 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]: /messages/by-id/22769.1519323861@sss.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

On Tue, Oct 03, 2023 at 07:02:11PM +0530, Bharath Rupireddy wrote:

On Tue, Oct 3, 2023 at 5:45 PM Drouvot, Bertrand
<bertranddrouvot.pg@gmail.com> 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

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

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

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

Hi,

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

On Wed, Oct 04, 2023 at 12:54:24PM +0200, Drouvot, Bertrand wrote:

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

Thanks, I've applied the refactoring, including in it the stuff to be
able to control the flags used when launching a dynamic worker. 0001
also needed some indenting. You'll notice that the diffs in
worker_spi are minimal now. worker_spi_main is no more, as an effect
of.. Cough.. c8e318b1b.

Thanks!

+# An error message should be issued.
+my $nb_errors = 0;
+for (my $i = 0; $i < 10 * $PostgreSQL::Test::Utils::timeout_default; $i++)
+{
+	if ($node->log_contains(
+			"role \"nologrole\" is not permitted to log in", $log_start))
+	{
+		$nb_errors = 1;
+		last;
+	}
+	usleep(100_000);
+}

This can be switched to use $node->wait_for_log, making the test
simpler. No need for Time::HiRes, either.

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

-extern void BackgroundWorkerInitializeConnection(const char *dbname,
const char *username, uint32 flags);
+extern void BackgroundWorkerInitializeConnection(const char *dbname,
const char *username, bits32 flags);
[...]
-BackgroundWorkerInitializeConnectionByOid(Oid dboid, Oid useroid,
uint32 flags)
+BackgroundWorkerInitializeConnectionByOid(Oid dboid, Oid useroid,
bits32 flags)

That's changing signatures just for the sake of breaking them. I
would leave that alone, I guess..

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

@@ -719,6 +719,7 @@ InitPostgres(const char *in_dbname, Oid dboid,
const char *username, Oid useroid,
bool load_session_libraries,
bool override_allow_connections,
+ bool bypass_login_check,
char *out_dbname)

I was not paying much attention here, but load_session_libraries gives
a good argument in favor of switching all these booleans to a single
bits32 argument as that would make three of them now, with a different
set of flags than the bgworker ones. This can be refactored on its
own.

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

-#define BGWORKER_BYPASS_ALLOWCONN 1
+#define BGWORKER_BYPASS_ALLOWCONN 0x0001

Can be a change of its own as well.

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

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

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

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

Thanks, LGTM.

Regards,

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

On Thu, Oct 5, 2023 at 12:22 PM Drouvot, Bertrand
<bertranddrouvot.pg@gmail.com> 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

Hi,

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

On Thu, Oct 5, 2023 at 12:22 PM Drouvot, Bertrand
<bertranddrouvot.pg@gmail.com> 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.com

On Thu, Oct 5, 2023 at 9:32 PM Drouvot, Bertrand
<bertranddrouvot.pg@gmail.com> 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 <varname>flags</varname> 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

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

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

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&amp;dt=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&amp;dt=2023-10-06%2001%3A04%3A05&amp;stg=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&amp;dt=2023-10-06%2001%3A04%3A05&amp;stg=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

Hi,

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

On Thu, Oct 5, 2023 at 9:32 PM Drouvot, Bertrand
<bertranddrouvot.pg@gmail.com> 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 <varname>flags</varname> 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.com