Re: [HACKERS] [COMMITTERS] pgsql: Use a bitmask to represent role attributes
On Tue, Dec 23, 2014 at 11:44 AM, Stephen Frost [via PostgreSQL] < ml-node+s1045698n5831875...@n5.nabble.com> wrote: > > 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: http://postgresql.nabble.com/Re-COMMITTERS-pgsql-Use-a-bitmask-to-represent-role-attributes-tp5831838p5831894.html Sent from the PostgreSQL - hackers mailing list archive at Nabble.com.
Re: [HACKERS] [COMMITTERS] pgsql: Use a bitmask to represent role attributes
Stephen Frost 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [COMMITTERS] pgsql: Use a bitmask to represent role attributes
* Alvaro Herrera (alvhe...@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 signature.asc Description: Digital signature
Re: [HACKERS] [COMMITTERS] pgsql: Use a bitmask to represent role attributes
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 http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [COMMITTERS] pgsql: Use a bitmask to represent role attributes
* Bruce Momjian (br...@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 signature.asc Description: Digital signature
Re: [HACKERS] [COMMITTERS] pgsql: Use a bitmask to represent role attributes
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 Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [COMMITTERS] pgsql: Use a bitmask to represent role attributes
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. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [COMMITTERS] pgsql: Use a bitmask to represent role attributes
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 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. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [COMMITTERS] pgsql: Use a bitmask to represent role attributes
On Tue, Dec 23, 2014 at 11:34:09AM -0500, Tom Lane wrote: > Stephen Frost 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 http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [COMMITTERS] pgsql: Use a bitmask to represent role attributes
Stephen Frost 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [COMMITTERS] pgsql: Use a bitmask to represent role attributes
* Tom Lane (t...@sss.pgh.pa.us) wrote: > Stephen Frost writes: > > * Tom Lane (t...@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 signature.asc Description: Digital signature
Re: [HACKERS] [COMMITTERS] pgsql: Use a bitmask to represent role attributes
* Robert Haas (robertmh...@gmail.com) wrote: > On Tue, Dec 23, 2014 at 10:51 AM, Tom Lane 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 signature.asc Description: Digital signature
Re: [HACKERS] [COMMITTERS] pgsql: Use a bitmask to represent role attributes
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 Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [COMMITTERS] pgsql: Use a bitmask to represent role attributes
* Andres Freund (and...@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 signature.asc Description: Digital signature
Re: [HACKERS] [COMMITTERS] pgsql: Use a bitmask to represent role attributes
Stephen Frost writes: > * Tom Lane (t...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [COMMITTERS] pgsql: Use a bitmask to represent role attributes
On Tue, Dec 23, 2014 at 10:51 AM, Tom Lane 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [COMMITTERS] pgsql: Use a bitmask to represent role attributes
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. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [COMMITTERS] pgsql: Use a bitmask to represent role attributes
* Tom Lane (t...@sss.pgh.pa.us) wrote: > Alvaro Herrera 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.gw28...@tamriel.snowman.net#20141016133218.gw28...@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 signature.asc Description: Digital signature
Re: [HACKERS] [COMMITTERS] pgsql: Use a bitmask to represent role attributes
Robert Haas 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [COMMITTERS] pgsql: Use a bitmask to represent role attributes
Stephen Frost writes: > * Tom Lane (t...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [COMMITTERS] pgsql: Use a bitmask to represent role attributes
On 2014-12-23 10:40:15 -0500, Robert Haas wrote: > On Tue, Dec 23, 2014 at 10:26 AM, Alvaro Herrera > 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.gw28...@tamriel.snowman.net#20141016133218.gw28...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [COMMITTERS] pgsql: Use a bitmask to represent role attributes
On Tue, Dec 23, 2014 at 10:26 AM, Alvaro Herrera 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.gw28...@tamriel.snowman.net#20141016133218.gw28...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [COMMITTERS] pgsql: Use a bitmask to represent role attributes
Alvaro Herrera 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.gw28...@tamriel.snowman.net#20141016133218.gw28...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [COMMITTERS] pgsql: Use a bitmask to represent role attributes
* Tom Lane (t...@sss.pgh.pa.us) wrote: > Alvaro Herrera 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 signature.asc Description: Digital signature
Re: [HACKERS] [COMMITTERS] pgsql: Use a bitmask to represent role attributes
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.gw28...@tamriel.snowman.net#20141016133218.gw28...@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 Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [COMMITTERS] pgsql: Use a bitmask to represent role attributes
Alvaro Herrera 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers