Thread: Re: [COMMITTERS] pgsql: Use a bitmask to represent role attributes
Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> Use a bitmask to represent role attributes
> The previous representation using a boolean column for each attribute
> would not scale as well as we want to add further attributes.
> Extra auxilliary functions are added to go along with this change, to
> make up for the lost convenience of access of the old representation.
I have to apologize for not having paid more attention, but ... is this
*really* such a great idea? You've just broken any client-side code
that looks directly at pg_authid. Moreover, I don't particularly buy
the idea that this somehow insulates us from the compatibility costs of
adding new role properties: you're still going to have to add columns to
the pg_roles view, and adjust clients that look at that, every time.
Replacing bool-column accesses with bitmask manipulation doesn't seem
like it's a win on a micro-optimization level either, certainly not for
SQL-level coding where you've probably made it two orders of magnitude
more expensive. And lastly, what happens when you run out of bits in
that bigint column?
Again, I suppose I should have objected earlier, but I really seriously
doubt that this is a good idea.
regards, tom lane
Tom Lane wrote: > Again, I suppose I should have objected earlier, but I really seriously > doubt that this is a good idea. Ugh. I thought we had a consensus that this was the accepted way forward; that's my reading of the old thread, http://www.postgresql.org/message-id/flat/20141016133218.GW28859@tamriel.snowman.net#20141016133218.GW28859@tamriel.snowman.net Breaking clients was considered acceptable, which is why some of these functions were introduced. There were some differing opinions; Simon for instance suggested the use of an array rather than a bitmask, but that would have broken clients all the same. If there's strong opposition to this whole line of development, I can revert. Anyone else wants to give an opinion? -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> > Use a bitmask to represent role attributes
> > The previous representation using a boolean column for each attribute
> > would not scale as well as we want to add further attributes.
>
> > Extra auxilliary functions are added to go along with this change, to
> > make up for the lost convenience of access of the old representation.
>
> I have to apologize for not having paid more attention, but ... is this
> *really* such a great idea? You've just broken any client-side code
> that looks directly at pg_authid.
Anything which looks at pg_authid for the relevant columns needs to be
updated for the changes which were made for rolreplication previously,
and now rolbypassrls and any other role attributes added. While I agree
that it's something to consider (and even mentioned it earlier in the
thread..), there wasn't any argument about it from those who were
following the discussion.
Perhaps we could have gone out of our way to ask the pgAdmin authors and
other client-side utilities which look at these columns if this will
more issues than new columns do.
> Moreover, I don't particularly buy
> the idea that this somehow insulates us from the compatibility costs of
> adding new role properties: you're still going to have to add columns to
> the pg_roles view, and adjust clients that look at that, every time.
That's correct, however, this change wasn't intended to insulate anyone
from those compatibility changes but rather to make better use of the
bytes in each pg_authid record. We were already up to 8 individual bool
columns, wasting space for each.
> Replacing bool-column accesses with bitmask manipulation doesn't seem
> like it's a win on a micro-optimization level either, certainly not for
> SQL-level coding where you've probably made it two orders of magnitude
> more expensive.
I agree that it's more expensive for SQL users, but it's not our first
use of a bitmask in the catalog. Perhaps this is more user-visible than
other use-cases but we also provide views to address common usages in
this case already, where we don't in other situations.
> And lastly, what happens when you run out of bits in
> that bigint column?
We can add another, though this seems unlikely to happen given the
previous discussion and review of what additional role attributes would
be nice to add. There are some scenarios I've considered which would
lead to using perhaps another 15-20, but 64 sure seems far off.
> Again, I suppose I should have objected earlier, but I really seriously
> doubt that this is a good idea.
I tried to solicit discussion about these concerns earlier but we're all
busy, no problem.
For my 2c, I do think this is the right direction to go in as adding
another 15 boolean columns to pg_authid definitely strikes me as a bad
idea, but we can certainly discuss the trade-offs. Another proposal
(from Simon, as I recall) was to use an array. That runs into nearly
all the same problems and the on-disk representation ends up being even
larger, which is why I didn't see that being a good idea.
Thanks,
Stephen
Alvaro Herrera <alvherre@2ndquadrant.com> writes: > Tom Lane wrote: >> Again, I suppose I should have objected earlier, but I really seriously >> doubt that this is a good idea. > Ugh. I thought we had a consensus that this was the accepted way > forward; that's my reading of the old thread, > http://www.postgresql.org/message-id/flat/20141016133218.GW28859@tamriel.snowman.net#20141016133218.GW28859@tamriel.snowman.net I was aware that we were thinking of introducing a bunch more role attributes, but I'm wondering what's the rationale for assuming that (a) they'll all be booleans, and (b) there will never, ever, be more than 64 of them. The argument that lots of boolean columns won't scale nicely doesn't seem to lead to the conclusion that a fixed-size bitmap is better. I'd have gone with just adding more bool columns as needed. regards, tom lane
On Tue, Dec 23, 2014 at 10:26 AM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote: > Tom Lane wrote: >> Again, I suppose I should have objected earlier, but I really seriously >> doubt that this is a good idea. > > Ugh. I thought we had a consensus that this was the accepted way > forward; that's my reading of the old thread, > http://www.postgresql.org/message-id/flat/20141016133218.GW28859@tamriel.snowman.net#20141016133218.GW28859@tamriel.snowman.net > > Breaking clients was considered acceptable, which is why some of these > functions were introduced. There were some differing opinions; Simon > for instance suggested the use of an array rather than a bitmask, but > that would have broken clients all the same. > > If there's strong opposition to this whole line of development, I can > revert. Anyone else wants to give an opinion? I would have preferred (and I believe argued for) keeping the existing catalog representation for existing attributes and using a bitmask for new ones, to avoid breaking client code. But I am not sure if that's actually the best decision. I find Tom's concern about needing more than 64 attributes to be ill-founded; I can't really see that happening on any timescale that matters. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 2014-12-23 10:40:15 -0500, Robert Haas wrote: > On Tue, Dec 23, 2014 at 10:26 AM, Alvaro Herrera > <alvherre@2ndquadrant.com> wrote: > > Tom Lane wrote: > >> Again, I suppose I should have objected earlier, but I really seriously > >> doubt that this is a good idea. > > > > Ugh. I thought we had a consensus that this was the accepted way > > forward; that's my reading of the old thread, > > http://www.postgresql.org/message-id/flat/20141016133218.GW28859@tamriel.snowman.net#20141016133218.GW28859@tamriel.snowman.net > > > > Breaking clients was considered acceptable, which is why some of these > > functions were introduced. There were some differing opinions; Simon > > for instance suggested the use of an array rather than a bitmask, but > > that would have broken clients all the same. > > > > If there's strong opposition to this whole line of development, I can > > revert. Anyone else wants to give an opinion? > > I would have preferred (and I believe argued for) keeping the existing > catalog representation for existing attributes and using a bitmask for > new ones, to avoid breaking client code. But I am not sure if that's > actually the best decision. I personally think in this case the clear break is slightly better than having different styles of representation around for a long while. > I find Tom's concern about needing more > than 64 attributes to be ill-founded; I can't really see that > happening on any timescale that matters. I personally would prefer a 'custom' type to represent the permissions. Internally that could very well be current bitmask, but the external representation could be more complex (i.e. some textual representation). That'd make it easy to make the representation wider/more complex if needed. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
Stephen Frost <sfrost@snowman.net> writes:
> * Tom Lane (tgl@sss.pgh.pa.us) wrote:
>> I have to apologize for not having paid more attention, but ... is this
>> *really* such a great idea? You've just broken any client-side code
>> that looks directly at pg_authid.
> Anything which looks at pg_authid for the relevant columns needs to be
> updated for the changes which were made for rolreplication previously,
> and now rolbypassrls and any other role attributes added.
Right. 95% of the compatibility costs of adding a property are going to
be sunk in any case because clients will need to know what flags exist,
how to spell them in CREATE/ALTER USER commands, etc. (Some of this could
be alleviated perhaps if we invented a server-side function that produced
a text string representing all of a user's properties, but that's quite
orthogonal to this patch.)
> That's correct, however, this change wasn't intended to insulate anyone
> from those compatibility changes but rather to make better use of the
> bytes in each pg_authid record. We were already up to 8 individual bool
> columns, wasting space for each.
You're really seriously concerned about a couple of dozen bytes per role?
That is micro-optimization of the very worst sort. We are routinely
wasting multiples of that on things like using "name" rather than a
variable-length text representation for name columns, and I don't think
anyone is especially concerned about that anymore. Maybe back in the
nineties it'd have been worth bit-shaving that way.
To me, a bitmask might make sense if the properties could usefully be
manipulated as a unit, but I'm not seeing any such advantage here.
> For my 2c, I do think this is the right direction to go in as adding
> another 15 boolean columns to pg_authid definitely strikes me as a bad
> idea, but we can certainly discuss the trade-offs.
Meh. To the extent that users look at pg_roles rather than pg_authid,
it's going to look like another 15 boolean columns to them anyway ...
except that now, those columns are suddenly a lot more expensive to read.
15-20 more bool columns sounds entirely fine to me. If we were talking
a couple of hundred, I'd start to worry; but this approach would not
handle that very well either.
regards, tom lane
Robert Haas <robertmhaas@gmail.com> writes:
> I would have preferred (and I believe argued for) keeping the existing
> catalog representation for existing attributes and using a bitmask for
> new ones, to avoid breaking client code. But I am not sure if that's
> actually the best decision. I find Tom's concern about needing more
> than 64 attributes to be ill-founded; I can't really see that
> happening on any timescale that matters.
I tend to agree, which is why I'm questioning the decision to not just
keep adding bool columns. I don't see how that's not both more convenient
and less surprising.
regards, tom lane
* Tom Lane (tgl@sss.pgh.pa.us) wrote: > Alvaro Herrera <alvherre@2ndquadrant.com> writes: > > Tom Lane wrote: > >> Again, I suppose I should have objected earlier, but I really seriously > >> doubt that this is a good idea. > > > Ugh. I thought we had a consensus that this was the accepted way > > forward; that's my reading of the old thread, > > http://www.postgresql.org/message-id/flat/20141016133218.GW28859@tamriel.snowman.net#20141016133218.GW28859@tamriel.snowman.net > > I was aware that we were thinking of introducing a bunch more role > attributes, but I'm wondering what's the rationale for assuming that > (a) they'll all be booleans, I attempted to do a comprehensive review of the case by looking at all of the existing superuser checks and considering where it made sense to make things more flexible. The result was specifically that they were all boolean cases except for the previously-discussed 'create directory' idea. Had there been other cases which weren't boolean, I would have been looking for another representation. That said, this does not wall-off additional columns going into pg_authid later, if there are any non-boolean cases which would make sense. Those cases, I suspect, would lead to new catalog tables in their own right anyway though, rather than additional columns in pg_authid. The 'create directory' idea certainly made more sense to me with a new catalog table. Having an array of directories in pg_authid was never considered, though I did consider adding a catalog table which was essentially role+key/value where 'value' was an arbitrary string or perhaps bytea blob, but all the boolean cases would have gone into pg_authid and that new catalog would have only been used for the 'directory' case (or at least, I couldn't find any other use-cases for it in my review). > and (b) there will never, ever, be more > than 64 of them. I do not think this approach walls off adding more than 64. On the other hand, I don't like the idea of doubling the size of pg_authid if we do get to 64 because we really want to use boolean columns instead of a bitmap. > The argument that lots of boolean columns won't > scale nicely doesn't seem to lead to the conclusion that a fixed-size > bitmap is better. Perhaps it's because I just recently came out of an environment where we were constantly fighting with size issues for boolean values, but boolean columns definitely don't scale nicely. I admit that we still have larger issues when it comes to running databases with lots of roles (which means people tend to not do it), such as being unable to partition catalog tables, but I'm still hopefull we'll one day get to a point where more users can exist as real database users (and therefore have the database enforcing the permission system, rather than each application having to write its own) and I'd rather not have pg_authid bloated without cause. > I'd have gone with just adding more bool columns as needed. I don't think I was the only one concerned that adding a bunch of new columns would bloat the size of pg_authid and the C structure behind it, but I'm not remembering offhand who else considered it. Thanks, Stephen
On 12/23/2014 04:46 PM, Andres Freund wrote:
> [snip]
> I find Tom's concern about needing more
> than 64 attributes to be ill-founded; I can't really see that
> happening on any timescale that matters.
Hmm... most probably, not (or so I hope)... Unless we begin to add many
differerent "capabilities", like it was recently suggested.
I, for one, have at least two of them to propose, but I guess not that
many more should be needed.
> I personally would prefer a 'custom' type to represent the
> permissions. Internally that could very well be current bitmask, but the
> external representation could be more complex (i.e. some textual
> representation). That'd make it easy to make the representation wider/more
> complex if needed.
Indeed, though this would imply adding a new "bitstring?" type to core
Postgres.
Do you have any further input on what this type would look like ? Any
operators that might be useful? ISTM that this would actually be the
greatest strength of a type proper (vs. "hardcoded" bit-wise operations
in core)
In any case, having the type's input/output perform the conversion
from/to text is quite equivalent to the current implementation.
Considering that this custom type would need to be in core, the
differences should be minimal.
Or am I missing something obvious?
Thanks,
/ J.L.
On Tue, Dec 23, 2014 at 10:51 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Meh. To the extent that users look at pg_roles rather than pg_authid, > it's going to look like another 15 boolean columns to them anyway ... > except that now, those columns are suddenly a lot more expensive to read. Ugh. I think that's actually a really good point. I guess I'll +1 reverting this, then. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Stephen Frost <sfrost@snowman.net> writes:
> * Tom Lane (tgl@sss.pgh.pa.us) wrote:
>> I'd have gone with just adding more bool columns as needed.
> I don't think I was the only one concerned that adding a bunch of new
> columns would bloat the size of pg_authid and the C structure behind it,
> but I'm not remembering offhand who else considered it.
Lessee, as of 9.4 pg_authid required 76 bytes per row, plus row header
overhead that'd have probably pushed it to 104 bytes per row (more if
you had non-null rolpassword or rolvaliduntil). If we add as many as 20
more booleans we'd be at 124 bytes per row, whereas with this approach
we'd have, well, 104 bytes per row. I'm not seeing much benefit to
justify such a drastic change of approach.
regards, tom lane
José Luis Tallón wrote: > On 12/23/2014 04:46 PM, Andres Freund wrote: > >I personally would prefer a 'custom' type to represent the > >permissions. Internally that could very well be current bitmask, but the > >external representation could be more complex (i.e. some textual > >representation). That'd make it easy to make the representation wider/more > >complex if needed. > > Indeed, though this would imply adding a new "bitstring?" type to core > Postgres. We already have varlena bitstrings, in the guise of types bit and varbit. > Do you have any further input on what this type would look like ? Any > operators that might be useful? ISTM that this would actually be the > greatest strength of a type proper (vs. "hardcoded" bit-wise operations in > core) I imagine something like the "reg*" types (regclass, regtype etc): on input you can pass them an OID, or an possibly-qualified object name; internally they store the OID. You can cast them to OID to obtain the numerical value, or just print them out to get the possibly-qualified name. In the case at hand, on output you would get the equivalent of the text[] you get from pg_role_all_attributes(), and you can input it in the same way or you can input the bitmask; and the underlying storage is the bitmask. This doesn't solve the client compatibility break, or the issue that querying pg_roles is expensive. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
* Andres Freund (andres@2ndquadrant.com) wrote:
> On 2014-12-23 10:40:15 -0500, Robert Haas wrote:
> > I would have preferred (and I believe argued for) keeping the existing
> > catalog representation for existing attributes and using a bitmask for
> > new ones, to avoid breaking client code. But I am not sure if that's
> > actually the best decision.
>
> I personally think in this case the clear break is slightly better than
> having different styles of representation around for a long while.
Yes, I'm completely with Andres on this point. Having a mixed case
where there are some boolean columns and then a bitmask strikes as the
worst approach- it doesn't save anyone from the client-side code changes
but rather makes them have to consider both ways and keep an internal
list somewhere of which ones are in boolean columns and which are in the
bitmask, yuck.
> > I find Tom's concern about needing more
> > than 64 attributes to be ill-founded; I can't really see that
> > happening on any timescale that matters.
>
> I personally would prefer a 'custom' type to represent the
> permissions. Internally that could very well be current bitmask, but the
> external representation could be more complex (i.e. some textual
> representation). That'd make it easy to make the representation wider/more
> complex if needed.
In some ways, I feel like this is what we actually have now.. If you
consider pg_authid to be 'internal' and pg_roles to be 'external'. That
said, I'm not against what you're proposing, but at the same time I'm
not quite sure what that would end up looking like or how difficult it
would be to support a complex type in the catalog and I don't think
there's any way it would address the on-disk size concern, and if we
have to start having a different C-level representation for pg_authid
than the on-disk representation, well, that strikes me as a lot more
complicated too..
Thanks,
Stephen
* Robert Haas (robertmhaas@gmail.com) wrote:
> On Tue, Dec 23, 2014 at 10:51 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > Meh. To the extent that users look at pg_roles rather than pg_authid,
> > it's going to look like another 15 boolean columns to them anyway ...
> > except that now, those columns are suddenly a lot more expensive to read.
>
> Ugh. I think that's actually a really good point. I guess I'll +1
> reverting this, then.
If that's the only consideration for this, well, that's certainly quite
straight-forward to change in the other direction too. The new function
suggested by Andres actually makes it really easy to get a textual list
of all the role attributes which a role has from the bitmask too. I was
more concerned with the on-disk and C-level structure and size than
about the time required to get at the value of each bit at the
SQL-level.
Thanks,
Stephen
* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Stephen Frost <sfrost@snowman.net> writes:
> > * Tom Lane (tgl@sss.pgh.pa.us) wrote:
> >> I'd have gone with just adding more bool columns as needed.
>
> > I don't think I was the only one concerned that adding a bunch of new
> > columns would bloat the size of pg_authid and the C structure behind it,
> > but I'm not remembering offhand who else considered it.
>
> Lessee, as of 9.4 pg_authid required 76 bytes per row, plus row header
> overhead that'd have probably pushed it to 104 bytes per row (more if
> you had non-null rolpassword or rolvaliduntil). If we add as many as 20
> more booleans we'd be at 124 bytes per row, whereas with this approach
> we'd have, well, 104 bytes per row. I'm not seeing much benefit to
> justify such a drastic change of approach.
I suppose. I didn't consider it to be a terribly drastic change but
rather simply using a better representation for a mostly-internal bit of
data. It also lended itself pretty nicely to maniuplation (at least,
imv, the code is a lot cleaner with the bitmask, but it's not a huge
deal). Guess I had been expecting concerns to be raised around adding
many more bytes where there wouldn't have been.
Thanks,
Stephen
Stephen Frost <sfrost@snowman.net> writes:
> If that's the only consideration for this, well, that's certainly quite
> straight-forward to change in the other direction too. The new function
> suggested by Andres actually makes it really easy to get a textual list
> of all the role attributes which a role has from the bitmask too.
We could have that regardless of the representation, if the function is
defined along the lines of "given a user OID, give me a text string
representing the user's attributes". However, that only helps for
pg_dumpall and any other clients whose requirement is exactly satisfied
by a string that fits into CREATE/ALTER USER. The current formatting
of psql's \du, for example, absolutely requires adding more client-side
code every time we add a property; whether the catalog representation is
bools or a bitmask really isn't going to change the pain level much there.
regards, tom lane
On Tue, Dec 23, 2014 at 11:34:09AM -0500, Tom Lane wrote:
> Stephen Frost <sfrost@snowman.net> writes:
> > If that's the only consideration for this, well, that's certainly quite
> > straight-forward to change in the other direction too. The new function
> > suggested by Andres actually makes it really easy to get a textual list
> > of all the role attributes which a role has from the bitmask too.
>
> We could have that regardless of the representation, if the function is
> defined along the lines of "given a user OID, give me a text string
> representing the user's attributes". However, that only helps for
> pg_dumpall and any other clients whose requirement is exactly satisfied
> by a string that fits into CREATE/ALTER USER. The current formatting
> of psql's \du, for example, absolutely requires adding more client-side
> code every time we add a property; whether the catalog representation is
> bools or a bitmask really isn't going to change the pain level much there.
I am with Tom on this --- there is more wasted space in the 'name'
column pg_authid.rolname than by shoving 40 boolean values into a
bitmap. Adding the complexity of a bitmap doesn't make sense here. I
also apologize for the late feedback.
Offtopic, what I would really _love_ to see improved is our display of
object permissions:
Access privileges Schema | Name | Type | Access privileges | Column
privileges| Policies--------+--------+-------+---------------------------+-------------------+---------- public |
crypto| table | postgres=arwdDxt/postgres+| | | | | =r/postgres |
|
That is nasty user display --- it looks like line noise.
-- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB
http://enterprisedb.com
+ Everyone has their own god. +
On 12/23/2014 06:06 PM, Bruce Momjian wrote: > On Tue, Dec 23, 2014 at 11:34:09AM -0500, Tom Lane wrote: >> Stephen Frost <sfrost@snowman.net> writes: >>> If that's the only consideration for this, well, that's certainly quite >>> straight-forward to change in the other direction too. The new function >>> suggested by Andres actually makes it really easy to get a textual list >>> of all the role attributes which a role has from the bitmask too. >> We could have that regardless of the representation, if the function is >> defined along the lines of "given a user OID, give me a text string >> representing the user's attributes". However, that only helps for >> pg_dumpall and any other clients whose requirement is exactly satisfied >> by a string that fits into CREATE/ALTER USER. The current formatting >> of psql's \du, for example, absolutely requires adding more client-side >> code every time we add a property; whether the catalog representation is >> bools or a bitmask really isn't going to change the pain level much there. > I am with Tom on this --- there is more wasted space in the 'name' > column pg_authid.rolname than by shoving 40 boolean values into a > bitmap. Adding the complexity of a bitmap doesn't make sense here. Well, the code simplification alone might be worth the effort... and it does make adding additional attributes easier. > I also apologize for the late feedback. > > Offtopic, what I would really _love_ to see improved is our display of > object permissions: > > Access privileges > Schema | Name | Type | Access privileges | Column privileges | Policies > --------+--------+-------+---------------------------+-------------------+---------- > public | crypto | table | postgres=arwdDxt/postgres+| | > | | | =r/postgres | | > > That is nasty user display --- it looks like line noise. Hmm... http://www.postgresql.org/docs/9.4/static/sql-grant.html does describe the mapping from letters to permissions, but I agree that it could be easier for beginners. Any idea on how this display can be made more "human friendly"? (just for the sake of discussion --- I don't think I have time to do much about that, unfortunately) Cheers, / J.L.
José Luis Tallón-2 wrote > On 12/23/2014 06:06 PM, Bruce Momjian wrote: >> On Tue, Dec 23, 2014 at 11:34:09AM -0500, Tom Lane wrote: >>> Stephen Frost < > sfrost@ > > writes: >>>> If that's the only consideration for this, well, that's certainly quite >>>> straight-forward to change in the other direction too. The new >>>> function >>>> suggested by Andres actually makes it really easy to get a textual list >>>> of all the role attributes which a role has from the bitmask too. >>> We could have that regardless of the representation, if the function is >>> defined along the lines of "given a user OID, give me a text string >>> representing the user's attributes". However, that only helps for >>> pg_dumpall and any other clients whose requirement is exactly satisfied >>> by a string that fits into CREATE/ALTER USER. The current formatting >>> of psql's \du, for example, absolutely requires adding more client-side >>> code every time we add a property; whether the catalog representation is >>> bools or a bitmask really isn't going to change the pain level much >>> there. >> I am with Tom on this --- there is more wasted space in the 'name' >> column pg_authid.rolname than by shoving 40 boolean values into a >> bitmap. Adding the complexity of a bitmap doesn't make sense here. > Well, the code simplification alone might be worth the effort... and it > does make adding additional attributes easier. > >> I also apologize for the late feedback. >> >> Offtopic, what I would really _love_ to see improved is our display of >> object permissions: >> >> Access privileges >> Schema | Name | Type | Access privileges | Column privileges >> | Policies >> >> --------+--------+-------+---------------------------+-------------------+---------- >> public | crypto | table | postgres=arwdDxt/postgres+| >> | >> | | | =r/postgres | >> | >> >> That is nasty user display --- it looks like line noise. > > Hmm... http://www.postgresql.org/docs/9.4/static/sql-grant.html does > describe the mapping from letters to permissions, but I agree that it > could be easier for beginners. > Any idea on how this display can be made more "human friendly"? (just > for the sake of discussion --- I don't think I have time to do much > about that, unfortunately) I'm not exploring this at the moment but creating an ASCII table that looks similar to what pgAdmin outputs would be the best solution. While pgAdmin would allow for interaction on the table the presentation there is likely more user-friendly than this (not a high bar to clear...) Ideally the column headers would go vertical for narrow columns; but even using header abbreviations with a key would work but the number of columns should be constant and true indicators used to denote permission. Spatial factors (position, space/fill, size) provide stronger cues compared to trying to read a sequence of characters. David J. -- View this message in context: http://postgresql.nabble.com/Re-COMMITTERS-pgsql-Use-a-bitmask-to-represent-role-attributes-tp5831838p5831869.html Sent from the PostgreSQL - hackers mailing list archive at Nabble.com.
Bruce Momjian wrote: > I am with Tom on this --- there is more wasted space in the 'name' > column pg_authid.rolname than by shoving 40 boolean values into a > bitmap. Adding the complexity of a bitmap doesn't make sense here. I > also apologize for the late feedback. Okay, it seems we have a majority that does not want this patch -- at least Tom, Robert and Bruce plus-one'd the reversion. I am going to revert it, mainly because I don't want to be on the hook for fixing it later on. I'm also going to mark it Rejected in commitfest. Adam and Stephen can rework as they see fit, according to whatever acceptable design is found. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
* Bruce Momjian (bruce@momjian.us) wrote:
> I am with Tom on this --- there is more wasted space in the 'name'
> column pg_authid.rolname than by shoving 40 boolean values into a
> bitmap.
I agree that we waste a lot of space in 'name' but I don't think there's
an easy solution to that problem. This, on the other hand, seemed like
a relatively minor change to an internal catalog definition.
> Adding the complexity of a bitmap doesn't make sense here. I
> also apologize for the late feedback.
The complexity of the bitmap on the SQL side actually makes the code
side cleaner. :/
It would be great to figure out a way to get feedback like this earlier
on in the development. This patch has been floating around for quite a
while, with intentional breaks for feedback taken prior to incremental
improvements and documentation additions. Clearly that gets back to the
discussion around the commitfest situation.
> Offtopic, what I would really _love_ to see improved is our display of
> object permissions:
That's a whole different discussion and really belongs on a new thread.
I'm certainly curious what ideas you have regarding how to fix this;
it's not like Unix permission display is particularly elegant. Is there
something you've seen that looks nicer while still conveying the
necessary information in a relatively small amount of space?
Thanks,
Stephen
On Tue, Dec 23, 2014 at 01:43:47PM -0500, Stephen Frost wrote: > > Offtopic, what I would really _love_ to see improved is our display of > > object permissions: > > That's a whole different discussion and really belongs on a new thread. > I'm certainly curious what ideas you have regarding how to fix this; > it's not like Unix permission display is particularly elegant. Is there > something you've seen that looks nicer while still conveying the > necessary information in a relatively small amount of space? No, just hoping for something better. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +
* Alvaro Herrera (alvherre@2ndquadrant.com) wrote:
> Bruce Momjian wrote:
>
> > I am with Tom on this --- there is more wasted space in the 'name'
> > column pg_authid.rolname than by shoving 40 boolean values into a
> > bitmap. Adding the complexity of a bitmap doesn't make sense here. I
> > also apologize for the late feedback.
>
> Okay, it seems we have a majority that does not want this patch -- at
> least Tom, Robert and Bruce plus-one'd the reversion. I am going to
> revert it, mainly because I don't want to be on the hook for fixing it
> later on. I'm also going to mark it Rejected in commitfest.
Well, I'd be happy for fixing it, but that's not what is at issue here.
If it was only about getting someone to maintain it, I don't think
there'd be a discussion.
> Adam and Stephen can rework as they see fit, according to whatever
> acceptable design is found.
In the end, this is simpler, and the patch to add the other role
attributes which we were looking to add is probably closer to being done
with this outcome anyway, since it doesn't need to be rebased on top of
the bitmap work.
Thanks,
Stephen
Stephen Frost <sfrost@snowman.net> writes:
> It would be great to figure out a way to get feedback like this earlier
> on in the development. This patch has been floating around for quite a
> while, with intentional breaks for feedback taken prior to incremental
> improvements and documentation additions. Clearly that gets back to the
> discussion around the commitfest situation.
Yeah. Again, I apologize for having ignored the role-attributes thread
for a long time ... but it's not a topic I have any great interest in
personally, and frankly the -hackers traffic volume has gotten to the
point where I *can't* follow every thread. I think most of us are in
that boat actually. Not sure if there's anything to be done about it.
(It would help some if more people paid attention to not wasting their
readers' time, eg by trimming quotes rather than reposting the entirety
of some thread in order to add a paragraph here and there. But I fear
email etiquette is getting to be a lost art.)
regards, tom lane
It would be great to figure out a way to get feedback like this earlier
on in the development. This patch has been floating around for quite a
while, with intentional breaks for feedback taken prior to incremental
improvements and documentation additions. Clearly that gets back to the
discussion around the commitfest situation.
There four possible situations here:
Seen, agreeable
Seen, not agreeable
Seen, abstain
Not Seen
Tracking, for the committers in particular, the not seen and directly soliciting their agree/disagree/abstain opinion is really the only way to avoid this situation where Tom probably saw the subject lines but ended up filtering them out since his focus was elsewhere. However, something getting committed definitely gets his attention.
FWIW my initial reaction to this idea of introducing bitmaps was "why?" but I didn't have anything to go on but the feeling that bitmaps are not the most obvious API in modern coding. I didn't have anything else to support that, including coding experience, so I didn't voice it and figured when no one else did that I likely was missing something. I'm not sure how an email to Tom saying: "hey, this doesn't smell right to me" would have been taken but changing the underlying authorization mechanisms does seem like something Tom should comment on before development gets to far along - and that input should be prompted for if not seen.
That's my current feeling as strictly a monitor of these lists and observing a subset of the threads and new features that are being currently developed - take it as you will.
David J.
View this message in context: Re: [COMMITTERS] pgsql: Use a bitmask to represent role attributes
Sent from the PostgreSQL - hackers mailing list archive at Nabble.com.