Re: Proposal: allow database-specific role memberships

2022-07-24 Thread Kenaniah Cerny
Hi Antonin,

Thank you again for the detailed review and questions. It was encouraging
to see the increasing level of nuance in this latest round.

It's not clear from the explanation of the GRANT ... IN DATABASE ... / GRANT
>   ... IN CURRENT DATABASE ... that, even if "membership in ... will be
>   effective only when the recipient is connected to the database ...", the
>   ADMIN option might not be "fully effective".


While I'm not entirely sure what you mean by fully effective, it sounds
like you may have expected a database-specific WITH ADMIN OPTION grant to
be able to take effect when connected to a different database (such as
being able to use db_4's database-specific grants when connected to db_3).
The documentation updated in this patch specifies that membership (for
database-specific grants) would be effective only when the grantee is
connected to the same database that the grant was issued for.

In the case of attempting to make a role grant to db_4 from within db_3,
the user would need to have a cluster-wide admin option for the role being
granted, as the test case you referenced in your example aims to verify.

I have added a couple of lines to the documentation included with this
patch in order to clarify.


> Specifically on the regression tests:
>
> * The function check_memberships() has no parameters - is there a
> reason not to use a view?
>

I believe a view would work just as well -- this was an implementation
detail that was fashioned to match the pre-existing rolenames.sql file's
test format.


> * I'm not sure if the pg_auth_members catalog can contain InvalidOid in
>   other columns than dbid. Thus I think that the query in
>   check_memberships() only needs an outer JOIN for the pg_database
> table,
>   while the other joins can be inner.
>

This is probably true. The tests run just as well using inner joins for
pg_roles, as this latest version of the patch reflects.


> * In this part
>
> SET SESSION AUTHORIZATION role_read_12_noinherit;
> SELECT * FROM data; -- error
> SET ROLE role_read_12; -- error
> SELECT * FROM data; -- error
>
> I think you don't need to query the table again if the SET ROLE
> statement
> failed and the same query had been executed before the SET ROLE.


I left that last query in place as a sanity check to ensure that
role_read_12's privileges were indeed not in effect after the call to SET
ROLE.

As we appear to now be working through the minutiae, it is my hope that
this will soon be ready for merge.

- Kenaniah


database-role-memberships-v11.patch
Description: Binary data


Re: Proposal: allow database-specific role memberships

2022-07-17 Thread Kenaniah Cerny
Rebased yet again...

On Mon, Jul 4, 2022 at 1:17 PM Kenaniah Cerny  wrote:

> Hi Antonin,
>
> First of all, thank you so much for taking the time to review my patch.
> I'll answer your questions in reverse order:
>
> The "unsafe_tests" directory is where the pre-existing role tests were
> located. According to the readme of the "unsafe_tests" directory, the tests
> contained within are not run during "make installcheck" because they could
> have side-effects that seem undesirable for a production installation. This
> seemed like a reasonable location as the new tests that this patch
> introduces also modifies the "state" of the database cluster by adding,
> modifying, and removing roles & databases (including template1).
>
> Regarding roles_is_member_of(), the nuance is that role "A" in your
> example would only be considered a member of role "B" (and by extension
> role "C") when connected to the database in which "A" was granted
> database-specific membership to "B". Conversely, when connected to any
> other database, "A" would not be considered to be a member of "B".
>
> This patch is designed to solve the scenarios in which one may want to
> grant constrained access to a broader set of privileges. For example,
> membership in "pg_read_all_data" effectively grants SELECT and USAGE rights
> on everything (implicitly cluster-wide in today's implementation). By
> granting a role membership to "pg_read_all_data" within the context of a
> specific database, the grantee's read-everything privilege is effectively
> constrained to just that specific database (as membership within
> "pg_read_all_data" would not otherwise be held).
>
> A rebased version is attached.
>
> Thanks again!
>
> - Kenaniah
>
> On Wed, Jun 29, 2022 at 6:45 AM Antonin Houska  wrote:
>
>> Kenaniah Cerny  wrote:
>>
>> > Attached is a newly-rebased patch -- would love to get a review from
>> someone whenever possible.
>>
>> I've picked this patch for a review. The patch currently does not apply
>> to the
>> master branch, so I could only read the diff. Following are my comments:
>>
>> * I think that roles_is_member_of() deserves a comment explaining why the
>> code
>>   that you moved into append_role_memberships() needs to be called twice,
>>   i.e. once for global memberships and once for the database-specific
>> ones.
>>
>>   I think the reason is that if, for example, role "A" is a
>> database-specific
>>   member of role "B" and "B" is a "global" member of role "C", then "A"
>> should
>>   not be considered a member of "C", unless "A" is granted "C"
>> explicitly. Is
>>   this behavior intended?
>>
>>   Note that in this example, the "C" members are a superset of "B"
>> members,
>>   and thus "C" should have weaker permissions on database objects than
>>   "B". What's then the reason to not consider "A" a member of "C"? If "C"
>>   gives its members some permissions of "B" (e.g. "pg_write_all_data"),
>> then I
>>   think the roles hierarchy is poorly designed.
>>
>>   A counter-example might help me to understand.
>>
>> * Why do you think that "unsafe_tests" is the appropriate name for the
>>   directory that contains regression tests?
>>
>> I can spend more time on the review if the patch gets rebased.
>>
>> --
>> Antonin Houska
>> Web: https://www.cybertec-postgresql.com
>>
>


database-role-memberships-v10.patch
Description: Binary data


Re: Proposal: allow database-specific role memberships

2022-07-04 Thread Kenaniah Cerny
Hi Antonin,

First of all, thank you so much for taking the time to review my patch.
I'll answer your questions in reverse order:

The "unsafe_tests" directory is where the pre-existing role tests were
located. According to the readme of the "unsafe_tests" directory, the tests
contained within are not run during "make installcheck" because they could
have side-effects that seem undesirable for a production installation. This
seemed like a reasonable location as the new tests that this patch
introduces also modifies the "state" of the database cluster by adding,
modifying, and removing roles & databases (including template1).

Regarding roles_is_member_of(), the nuance is that role "A" in your example
would only be considered a member of role "B" (and by extension role "C")
when connected to the database in which "A" was granted database-specific
membership to "B". Conversely, when connected to any other database, "A"
would not be considered to be a member of "B".

This patch is designed to solve the scenarios in which one may want to
grant constrained access to a broader set of privileges. For example,
membership in "pg_read_all_data" effectively grants SELECT and USAGE rights
on everything (implicitly cluster-wide in today's implementation). By
granting a role membership to "pg_read_all_data" within the context of a
specific database, the grantee's read-everything privilege is effectively
constrained to just that specific database (as membership within
"pg_read_all_data" would not otherwise be held).

A rebased version is attached.

Thanks again!

- Kenaniah

On Wed, Jun 29, 2022 at 6:45 AM Antonin Houska  wrote:

> Kenaniah Cerny  wrote:
>
> > Attached is a newly-rebased patch -- would love to get a review from
> someone whenever possible.
>
> I've picked this patch for a review. The patch currently does not apply to
> the
> master branch, so I could only read the diff. Following are my comments:
>
> * I think that roles_is_member_of() deserves a comment explaining why the
> code
>   that you moved into append_role_memberships() needs to be called twice,
>   i.e. once for global memberships and once for the database-specific ones.
>
>   I think the reason is that if, for example, role "A" is a
> database-specific
>   member of role "B" and "B" is a "global" member of role "C", then "A"
> should
>   not be considered a member of "C", unless "A" is granted "C" explicitly.
> Is
>   this behavior intended?
>
>   Note that in this example, the "C" members are a superset of "B" members,
>   and thus "C" should have weaker permissions on database objects than
>   "B". What's then the reason to not consider "A" a member of "C"? If "C"
>   gives its members some permissions of "B" (e.g. "pg_write_all_data"),
> then I
>   think the roles hierarchy is poorly designed.
>
>   A counter-example might help me to understand.
>
> * Why do you think that "unsafe_tests" is the appropriate name for the
>   directory that contains regression tests?
>
> I can spend more time on the review if the patch gets rebased.
>
> --
> Antonin Houska
> Web: https://www.cybertec-postgresql.com
>


database-role-memberships-v9.patch
Description: Binary data


Re: Proposal: allow database-specific role memberships

2022-03-23 Thread Kenaniah Cerny
Hi all,

cfbot is once again green as of the v7 patch:
https://cirrus-ci.com/github/postgresql-cfbot/postgresql/commitfest/37/3374

- Kenaniah


Re: Proposal: allow database-specific role memberships

2022-03-23 Thread Kenaniah Cerny
Thanks Andres,

An updated patch is attached.

- Kenaniah

On Mon, Mar 21, 2022 at 5:40 PM Andres Freund  wrote:

> Hi,
>
> On 2022-01-22 22:56:44 +0800, Julien Rouhaud wrote:
> > On Sat, Jan 22, 2022 at 05:28:05AM -0800, Kenaniah Cerny wrote:
> > > Thank you so much for the backtrace!
> > >
> > > This latest patch should address by moving the dumpRoleMembership call
> to
> > > before the pointer is closed.
> >
> > Thanks!  The cfbot turned green since:
> >
> https://cirrus-ci.com/github/postgresql-cfbot/postgresql/commitfest/36/3374
>
> red again:
> https://cirrus-ci.com/task/5516269981007872?logs=test_world#L1480
>
> Marked as waiting-on-author.
>
> - Andres
>


database-role-memberships-v7.patch
Description: Binary data


Re: Parameter for planner estimate of recursive queries

2022-01-22 Thread Kenaniah Cerny
> The factor 10 should not be hardcoded in the planner, but should be
settable, just as cursor_tuple_fraction is.

I feel considerably out of my depth here, but I like the idea of a working
table size multiplier GUC, given the challenges of predicting the number of
iterations (and any adjustments to cardinality per iteration). An
exponential cost function may lead to increasingly pathological outcomes,
especially when estimates for cte_rows are off.

In the EXPLAINs, it looked like the estimates for knows_pkey were off by an
order of magnitude or so. It's possible the planner would have chosen the
Nested Loop plan if knows_pkey had estimated to rows=87 (as the WindowAgg
would have estimated to roughly the same size as the second plan anyways,
even with rel->tuples = 10 * cte_rows).

I also wonder if there's a better default than cte_rows * 10, but
implementing a new GUC sounds like a reasonable solution to this as well.

--

Kenaniah

On Sat, Jan 22, 2022 at 1:58 PM Simon Riggs 
wrote:

> On Wed, 27 Oct 2021 at 15:58, Simon Riggs 
> wrote:
>
> > The poor performance is traced to the planner cost estimates for
> > recursive queries. Specifically, the cost of the recursive arm of the
> > query is evaluated based upon both of these hardcoded assumptions:
> >
> > 1. The recursion will last for 10 loops
> > 2. The average size of the worktable will be 10x the size of the
> > initial query (non-recursive term).
> >
> > Taken together these assumptions lead to a very poor estimate of the
> > worktable activity (in this case), which leads to the plan changing as
> > a result.
> >
> > The factor 10 is a reasonably safe assumption and helps avoid worst
> > case behavior in bigger graph queries. However, the factor 10 is way
> > too large for many types of graph query, such as where the path
> > through the data is tight, and/or the query is written to prune bushy
> > graphs, e.g. shortest path queries. The factor 10 should not be
> > hardcoded in the planner, but should be settable, just as
> > cursor_tuple_fraction is.
>
> If you think this should be derived without parameters, then we would
> want a function that starts at 1 for 1 input row and gets much larger
> for larger input. The thinking here is that Graph OLTP is often a
> shortest path between two nodes, whereas Graph Analytics and so the
> worktable will get much bigger.
>
> So I'm, thinking we use
>
> rel->tuples = min(1, cte_rows * cte_rows/2);
>
> --
> Simon Riggshttp://www.EnterpriseDB.com/
>
>
>
>
>


Re: Proposal: allow database-specific role memberships

2022-01-22 Thread Kenaniah Cerny
Thank you so much for the backtrace!

This latest patch should address by moving the dumpRoleMembership call to
before the pointer is closed.

On Sat, Jan 22, 2022 at 1:11 AM Julien Rouhaud  wrote:

> Hi,
>
> On Fri, Jan 21, 2022 at 07:01:21PM -0800, Kenaniah Cerny wrote:
> > Thanks for the feedback.
> >
> > I have attached an alternate version of the v5 patch that incorporates
> the
> > suggested changes to the documentation and DRYs up some of the acl.c code
> > for comparison. As for the databaseOid / InvalidOid parameter, I'm open
> to
> > any suggestions for how to make that even cleaner, but am currently at a
> > loss as to how that would look.
> >
> > CI is showing a failure to run pg_dump on just the Linux - Debian
> Bullseye
> > job (https://cirrus-ci.com/task/5265343722553344). Does anyone have any
> > ideas as to where I should look in order to debug that?
>
> Did you try to reproduce it on some GNU/Linux system?  FTR I had and I get
> a
> segfault in pg_dumpall:
>
> (gdb) bt
> #0  __pthread_kill_implementation (threadid=,
> signo=signo@entry=6, no_tid=no_tid@entry=0) at pthread_kill.c:44
> #1  0x7f329e7e40cf in __pthread_kill_internal (signo=6,
> threadid=) at pthread_kill.c:78
> #2  0x7f329e7987a2 in __GI_raise (sig=sig@entry=6) at
> ../sysdeps/posix/raise.c:26
> #3  0x7f329e783449 in __GI_abort () at abort.c:79
> #4  0x7f329e7d85d8 in __libc_message (action=action@entry=do_abort,
> fmt=fmt@entry=0x7f329e90b6aa "%s\n") at ../sysdeps/posix/libc_fatal.c:155
> #5  0x7f329e7edcfa in malloc_printerr (str=str@entry=0x7f329e9092c3
> "free(): invalid pointer") at malloc.c:5536
> #6  0x7f329e7ef504 in _int_free (av=, p= out>, have_lock=0) at malloc.c:4327
> #7  0x7f329e7f1f81 in __GI___libc_free (mem=) at
> malloc.c:3279
> #8  0x7f329e7dbec5 in __GI__IO_free_backup_area 
> (fp=fp@entry=0x561775f126c0)
> at genops.c:190
> #9  0x7f329e7db6af in _IO_new_file_overflow (f=0x561775f126c0, ch=-1)
> at fileops.c:758
> #10 0x7f329e7da7be in _IO_new_file_xsputn (n=2, data=,
> f=) at
> /usr/src/debug/sys-libs/glibc-2.34-r4/glibc-2.34/libio/libioP.h:947
> #11 _IO_new_file_xsputn (f=0x561775f126c0, data=, n=2) at
> fileops.c:1197
> #12 0x7f329e7cfd32 in __GI__IO_fwrite (buf=0x7ffc90bb0ac0, size=1,
> count=2, fp=0x561775f126c0) at
> /usr/src/debug/sys-libs/glibc-2.34-r4/glibc-2.34/libio/libioP.h:947
> #13 0x56177483c758 in flushbuffer (target=0x7ffc90bb0a90) at
> snprintf.c:310
> #14 0x56177483c4e8 in pg_vfprintf (stream=0x561775f126c0,
> fmt=0x561774840dec "\n\n", args=0x7ffc90bb0f00) at snprintf.c:259
> #15 0x56177483c5ce in pg_fprintf (stream=0x561775f126c0,
> fmt=0x561774840dec "\n\n") at snprintf.c:270
> #16 0x561774831893 in dumpRoleMembership (conn=0x561775f09600,
> databaseId=0x561775f152d2 "1") at pg_dumpall.c:991
> #17 0x561774832426 in dumpDatabases (conn=0x561775f09600) at
> pg_dumpall.c:1332
> #18 0x56177483049e in main (argc=3, argv=0x7ffc90bb1658) at
> pg_dumpall.c:596
>
> I didn't look in detail, but:
>
> @@ -1323,6 +1327,10 @@ dumpDatabases(PGconn *conn)
> exit_nicely(1);
> }
>
> +   /* Dump database-specific roles if server is running 15.0 or later
> */
> +   if (server_version >= 15)
> +   dumpRoleMembership(conn, dbid);
> +
>
> Isn't that trying print to OPF after the possible fclose(OPF) a bit before
> and
> before it's reopened?
>
diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index 1e65c426b28e..a0beec6136e6 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -1642,11 +1642,10 @@ SCRAM-SHA-256$iteration count:
   
 
   
-   Because user identities are cluster-wide,
-   pg_auth_members
-   is shared across all databases of a cluster: there is only one
-   copy of pg_auth_members per cluster, not
-   one per database.
+   User identities are cluster-wide, but role memberships can be either
+   cluster-wide or database-specific (as specified by the value of the
+   dbid column).  The pg_auth_members
+   catalog is shared across all databases of a cluster.
   
 
   
@@ -1703,6 +1702,17 @@ SCRAM-SHA-256$iteration count:
roleid to others
   
  
+
+  
+  
+   dbid oid
+   (references pg_database.oid)
+  
+  
+   ID of the database that this membership is constrained to; zero if membership is cluster-wide
+  
+ 
+
 

   
diff --git a/doc/src/sgml/ref/grant.sgml b/doc/src/sgml/ref/grant.sgml
index a897712de2e5..98bcfed5f507 100644
--- a/doc/src/sgml/ref/grant.sgml
+++ b/doc/src/sgml/ref/grant.sgml
@@ -93,6 +93,7 @@ GRANT { USAGE | ALL [ PRIVILEGES ] }
 [ GRANTED 

Re: Proposal: allow database-specific role memberships

2022-01-21 Thread Kenaniah Cerny
Thanks for the feedback.

I have attached an alternate version of the v5 patch that incorporates the
suggested changes to the documentation and DRYs up some of the acl.c code
for comparison. As for the databaseOid / InvalidOid parameter, I'm open to
any suggestions for how to make that even cleaner, but am currently at a
loss as to how that would look.

CI is showing a failure to run pg_dump on just the Linux - Debian Bullseye
job (https://cirrus-ci.com/task/5265343722553344). Does anyone have any
ideas as to where I should look in order to debug that?

Kenaniah

On Fri, Jan 21, 2022 at 3:04 PM David G. Johnston <
david.g.johns...@gmail.com> wrote:

> On Fri, Jan 21, 2022 at 3:12 PM Kenaniah Cerny  wrote:
>
>> The latest rebased version of the patch is attached.
>>
>
> As I was just reminded, we tend to avoid specifying specific PostgreSQL
> versions in our documentation.  We just say what the current version does.
> Here, the note sentences at lines 62 and 63 don't follow documentation
> norms on that score and should just be removed.  The last two sentences
> belong in the main description body, not a note.  Thus the whole note goes
> away.
>
> I don't think I really appreciated the value this feature would have when
> combined with the predefined roles like pg_read_all_data and
> pg_write_all_data.
>
> I suppose I don't really appreciate the warning about SUPERUSER, etc...or
> at least why this warning is somehow specific to the per-database version
> of role membership.  If this warning is desirable it should be worded to
> apply to role membership in general - and possibly proposed as a separate
> patch for consideration.
>
> I didn't dive deeply but I think we now have at three places in the acl.c
> code where after setting memlist from the system cache we perform nearly
> identical for loops to generate the final roles_list.  Possibly this needs
> a refactor first so that you can introduce the per-database stuff more
> succinctly.  Basically, the vast majority of this commit is just adding
> InvalidOid and databaseOid all other the place - with a few minor code
> changes to accommodate the new arguments.  The acl.c code should try and be
> made done the same after post-refactor.
>
> David J.
>
>


database-role-memberships-v5-alternate.patch
Description: Binary data


Re: Proposal: allow database-specific role memberships

2022-01-21 Thread Kenaniah Cerny
The latest rebased version of the patch is attached.

On Tue, Jan 11, 2022 at 11:01 PM Julien Rouhaud  wrote:

> Hi,
>
> On Thu, Dec 2, 2021 at 2:26 AM Kenaniah Cerny  wrote:
> >
> > Attached is a rebased version of the patch that omits catversion.h in
> order to avoid conflicts.
>
> Unfortunately even without that the patch doesn't apply anymore
> according to the cfbot: http://cfbot.cputube.org/patch_36_3374.log
>
> 1 out of 3 hunks FAILED -- saving rejects to file
> src/backend/parser/gram.y.rej
> [...]
> 2 out of 8 hunks FAILED -- saving rejects to file
> src/bin/pg_dump/pg_dumpall.c.rej
>
> Could you send a rebased version?
>
> In the meantime I'm switching the patch to Waiting on Author.
>


database-role-memberships-v5.patch
Description: Binary data


Re: Proposal: allow database-specific role memberships

2021-12-01 Thread Kenaniah Cerny
Thank you for the advice!

Attached is a rebased version of the patch that omits catversion.h in order
to avoid conflicts.

On Wed, Nov 17, 2021 at 6:17 AM Daniel Gustafsson  wrote:

> > On 28 Oct 2021, at 21:39, Kenaniah Cerny  wrote:
>
> > Thank you Asif. A rebased patch is attached.
>
> This patch fails to apply yet again, this time due to a collusion in
> catversion.h.  I think it's fine to omit the change in catversion.h as it's
> likely to repeatedly cause conflicts, and instead just mention it on the
> thread.  Any committer picking it up will know to perform the change
> anyways,
> so leaving it out can keep the patch from conflicting.
>
> --
> Daniel Gustafsson   https://vmware.com/
>
>


database-role-memberships-v4.patch
Description: Binary data


Re: Proposal: allow database-specific role memberships

2021-10-28 Thread Kenaniah Cerny
Thank you Asif. A rebased patch is attached.

On Thu, Oct 28, 2021 at 11:04 AM Asif Rehman  wrote:

> The following review has been posted through the commitfest application:
> make installcheck-world:  not tested
> Implements feature:   not tested
> Spec compliant:   not tested
> Documentation:not tested
>
> The patch does not apply on HEAD anymore. Looks like it needs to be
> rebased.
>
> The new status of this patch is: Waiting on Author
>


database-role-memberships-v3.patch
Description: Binary data


Re: Proposal: allow database-specific role memberships

2021-10-24 Thread Kenaniah Cerny
Hi all,

Thank you for the feedback so far!

Attached is a completed implementation (including tests and documentation).
Based on the feedback I have received so far, I will be submitting this
implementation to the commitfest.

Thanks again,

Kenaniah

On Mon, Oct 11, 2021 at 9:05 AM Stephen Frost  wrote:

> Greetings,
>
> * David G. Johnston (david.g.johns...@gmail.com) wrote:
> > On Monday, October 11, 2021, Stephen Frost  wrote:
> > > I don't think "just don't grant access to those other databases"
> > > is actually a proper answer- there is certainly a use-case for "I want
> > > user X to have read access to all tables in *this* database, and also
> > > allow them to connect to some other database but not have that same
> > > level of access there."
> >
> > Sure, that has a benefit.  But creating a second user for the other
> > database and putting the onus on the user to use the correct credentials
> > when logging into a particular database is a valid option  - it is in
> fact
> > the status quo.  Due to the complexity of adding a whole new grant
> > dimension to the system the status quo is an appealing option.  Annoyance
> > factor aside it technically solves the per-database permissions problem
> put
> > forth.
>
> I disagree entirely that forcing users to have multiple accounts and to
> deal with "using the correct one" is at all reasonable.  That's an utter
> hack that results in a given user having multiple different accounts-
> something that gets really ugly to deal with in enterprise deployments
> which use any kind of centralized authentication system.
>
> No, that's not a solution.  Perhaps there's another way to implement
> this capability that is simpler than what's proposed here, but saying
> "just give each user two accounts" isn't a solution.  Sure, it'll work
> for existing released versions of PG, just like there's a lot of things
> that people can do to hack around our deficiencies, but that doesn't
> change that these are areas which we are lacking and where we should be
> trying to provide a proper solution.
>
> Thanks,
>
> Stephen
>


database-role-memberships-v2.patch
Description: Binary data


Proposal: allow database-specific role memberships

2021-10-10 Thread Kenaniah Cerny
Hi all,

In building off of prior art regarding the 'pg_read_all_data' and
'pg_write_all_data' roles, I would like to propose an extension to roles
that would allow for database-specific role memberships (for the purpose of
granting database-specific privileges) as an additional layer of
abstraction.

= Problem =

There is currently no mechanism to grant the privileges afforded by the
default roles on a per-database basis. This makes it difficult to cleanly
accomplish permissions such as 'db_datareader' and 'db_datawriter' (which
are database-level roles in SQL Server that respectively grant read and
write access within a specific database).

The recently-added 'pg_read_all_data' and 'pg_write_all_data' work
similarly to 'db_datareader' and 'db_datawriter', but work cluster-wide.

= Proposal =

I propose an extension to the GRANT / REVOKE syntax as well as an
additional column within pg_auth_members in order to track role memberships
that are only effective within the specified database.

Role membership (and subsequent privileges) would be calculated using the
following algorithm:
 - Check for regular (cluster-wide) role membership (the way it works today)
 - Check for database-specific role membership based on the
currently-connected database

Attached is a proof of concept patch that implements this.

= Implementation Notes =

- A new column (pg_auth_members.dbid) in the system catalog that is set to
InvalidOid for regular role memberships, or the oid of the given database
for database-specific role memberships.

- GRANT / REVOKE syntax has been extended to include the ability to specify
a database-specific role membership:
  - "IN DATABASE database_name" would cause the GRANT to be applicable only
within the specified database.
  - "IN CURRENT DATABASE" would cause the GRANT to be applicable only
within the currently-connected database.
  - Omission of the clause would create a regular (cluster-wide) role
membership (the way it works today).

The proposed syntax (applies to REVOKE as well):

GRANT role_name [, ...] TO role_specification [, ...]
[ IN DATABASE database_name | IN CURRENT DATABASE ]
[ WITH ADMIN OPTION ]
[ GRANTED BY role_specification ]

- DROP DATABASE has been updated to clean up any database-specific role
memberships that are associated with the database being dropped.

- pg_dump_all will dump database-specific role memberships using the "IN
CURRENT DATABASE" syntax. (pg_dump has not been modified)

- is_admin_of_role()'s signature has been updated to include the oid of the
database being checked as a third argument. This now returns true if the
member has WITH ADMIN OPTION either globally or for the database given.

- roles_is_member_of() will additionally include any database-specific role
memberships for the database being checked in its result set.

= Example =

CREATE DATABASE accounting;
CREATE DATABASE sales;

CREATE ROLE alice;
CREATE ROLE bob;

-- Alice is granted read-all privileges cluster-wide (nothing new here)
GRANT pg_read_all_data TO alice;

-- Bob is granted read-all privileges to just the accounting database
GRANT pg_read_all_data TO bob IN DATABASE accounting;

= Final Thoughts =

This is my first attempt at contributing code to the project, and I would
not self-identify as a C programmer. I wanted to get a sense for how
receptive the contributors and community would be to this proposal and
whether there were any concerns or preferred alternatives before I further
embark on a fool's errand.

Thoughts?

Thanks,

-- Kenaniah


poc-database-role-membership-v1.patch
Description: Binary data