Re: [HACKERS] Providing catalog view to pg_hba.conf file - Patch submission
On 05/16/2015 06:00 AM, Haribabu Kommi wrote: Regarding next version- are you referring to 9.6 and therefore we should go ahead and bounce this to the next CF, or were you planning to post a next version of the patch today? Yes, for 9.6 version. No new patch emerged that could be reviewed in this commitfest (July), so I've marked this as Returned with feedback. - Heikki -- 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] Providing catalog view to pg_hba.conf file - Patch submission
* Haribabu Kommi (kommi.harib...@gmail.com) wrote: On Tue, May 5, 2015 at 6:48 AM, Peter Eisentraut pete...@gmx.net wrote: On 5/1/15 12:33 PM, Andres Freund wrote: On 2015-04-08 19:19:29 +0100, Greg Stark wrote: I'm not sure what the best way to handle the hand-off from patch contribution to reviewer/committer. If I start tweaking things then you send in a new version it's actually more work to resolve the conflicts. I think at this point it's easiest if I just take it from here. Are you intending to commit this? It still looks quite dubious to me. The more I test this, the more fond I grow of the idea of having this information available in SQL. But I'm also growing more perplexed by how this the file is mapped to a table. It just isn't a good match. For instance: What is keyword_databases? Why is it an array? Same for keyword_users. How can I know whether a given database or user matches a keyword? What is compare_method? (Should perhaps be keyword_address?) Why is compare method set to mask when a hostname is set? (Column order is also a bit confusing here.) I'd also like options to be jsonb instead of a text array. Thanks for your suggestion. I am not sure how to use jsonb here, i will study the same and provide a patch for the next version. Regarding next version- are you referring to 9.6 and therefore we should go ahead and bounce this to the next CF, or were you planning to post a next version of the patch today? This is certainly a capability which I'd like to see, though I share Peter's concerns regarding the splitting up of the keywords rather than keeping the same structure as what's in the actual pg_hba.conf. That strikes me as confusing. It'd be neat if we were able to change pg_hba.conf to make more sense and then perhaps the SQL version wouldn't look so different but I don't think there's any way to do that. I discussed the patch briefing with Greg over IM, who pointed out that keeping things just exactly as they are in the config file would mean implementing, essentially, a pg_hba.conf parser in SQL. I can understand that perspective, but I don't think there's really much hope in users being able to use this view directly without a lot of effort, regardless. We need to provide a function which takes the arguments that our pg_hba lookup does (database, user-to-login-as, maybe system user for pg_ident checks, optionally an IP, etc) and then returns the record that matches. Apologies for not being able to provide more feedback earlier. I'll be happy to help with all of the above and review the patch. Independently, I'd love to see an SQL interface to pg_ident.conf too, where, I expect anyway, it'll be a lot simpler, though I'm not sure that it's very useful until we also have pg_hba.conf available through SQL. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] Providing catalog view to pg_hba.conf file - Patch submission
On Fri, May 15, 2015 at 11:24 PM, Stephen Frost sfr...@snowman.net wrote: * Haribabu Kommi (kommi.harib...@gmail.com) wrote: On Tue, May 5, 2015 at 6:48 AM, Peter Eisentraut pete...@gmx.net wrote: It still looks quite dubious to me. The more I test this, the more fond I grow of the idea of having this information available in SQL. But I'm also growing more perplexed by how this the file is mapped to a table. It just isn't a good match. For instance: What is keyword_databases? Why is it an array? Same for keyword_users. How can I know whether a given database or user matches a keyword? What is compare_method? (Should perhaps be keyword_address?) Why is compare method set to mask when a hostname is set? (Column order is also a bit confusing here.) I'd also like options to be jsonb instead of a text array. Thanks for your suggestion. I am not sure how to use jsonb here, i will study the same and provide a patch for the next version. Regarding next version- are you referring to 9.6 and therefore we should go ahead and bounce this to the next CF, or were you planning to post a next version of the patch today? Yes, for 9.6 version. This is certainly a capability which I'd like to see, though I share Peter's concerns regarding the splitting up of the keywords rather than keeping the same structure as what's in the actual pg_hba.conf. That strikes me as confusing. It'd be neat if we were able to change pg_hba.conf to make more sense and then perhaps the SQL version wouldn't look so different but I don't think there's any way to do that. I discussed the patch briefing with Greg over IM, who pointed out that keeping things just exactly as they are in the config file would mean implementing, essentially, a pg_hba.conf parser in SQL. I can understand that perspective, but I don't think there's really much hope in users being able to use this view directly without a lot of effort, regardless. We need to provide a function which takes the arguments that our pg_hba lookup does (database, user-to-login-as, maybe system user for pg_ident checks, optionally an IP, etc) and then returns the record that matches. Thanks for details. I will try to come up with a view and a function by considering all the above for the next commitfest. Apologies for not being able to provide more feedback earlier. I'll be happy to help with all of the above and review the patch. Independently, I'd love to see an SQL interface to pg_ident.conf too, where, I expect anyway, it'll be a lot simpler, though I'm not sure that it's very useful until we also have pg_hba.conf available through SQL. Yes, Definitely I look into pg_ident also along with pg_hba. Regards, Hari Babu Fujitsu Australia -- 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] Providing catalog view to pg_hba.conf file - Patch submission
On Tue, May 5, 2015 at 6:48 AM, Peter Eisentraut pete...@gmx.net wrote: On 5/1/15 12:33 PM, Andres Freund wrote: On 2015-04-08 19:19:29 +0100, Greg Stark wrote: I'm not sure what the best way to handle the hand-off from patch contribution to reviewer/committer. If I start tweaking things then you send in a new version it's actually more work to resolve the conflicts. I think at this point it's easiest if I just take it from here. Are you intending to commit this? It still looks quite dubious to me. The more I test this, the more fond I grow of the idea of having this information available in SQL. But I'm also growing more perplexed by how this the file is mapped to a table. It just isn't a good match. For instance: What is keyword_databases? Why is it an array? Same for keyword_users. How can I know whether a given database or user matches a keyword? What is compare_method? (Should perhaps be keyword_address?) Why is compare method set to mask when a hostname is set? (Column order is also a bit confusing here.) I'd also like options to be jsonb instead of a text array. Thanks for your suggestion. I am not sure how to use jsonb here, i will study the same and provide a patch for the next version. Regards, Hari Babu Fujitsu Australia -- 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] Providing catalog view to pg_hba.conf file - Patch submission
On 5/1/15 12:33 PM, Andres Freund wrote: On 2015-04-08 19:19:29 +0100, Greg Stark wrote: I'm not sure what the best way to handle the hand-off from patch contribution to reviewer/committer. If I start tweaking things then you send in a new version it's actually more work to resolve the conflicts. I think at this point it's easiest if I just take it from here. Are you intending to commit this? It still looks quite dubious to me. The more I test this, the more fond I grow of the idea of having this information available in SQL. But I'm also growing more perplexed by how this the file is mapped to a table. It just isn't a good match. For instance: What is keyword_databases? Why is it an array? Same for keyword_users. How can I know whether a given database or user matches a keyword? What is compare_method? (Should perhaps be keyword_address?) Why is compare method set to mask when a hostname is set? (Column order is also a bit confusing here.) I'd also like options to be jsonb instead of a text array. Ultimately, I don't see how this is better than just showing the raw file. I don't see a sensible way to compute answers to questions such as What is the authentication method for user X from IP address Y. If I can't do that, what's the point of having a processed version? -- 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] Providing catalog view to pg_hba.conf file - Patch submission
On 2015-04-08 19:19:29 +0100, Greg Stark wrote: I'm not sure what the best way to handle the hand-off from patch contribution to reviewer/committer. If I start tweaking things then you send in a new version it's actually more work to resolve the conflicts. I think at this point it's easiest if I just take it from here. Are you intending to commit this? Greetings, Andres Freund -- 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] Providing catalog view to pg_hba.conf file - Patch submission
I'm not sure what the best way to handle the hand-off from patch contribution to reviewer/committer. If I start tweaking things then you send in a new version it's actually more work to resolve the conflicts. I think at this point it's easiest if I just take it from here. I'm puzzled about the change from pg_hba_settings to pg_hba_conf. They're both ok but if pressed I would have preferred the original pg_hba_settings since that's consistent with the pg_settings view. There's no reason to quote tokens, that defeats the whole point of breaking the keywords into a separate column. Also there's no point in breaking out all but then still leaving +foo in the same column. My version had that moved into a new column as well called recursive_roles. We could call that just roles or groups but I was opting to the more explicit. -- greg -- 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] Providing catalog view to pg_hba.conf file - Patch submission
2015-04-04 15:29 GMT+02:00 Haribabu Kommi kommi.harib...@gmail.com: On Sat, Apr 4, 2015 at 4:19 PM, Pavel Stehule pavel.steh...@gmail.com wrote: Hi 2015-03-31 14:38 GMT+02:00 Haribabu Kommi kommi.harib...@gmail.com: keyword_databases - The database name can be all, replication, sameuser, samerole and samegroup. keyword_roles - The role can be all and a group name prefixed with +. I am not very happy about name - but I have no better idea :( - maybe database_mask, user_mask How about database_keywords and user_keywords as column names? I am thinking, it is better than keyword_databases - it is more logical. But it is maybe question for mathematicians. some other variant database_filters and user_filters or database_predicates and user_predicates but it is not terrible nice too Regards Pavel The rest of the database and role names are treated as normal database and role names. 2. Added the code changes to identify the names with quoted. Is it a good idea? Database's names are not quoted every else (in system catalog). So now, we cannot to use join to this view. postgres=# select (databases)[1] from pg_hba_conf ; databases --- omega 2 (4 rows) postgres=# select datname from pg_database ; datname --- template1 template0 postgres omega 2 (4 rows) I dislike this - we know, so the name must be quoted in file (without it, the file was incorrect). And if you need quotes, there is a function quote_ident. If we use quotes elsewhere, then it should be ok, bot not now. Please, remove it. More, it is not necessary, when you use a keyword columns. Thanks. The special handling of quotes for pg_hba_conf is not required. I will keep the behaviour similar to the other catalog tables. I will remove the quote support in the next version. Regards, Hari Babu Fujitsu Australia
Re: [HACKERS] Providing catalog view to pg_hba.conf file - Patch submission
On Sat, Apr 4, 2015 at 4:19 PM, Pavel Stehule pavel.steh...@gmail.com wrote: Hi 2015-03-31 14:38 GMT+02:00 Haribabu Kommi kommi.harib...@gmail.com: keyword_databases - The database name can be all, replication, sameuser, samerole and samegroup. keyword_roles - The role can be all and a group name prefixed with +. I am not very happy about name - but I have no better idea :( - maybe database_mask, user_mask How about database_keywords and user_keywords as column names? The rest of the database and role names are treated as normal database and role names. 2. Added the code changes to identify the names with quoted. Is it a good idea? Database's names are not quoted every else (in system catalog). So now, we cannot to use join to this view. postgres=# select (databases)[1] from pg_hba_conf ; databases --- omega 2 (4 rows) postgres=# select datname from pg_database ; datname --- template1 template0 postgres omega 2 (4 rows) I dislike this - we know, so the name must be quoted in file (without it, the file was incorrect). And if you need quotes, there is a function quote_ident. If we use quotes elsewhere, then it should be ok, bot not now. Please, remove it. More, it is not necessary, when you use a keyword columns. Thanks. The special handling of quotes for pg_hba_conf is not required. I will keep the behaviour similar to the other catalog tables. I will remove the quote support in the next version. Regards, Hari Babu Fujitsu Australia -- 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] Providing catalog view to pg_hba.conf file - Patch submission
Hi 2015-03-31 14:38 GMT+02:00 Haribabu Kommi kommi.harib...@gmail.com: On Mon, Mar 30, 2015 at 4:34 AM, Pavel Stehule pavel.steh...@gmail.com wrote: Hi I checked this patch. I like the functionality and behave. Thanks for the review. Here I attached updated patch with the following changes. 1. Addition of two new keyword columns keyword_databases - The database name can be all, replication, sameuser, samerole and samegroup. keyword_roles - The role can be all and a group name prefixed with +. I am not very happy about name - but I have no better idea :( - maybe database_mask, user_mask The rest of the database and role names are treated as normal database and role names. 2. Added the code changes to identify the names with quoted. Is it a good idea? Database's names are not quoted every else (in system catalog). So now, we cannot to use join to this view. postgres=# select (databases)[1] from pg_hba_conf ; databases --- omega 2 (4 rows) postgres=# select datname from pg_database ; datname --- template1 template0 postgres omega 2 (4 rows) I dislike this - we know, so the name must be quoted in file (without it, the file was incorrect). And if you need quotes, there is a function quote_ident. If we use quotes elsewhere, then it should be ok, bot not now. Please, remove it. More, it is not necessary, when you use a keyword columns. Regards Pavel 3. Updated documentation changes 4. Regression test is corrected. Regards, Hari Babu Fujitsu Australia
Re: [HACKERS] Providing catalog view to pg_hba.conf file - Patch submission
On Mon, Mar 30, 2015 at 4:34 AM, Pavel Stehule pavel.steh...@gmail.com wrote: Hi I checked this patch. I like the functionality and behave. Thanks for the review. Here I attached updated patch with the following changes. 1. Addition of two new keyword columns keyword_databases - The database name can be all, replication, sameuser, samerole and samegroup. keyword_roles - The role can be all and a group name prefixed with +. The rest of the database and role names are treated as normal database and role names. 2. Added the code changes to identify the names with quoted. 3. Updated documentation changes 4. Regression test is corrected. Regards, Hari Babu Fujitsu Australia Catalog_view_to_HBA_settings_patch_V9.patch Description: Binary data -- 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] Providing catalog view to pg_hba.conf file - Patch submission
Hi I checked this patch. I like the functionality and behave. There is minor issue with outdated regress test test rules... FAILED I have no objections. Regards Pavel 2015-03-27 9:23 GMT+01:00 Haribabu Kommi kommi.harib...@gmail.com: On Fri, Mar 13, 2015 at 1:33 PM, Peter Eisentraut pete...@gmx.net wrote: On 3/4/15 1:34 AM, Haribabu Kommi wrote: On Wed, Mar 4, 2015 at 12:35 PM, Haribabu Kommi kommi.harib...@gmail.com wrote: + foreach(line, parsed_hba_lines) In the above for loop it is better to add check_for_interrupts to avoid it looping if the parsed_hba_lines are more. Updated patch is attached with the addition of check_for_interrupts in the for loop. I tried out your latest patch. I like that it updates even in running sessions when the file is reloaded. Thanks for the review. Sorry for late reply. The permission checking is faulty, because unprivileged users can execute pg_hba_settings() directly. corrected. Check the error messages against the style guide (especially capitalization). corrected. I don't like that there is a hard-coded limit of 16 options 5 pages away from where it is actually used. That could be done better. changed to 12 instead of 16. I'm not sure about the name pg_hba_settings. Why not just pg_hba or pg_hba_conf if you're going for a representation that is close to the file (as opposed to pg_settings, which is really a lot more abstract than any particular file). changed to pg_hba_conf. I would put the line_number column first. changed. I continue to think that it is a serious mistake to stick special values like 'all' and 'replication' into the arrays without additional decoration. That makes this view useless or at least dangerous for editing tools or tools that want to reassemble the original file. Clearly at least one of those has to be a use case. Otherwise we can just print out the physical lines without interpretation. It is possible to provide more than one keyword for databases or users. Is it fine to use the text array for keyword databases and keyword users. The mask field can go, because address is of type inet, which can represent masks itself. (Or should it be cidr then? Who knows.) The preferred visual representation of masks in pg_hba.conf has been address/mask for a while now, so we should preserve that. Additionally, you can then use the existing inet/cidr operations to do things like checking whether some IP address is contained in an address specification. removed. I can't tell from the documentation what the compare_method field is supposed to do. I see it on the code, but that is not a natural representation of pg_hba.conf. In fact, this just supports my earlier statement. Why are special values in the address field special, but not in the user or database fields? uaImplicitReject is not a user-facing authentication method, so it shouldn't be shown (or showable). removed. I would have expected options to be split into keys and values. All that code to reassemble the options from the parsed struct representation seems crazy to me. Surely, we could save the strings as we parse them? I didn't get this point clearly. Can you explain it a bit more. I can't make sense of the log message pg_hba.conf not reloaded, pg_hba_settings may show stale information. If load_hba() failed, the information isn't stale, is it? In any case, printing this to the server log seems kind of pointless, because someone using the view is presumably doing so because they can't or don't want to read the server log. The proper place might be a warning when the view is actually called. Changed accordingly as if the reload fails, further selects on the view through a warning as view data may not be proper like below. There was some failure in reloading pg_hba.conf file. The pg_hba.conf settings data may contains stale information Here I attached latest patch with the fixes other than keyword columns. I will provide the patch with keyword columns and documentation changes later. Regards, Hari Babu Fujitsu Australia -- 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] Providing catalog view to pg_hba.conf file - Patch submission
On Fri, Mar 13, 2015 at 1:33 PM, Peter Eisentraut pete...@gmx.net wrote: On 3/4/15 1:34 AM, Haribabu Kommi wrote: On Wed, Mar 4, 2015 at 12:35 PM, Haribabu Kommi kommi.harib...@gmail.com wrote: + foreach(line, parsed_hba_lines) In the above for loop it is better to add check_for_interrupts to avoid it looping if the parsed_hba_lines are more. Updated patch is attached with the addition of check_for_interrupts in the for loop. I tried out your latest patch. I like that it updates even in running sessions when the file is reloaded. Thanks for the review. Sorry for late reply. The permission checking is faulty, because unprivileged users can execute pg_hba_settings() directly. corrected. Check the error messages against the style guide (especially capitalization). corrected. I don't like that there is a hard-coded limit of 16 options 5 pages away from where it is actually used. That could be done better. changed to 12 instead of 16. I'm not sure about the name pg_hba_settings. Why not just pg_hba or pg_hba_conf if you're going for a representation that is close to the file (as opposed to pg_settings, which is really a lot more abstract than any particular file). changed to pg_hba_conf. I would put the line_number column first. changed. I continue to think that it is a serious mistake to stick special values like 'all' and 'replication' into the arrays without additional decoration. That makes this view useless or at least dangerous for editing tools or tools that want to reassemble the original file. Clearly at least one of those has to be a use case. Otherwise we can just print out the physical lines without interpretation. It is possible to provide more than one keyword for databases or users. Is it fine to use the text array for keyword databases and keyword users. The mask field can go, because address is of type inet, which can represent masks itself. (Or should it be cidr then? Who knows.) The preferred visual representation of masks in pg_hba.conf has been address/mask for a while now, so we should preserve that. Additionally, you can then use the existing inet/cidr operations to do things like checking whether some IP address is contained in an address specification. removed. I can't tell from the documentation what the compare_method field is supposed to do. I see it on the code, but that is not a natural representation of pg_hba.conf. In fact, this just supports my earlier statement. Why are special values in the address field special, but not in the user or database fields? uaImplicitReject is not a user-facing authentication method, so it shouldn't be shown (or showable). removed. I would have expected options to be split into keys and values. All that code to reassemble the options from the parsed struct representation seems crazy to me. Surely, we could save the strings as we parse them? I didn't get this point clearly. Can you explain it a bit more. I can't make sense of the log message pg_hba.conf not reloaded, pg_hba_settings may show stale information. If load_hba() failed, the information isn't stale, is it? In any case, printing this to the server log seems kind of pointless, because someone using the view is presumably doing so because they can't or don't want to read the server log. The proper place might be a warning when the view is actually called. Changed accordingly as if the reload fails, further selects on the view through a warning as view data may not be proper like below. There was some failure in reloading pg_hba.conf file. The pg_hba.conf settings data may contains stale information Here I attached latest patch with the fixes other than keyword columns. I will provide the patch with keyword columns and documentation changes later. Regards, Hari Babu Fujitsu Australia Catalog_view_to_HBA_settings_patch_V8.patch Description: Binary data -- 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] Providing catalog view to pg_hba.conf file - Patch submission
Robert Haas wrote: On Wed, Mar 11, 2015 at 1:32 PM, Greg Stark st...@mit.edu wrote: I think what we have here is already a good semantic representation. It doesn't handle all the corner cases but those corner cases are a) very unlikely and b) easy to check for. A tool can check for any users starting with + or named all or any databases called sameuser or samerole. If they exist then the view isn't good enough to reconstruct the raw file. But they're very unlikely to exist, I've never heard of anyone with such things and can't imagine why someone would make them. -1. Like Peter, I think this is a bad plan. Somebody looking at the view should be able to understand with 100% confidence, and without additional parsing, what the semantics of the pg_hba.conf file are. Saying those cases are unlikely so we're not going to handle them is really selling ourselves short. +1 what Robert said. I think the additional keyword columns are a good solution to the issue. -- Á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] Providing catalog view to pg_hba.conf file - Patch submission
On Mon, Mar 16, 2015 at 4:29 PM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: +1 what Robert said. I think the additional keyword columns are a good solution to the issue. Huh. Well I disagree but obviously I'm in the minority. I'll put fix it up accordingly today and post the resulting view output (which I expect will look like the example in the previous message). I think it's cumbersome but it shouldn't be hard to implement. -- greg
Re: [HACKERS] Providing catalog view to pg_hba.conf file - Patch submission
On Wed, Mar 11, 2015 at 1:32 PM, Greg Stark st...@mit.edu wrote: I think what we have here is already a good semantic representation. It doesn't handle all the corner cases but those corner cases are a) very unlikely and b) easy to check for. A tool can check for any users starting with + or named all or any databases called sameuser or samerole. If they exist then the view isn't good enough to reconstruct the raw file. But they're very unlikely to exist, I've never heard of anyone with such things and can't imagine why someone would make them. -1. Like Peter, I think this is a bad plan. Somebody looking at the view should be able to understand with 100% confidence, and without additional parsing, what the semantics of the pg_hba.conf file are. Saying those cases are unlikely so we're not going to handle them is really selling ourselves short. -- 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] Providing catalog view to pg_hba.conf file - Patch submission
On Mon, Mar 16, 2015 at 9:29 AM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: Robert Haas wrote: On Wed, Mar 11, 2015 at 1:32 PM, Greg Stark st...@mit.edu wrote: I think what we have here is already a good semantic representation. It doesn't handle all the corner cases but those corner cases are a) very unlikely and b) easy to check for. A tool can check for any users starting with + or named all or any databases called sameuser or samerole. If they exist then the view isn't good enough to reconstruct the raw file. But they're very unlikely to exist, I've never heard of anyone with such things and can't imagine why someone would make them. -1. Like Peter, I think this is a bad plan. Somebody looking at the view should be able to understand with 100% confidence, and without additional parsing, what the semantics of the pg_hba.conf file are. Saying those cases are unlikely so we're not going to handle them is really selling ourselves short. +1 what Robert said. I think the additional keyword columns are a good solution to the issue. Why not just leave the double-quoting requirements intact. An unquoted any or sameuser (etc) would represent the special keyword while the quoted version would mean that the name is used literally. Have the: A separate file containing [database|user] names can be specified by preceding the file name with @ possibilities been added to the test suite? I'm not totally opposed to using some other quoting symbol to represent keywords as well - like * (e.g., *all*). Like Greg, I'm not to keen on the idea of adding additional keyword columns. *Logical View - Keyword Expansion* My other thought was to not use the keywords at all and simply resolve their meaning to actual role/database names and list them explicitly. That would be a true logical view and allow for simple user checking by saying: 'username' = ANY(users); without the need for ([...] OR '*any*' = ANY(users) or similar). If going that route I guess it would behoove us to either incorporate a physical view of the actual file converted to a table or to then maybe have some kind of tags fields with the special values encoded should someone want to know how the user list was generated. In effect, the pg_hba view in the original file but with actual names of users and databases instead of empty arrays. The sameuser = all pairing is a pure physical representation in that instance. What I would do in a logical representation is have a single record for each user that has a database of the same name in the current database. However, because of that limitation you either would be duplicating information by keeping sameuser,all or you would have to have a separate view representing the physical view of the file. I think having two views, one logical and one physical, would solve the two use cases nicely without having to compromise or incorporate too much redundancy and complexity. Likewise, if we know the host ip address and subnet mask the keywords samehost and samenet should be resolvable to actual values at the time of viewing. Though that would add the complication of multi-network listening... I guess a full logical view is a bit much but if we keep the same quoting mechanics as mandated by the file then there should be no ambiguity in terms of label meaning and whomever is looking at this stuff has to understand about the keywords and using quote to remove the special-ness anyway. David J.
Re: [HACKERS] Providing catalog view to pg_hba.conf file - Patch submission
On Mon, Mar 16, 2015 at 1:46 PM, David G. Johnston david.g.johns...@gmail.com wrote: Why not just leave the double-quoting requirements intact. An unquoted any or sameuser (etc) would represent the special keyword while the quoted version would mean that the name is used literally. That would be OK with me, I think. I'm not totally opposed to using some other quoting symbol to represent keywords as well - like * (e.g., *all*). Like Greg, I'm not to keen on the idea of adding additional keyword columns. We probably should not use one quoting convention in the file and another in the view. -- 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] Providing catalog view to pg_hba.conf file - Patch submission
On Mon, Mar 16, 2015 at 5:46 PM, David G. Johnston david.g.johns...@gmail.com wrote: Why not just leave the double-quoting requirements intact. An unquoted any or sameuser (etc) would represent the special keyword while the quoted version would mean that the name is used literally. For users that would be worse than not quoting. Then if they look up users they can't say WHERE username =ANY (users). They would have to do sumersaults like CASE WHEN username = 'all' then 'all' =ANY (users) else username =ALL (users). The whole point of having a view should be that you don't need to know the syntax rules for pg_hba.conf to interpret the data. If you do then you might as well just write a parser and read the file. -- greg
Re: [HACKERS] Providing catalog view to pg_hba.conf file - Patch submission
On Mon, Mar 16, 2015 at 11:11 AM, Greg Stark st...@mit.edu wrote: On Mon, Mar 16, 2015 at 5:46 PM, David G. Johnston david.g.johns...@gmail.com wrote: Why not just leave the double-quoting requirements intact. An unquoted any or sameuser (etc) would represent the special keyword while the quoted version would mean that the name is used literally. For users that would be worse than not quoting. Then if they look up users they can't say WHERE username =ANY (users). They would have to do sumersaults like CASE WHEN username = 'all' then 'all' =ANY (users) else username =ALL (users). The whole point of having a view should be that you don't need to know the syntax rules for pg_hba.conf to interpret the data. If you do then you might as well just write a parser and read the file. Create a pg_hba_user type, and an implicit cast from text to that type, so when you say: WHERE 'any' = ANY(...) the system does the syntax conversion for you and the user doesn't have to, for the most part, be aware of the special rules for quoting names. Otherwise I don't see how you can meet the requirement to accommodate any as a valid user identifier without using two columns - one for keywords and one for users using the quoting rules of PostgreSQL pg_role instead of using the, more restrictive, rules of pg_hba.conf In that case I would not leave the users column with an empty array when any is specified but would incorporate all known roles into the value; though maybe it is just noise during typical usage...it would likely be a long field that would be useful for querying but not necessarily for display. David J.
Re: [HACKERS] Providing catalog view to pg_hba.conf file - Patch submission
On 3/5/15 9:42 AM, Greg Stark wrote: Well if you want to read the file as is you can do so using the file reading functions which afaik were specifically intended for the purpose of writing config editing tools. Sure, but those things are almost never installed by default, and I don't want to install them if they provide easy write access, and they don't help you find the actual file. This proposal is much nicer. Joining against other catalog tables may be kind of exaggerated but if I have data in an SQL view I certainly expect to be able to write WHERE clauses to find the rows I want without having to do string parsing. If it were a comma-delimited string I have to do something like WHERE users LIKE '%,username,%' but then that's not good enough since it may be the first or last and the username itself may contain white-space or a comma etc etc. The point is, it should be one or the other (or both), not something in the middle. It's either a textual representation of the file or a semantic one. If it's the latter, then all user names, group names, and special key words need to be resolved in some way that they cannot be confused with unfortunately named user names. And there needs to be a way that we can add more in the future. -- 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] Providing catalog view to pg_hba.conf file - Patch submission
2015-03-04 22:41 GMT+01:00 Peter Eisentraut pete...@gmx.net: On 3/3/15 7:17 PM, Jim Nasby wrote: I think we're screwed in that regard anyway, because of the special constructs. You'd need different logic to handle things like +role and sameuser. We might even end up painted in a corner where we can't change it in the future because it'll break everyone's scripts. Yeah, I'm getting worried about this. I think most people agree that getting a peek at pg_hba.conf from within the server is useful, but everyone seems to have quite different uses for it. Greg wants to join against other catalog tables, Jim wants to reassemble a valid and accurate pg_hba.conf, Josh wants to write an editing tool. Personally, I'd like to see something as close to the actual file as possible. If there were an obviously correct way to map the various special constructs to the available SQL data types, then fine. But if there isn't, then we shouldn't give a false overinterpretation. So I'd render everything that's disputed as a plain text field. (Not even an array of text.) I disagree with last note - arrays where is expected are valid. I don't see any reason why anybody have to do parsing some informations from any table. The face of pg_hba.conf in SQL space can be different than original file - but all data should be clean (without necessity of additional parsing) Regards Pavel -- 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] Providing catalog view to pg_hba.conf file - Patch submission
On Wed, Mar 4, 2015 at 9:41 PM, Peter Eisentraut pete...@gmx.net wrote: everyone seems to have quite different uses for it. Greg wants to join against other catalog tables, Jim wants to reassemble a valid and accurate pg_hba.conf, Josh wants to write an editing tool. Personally, I'd like to see something as close to the actual file as possible. Well if you want to read the file as is you can do so using the file reading functions which afaik were specifically intended for the purpose of writing config editing tools. Just to repeat, even if you're reassembling a valid and accurage pg_hba.conf or writing an editing tool you can do that with what we have today as long as there are no databases named all, sameuser, or samerole and no roles named all. That's something an editing tool could check and provide a warning for to the user and refuse to write a config file if it's the case and I doubt there are any such users out there anyways. Having five separate columns would work but I just think it's way more complicated than necessary and burdens everyone who wants to use the view less formally. If there were an obviously correct way to map the various special constructs to the available SQL data types, then fine. But if there isn't, then we shouldn't give a false overinterpretation. So I'd render everything that's disputed as a plain text field. (Not even an array of text.) Joining against other catalog tables may be kind of exaggerated but if I have data in an SQL view I certainly expect to be able to write WHERE clauses to find the rows I want without having to do string parsing. If it were a comma-delimited string I have to do something like WHERE users LIKE '%,username,%' but then that's not good enough since it may be the first or last and the username itself may contain white-space or a comma etc etc. I think knowing about the + prefix and the risk of tokens like all and sameuser etc are pretty small compromises to make. But having to know the quoting rules and write a tokenizer are too far. -- greg -- 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] Providing catalog view to pg_hba.conf file - Patch submission
On 3/3/15 7:17 PM, Jim Nasby wrote: I think we're screwed in that regard anyway, because of the special constructs. You'd need different logic to handle things like +role and sameuser. We might even end up painted in a corner where we can't change it in the future because it'll break everyone's scripts. Yeah, I'm getting worried about this. I think most people agree that getting a peek at pg_hba.conf from within the server is useful, but everyone seems to have quite different uses for it. Greg wants to join against other catalog tables, Jim wants to reassemble a valid and accurate pg_hba.conf, Josh wants to write an editing tool. Personally, I'd like to see something as close to the actual file as possible. If there were an obviously correct way to map the various special constructs to the available SQL data types, then fine. But if there isn't, then we shouldn't give a false overinterpretation. So I'd render everything that's disputed as a plain text field. (Not even an array of text.) -- 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] Providing catalog view to pg_hba.conf file - Patch submission
On Wed, Mar 4, 2015 at 1:35 AM, Haribabu Kommi kommi.harib...@gmail.com wrote: I feel there is no problem of current pg_hba reloads, because the check_for_interrupts doesn't reload the conf files. It will be done in the postgresMain function once the query finishes. Am I missing something? Ah, no I guess that's right. I even noticed that earlier but forgot. -- greg -- 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] Providing catalog view to pg_hba.conf file - Patch submission
On Mon, Jun 30, 2014 at 8:06 AM, Abhijit Menon-Sen a...@2ndquadrant.com wrote:
After sleeping on it, I realised that the code would return '{all}' for
'all' in pg_hba.conf, but '{all}' for 'all'. So it's not exactly
ambiguous, but I don't think it's especially useful for callers.
Hm. Nope, it doesn't. It just says {all} regardless of whether all
is quoted or not.
This makes sense, the rules for when to quote things in pg_hba and
when to quote things for arrays are separate. And we definitely don't
want to start adding quotes to every token that the parser noted was
quoted because then you'll start seeing things like {\all\} and
{\database with space\} which won't make it any easier to match
things.
I'm not sure adding a separate column is really the solution either.
You can have things like all,databasename (which is the keyword all
not a database all). Or more likely something like sameuser,bob or
even things like replication,all.
I'm thinking leaving well enough alone is probably best. It's not
perfect but if a user does have a database named all or
replication or a user named sameuser or samerole then it's not
like pg_hba_settings crashes or anything, it just produces information
that's hard to interpret and the response might just be don't do
that.
The only other option I was pondering was using a jsonb instead of an
array. That would give us more flexibility and we could have a json
array that contained strings and objects representing keywords. But
most hba files consist *mostly* of singleton keywords, so the result
might be kind of cumbersome.
--
greg
--
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] Providing catalog view to pg_hba.conf file - Patch submission
On 3/3/15 9:08 AM, Greg Stark wrote:
On Mon, Jun 30, 2014 at 8:06 AM, Abhijit Menon-Sen a...@2ndquadrant.com wrote:
After sleeping on it, I realised that the code would return '{all}' for
'all' in pg_hba.conf, but '{all}' for 'all'. So it's not exactly
ambiguous, but I don't think it's especially useful for callers.
Hm. Nope, it doesn't. It just says {all} regardless of whether all
is quoted or not.
This makes sense, the rules for when to quote things in pg_hba and
when to quote things for arrays are separate. And we definitely don't
want to start adding quotes to every token that the parser noted was
quoted because then you'll start seeing things like {\all\} and
{\database with space\} which won't make it any easier to match
things.
I'm not sure adding a separate column is really the solution either.
You can have things like all,databasename (which is the keyword all
not a database all). Or more likely something like sameuser,bob or
even things like replication,all.
What about a separate column that's just the text from pg_hba? Or is
that what you're opposed to?
FWIW, I'd say that having the individual array elements be correct is
more important than what the result of array_out is. That way you could
always do array_to_string(..., ', ') and get valid pg_hba output.
--
Jim Nasby, Data Architect, Blue Treble Consulting
Data in Trouble? Get it in Treble! http://BlueTreble.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] Providing catalog view to pg_hba.conf file - Patch submission
On Tue, Mar 3, 2015 at 6:05 PM, Jim Nasby jim.na...@bluetreble.com wrote: What about a separate column that's just the text from pg_hba? Or is that what you're opposed to? I'm not sure what you mean by that. There's a rawline field we could put somewhere but it contains the entire line. FWIW, I'd say that having the individual array elements be correct is more important than what the result of array_out is. That way you could always do array_to_string(..., ', ') and get valid pg_hba output. Well I don't think you can get that without making the view less useful for every other purpose. Like, I would want to be able to do WHERE user @ array[?] or WHERE database = array[?] or to join against a list of users or databases somewhere else. To do what you suggest would mean the tokens will need to be quoted based on pg_hba.conf syntax requirements. That would mean I would need to check each variable or join value against pg_hba.conf's quoting requirements to compare with it. It seems more practical to have that knowledge if you're actually going to generate a pg_hba.conf than to pass around these quoted strings all the time. On further review I've made a few more changes attached. I think we should change the column names to users and databases to be clear they're lists and also to avoid the user SQL reserved word. I removed the dependency on strlist_to_array which is in objectaddress.c which isn't a very sensible dependency -- it does seem like it would be handy to have a list-based version of construct_array moved to arrayfuncs.c but for now it's not much more work to handle these ourselves. I changed the options to accumulate one big array instead of an array of bunches of options. Previously you could only end up with a singleton array with a comma-delimited string or a two element array with one of those and map=. I think the error if pg_hba fails to reload needs to be LOG. It would be too unexpected to the user who isn't necessarily the one who issued the SIGHUP to spontaneously get a warning. I also removed the mask from local entries and made some of the NULLS that shouldn't be possible to happen (unknown auth method or connection method) actually throw errors. -- greg *** a/doc/src/sgml/catalogs.sgml --- b/doc/src/sgml/catalogs.sgml *** *** 7393,7398 --- 7393,7403 entryviews/entry /row + row + entrylink linkend=view-pg-hba-settingsstructnamepg_hba_settings/structname/link/entry + entryclient authentication settings/entry + /row + /tbody /tgroup /table *** *** 9696,9699 SELECT * FROM pg_locks pl LEFT JOIN pg_prepared_xacts ppx --- 9701,9786 /sect1 + sect1 id=view-pg-hba-settings + titlestructnamepg_hba_settings/structname/title + + indexterm zone=view-pg-hba-settings +primarypg_hba_settings/primary + /indexterm + + para +The read-only structnamepg_hba_settings/structname view provides +access to the client authentication configuration from pg_hba.conf. +Access to this view is limited to superusers. + /para + + table +titlestructnamepg_hba_settings/ Columns/title + +tgroup cols=3 + thead + row + entryName/entry + entryType/entry + entryDescription/entry + /row + /thead + tbody + row + entrystructfieldtype/structfield/entry + entrytypetext/type/entry + entryType of connection/entry + /row + row + entrystructfielddatabases/structfield/entry + entrytypetext[]/type/entry + entryList of database names/entry + /row + row + entrystructfieldusers/structfield/entry + entrytypetext[]/type/entry + entryList of user names/entry + /row + row + entrystructfieldaddress/structfield/entry + entrytypeinet/type/entry + entryClient machine address/entry + /row + row + entrystructfieldmask/structfield/entry + entrytypeinet/type/entry + entryIP Mask/entry + /row + row + entrystructfieldcompare_method/structfield/entry + entrytypetext/type/entry + entryIP address comparison method/entry + /row + row + entrystructfieldhostname/structfield/entry + entrytypetext/type/entry + entryClient host name/entry + /row + row + entrystructfieldmethod/structfield/entry + entrytypetext/type/entry + entryAuthentication method/entry + /row + row + entrystructfieldoptions/structfield/entry + entrytypetext[]/type/entry + entryConfiguration options set for authentication method/entry + /row + row + entrystructfieldline_number/structfield/entry + entrytypeinteger/type/entry + entry +Line number within client authentication configuration file +the current value was set at + /entry + /row + /tbody +/tgroup + /table + /sect1 /chapter
Re: [HACKERS] Providing catalog view to pg_hba.conf file - Patch submission
On Wed, Mar 4, 2015 at 12:35 PM, Haribabu Kommi kommi.harib...@gmail.com wrote: + foreach(line, parsed_hba_lines) In the above for loop it is better to add check_for_interrupts to avoid it looping if the parsed_hba_lines are more. Updated patch is attached with the addition of check_for_interrupts in the for loop. Regards, Hari Babu Fujitsu Australia Catalog_view_to_HBA_settings_patch_V7.patch Description: Binary data -- 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] Providing catalog view to pg_hba.conf file - Patch submission
On Wed, Mar 4, 2015 at 12:17 AM, Jim Nasby jim.na...@bluetreble.com wrote: I can make these changes if you want. Personally I'm just not convinced this is worth it. It makes the catalogs harder for people to read and use and only benefits people who have users named all or databases named all, sameuser, or samerole which I can't really imagine would be anyone. If this were going to be the infrastructure on which lots of tools rested rather than just a read-only view that was mostly going to be read by humans that might be different. Are you envisioning a tool that would look at this view, offer a gui for users to make changes based on that information, and build a new pg_hba.conf to replace it? -- greg -- 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] Providing catalog view to pg_hba.conf file - Patch submission
On Wed, Mar 4, 2015 at 12:34 AM, Haribabu Kommi kommi.harib...@gmail.com wrote: Out of curiosity, regarding the result materialize code addition, Any way the caller of hba_settings function ExecMakeTableFunctionResult also stores the results in tuple_store. Is there any advantage doing it in hba_settings function rather than in the caller? That might protect against concurrent pg_hba reloads, though I suspect there's a CHECK_FOR_INTERRUPTS hanging out in that loop. We could maybe put one in this loop and check if the parser memory context has been reset. But the immediate problem is that having the API that looked up a line by line number and then calling it repeatedly with successive line numbers was O(n^2). Each time it was called it had to walk down the list from the head all over again. 1 + 2 + 3 + 4 + ... n = n(n+1)/2 or O(n^2). This isn't performance critical but it would not have run in a reasonable length of time for large pg_hba files. And having to store the state between calls means the code is at least as simple for the tuplestore, imho even simpler. -- greg -- 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] Providing catalog view to pg_hba.conf file - Patch submission
On 03/03/2015 05:07 PM, Greg Stark wrote: On Wed, Mar 4, 2015 at 12:17 AM, Jim Nasby jim.na...@bluetreble.com wrote: I can make these changes if you want. Personally I'm just not convinced this is worth it. It makes the catalogs harder for people to read and use and only benefits people who have users named all or databases named all, sameuser, or samerole which I can't really imagine would be anyone. If this were going to be the infrastructure on which lots of tools rested rather than just a read-only view that was mostly going to be read by humans that might be different. Are you envisioning a tool that would look at this view, offer a gui for users to make changes based on that information, and build a new pg_hba.conf to replace it? I'm planning to write such a tool. However, I am not concerned about weird name cases like the above. -- Josh Berkus PostgreSQL Experts Inc. http://pgexperts.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] Providing catalog view to pg_hba.conf file - Patch submission
On Wed, Mar 4, 2015 at 12:18 PM, Greg Stark st...@mit.edu wrote: On Wed, Mar 4, 2015 at 12:34 AM, Haribabu Kommi kommi.harib...@gmail.com wrote: Out of curiosity, regarding the result materialize code addition, Any way the caller of hba_settings function ExecMakeTableFunctionResult also stores the results in tuple_store. Is there any advantage doing it in hba_settings function rather than in the caller? That might protect against concurrent pg_hba reloads, though I suspect there's a CHECK_FOR_INTERRUPTS hanging out in that loop. We could maybe put one in this loop and check if the parser memory context has been reset. I feel there is no problem of current pg_hba reloads, because the check_for_interrupts doesn't reload the conf files. It will be done in the postgresMain function once the query finishes. Am I missing something? + foreach(line, parsed_hba_lines) In the above for loop it is better to add check_for_interrupts to avoid it looping if the parsed_hba_lines are more. But the immediate problem is that having the API that looked up a line by line number and then calling it repeatedly with successive line numbers was O(n^2). Each time it was called it had to walk down the list from the head all over again. 1 + 2 + 3 + 4 + ... n = n(n+1)/2 or O(n^2). This isn't performance critical but it would not have run in a reasonable length of time for large pg_hba files. And having to store the state between calls means the code is at least as simple for the tuplestore, imho even simpler. Got it. Thanks. Regards, Hari Babu Fujitsu Australia -- 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] Providing catalog view to pg_hba.conf file - Patch submission
On 3/3/15 12:57 PM, Greg Stark wrote: On Tue, Mar 3, 2015 at 6:05 PM, Jim Nasby jim.na...@bluetreble.com wrote: What about a separate column that's just the text from pg_hba? Or is that what you're opposed to? I'm not sure what you mean by that. There's a rawline field we could put somewhere but it contains the entire line. I mean have a field for each of user/databases that gives you valid pg_hba.conf output. That would allow for cut paste. But perhaps that's just not a use case. FWIW, I'd say that having the individual array elements be correct is more important than what the result of array_out is. That way you could always do array_to_string(..., ', ') and get valid pg_hba output. Well I don't think you can get that without making the view less useful for every other purpose. Like, I would want to be able to do WHERE user @ array[?] or WHERE database = array[?] or to join against a list of users or databases somewhere else. I think we're screwed in that regard anyway, because of the special constructs. You'd need different logic to handle things like +role and sameuser. We might even end up painted in a corner where we can't change it in the future because it'll break everyone's scripts. To do what you suggest would mean the tokens will need to be quoted based on pg_hba.conf syntax requirements. That would mean I would need to check each variable or join value against pg_hba.conf's quoting requirements to compare with it. It seems more practical to have that knowledge if you're actually going to generate a pg_hba.conf than to pass around these quoted strings all the time. What about this: - database_specials enum[] contains all occurrences of all, sameuser, samerole and replication (or maybe it doesn't need to be an array) - in_roles name[] is a list of all cases of +role_name, with the + stripped off (I think the @ case is already handled before the SRF is called??) - role_specials enum[] handles all (enum[] for any future expansion) Alternatively in_roles could just be combined with role_specials as long as we preserve the +. Any tokens that match the special conditions do not show up in databases/users, and those fields become name[]. AFAIK that means the quoting should be identical (at least looking at check_db() and check_role()). I can make these changes if you want. -- Jim Nasby, Data Architect, Blue Treble Consulting Data in Trouble? Get it in Treble! http://BlueTreble.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] Providing catalog view to pg_hba.conf file - Patch submission
On Wed, Mar 4, 2015 at 5:57 AM, Greg Stark st...@mit.edu wrote: On further review I've made a few more changes attached. I think we should change the column names to users and databases to be clear they're lists and also to avoid the user SQL reserved word. I removed the dependency on strlist_to_array which is in objectaddress.c which isn't a very sensible dependency -- it does seem like it would be handy to have a list-based version of construct_array moved to arrayfuncs.c but for now it's not much more work to handle these ourselves. I changed the options to accumulate one big array instead of an array of bunches of options. Previously you could only end up with a singleton array with a comma-delimited string or a two element array with one of those and map=. The changes are fine, this eliminates the unnecessary dependency on objectaddress. I think the error if pg_hba fails to reload needs to be LOG. It would be too unexpected to the user who isn't necessarily the one who issued the SIGHUP to spontaneously get a warning. I also removed the mask from local entries and made some of the NULLS that shouldn't be possible to happen (unknown auth method or connection method) actually throw errors. changes are fine. All the tests are passed. Out of curiosity, regarding the result materialize code addition, Any way the caller of hba_settings function ExecMakeTableFunctionResult also stores the results in tuple_store. Is there any advantage doing it in hba_settings function rather than in the caller? Regards, Hari Babu Fujitsu Australia -- 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] Providing catalog view to pg_hba.conf file - Patch submission
On Mon, Mar 2, 2015 at 7:51 PM, Greg Stark st...@mit.edu wrote: Nobody's allocating anything that big. It's a list of 25,000 pointers to 472-byte structs. That should add up to about 11MB. Instead the memory context is a total of 954606152 bytes which is still under a gigabyte and the database does start up. It drives my laptop to a crawl and Autovacuum crashes when it tries to start but the postmaster is mostly happy. That's about 38k per line or about 80x as much as it should be taking. Hm. Well it seems my laptop was just messed up from having run out of memory. This seems to have affected both ext4fs and the shared memory system in weird ways such that Postgres wasn't reading the new config when I thought (and thought I had confirmed) it was. Now that the filesystem is behaving properly Postgres seems to be behaving more reasonably. It seems to be allocating between 1kB and 1.6kB per hba line including the rawline string, the list of databases and roles which contain structs pointing to all. That still seems highish but not insane. And now that my machine is behaving better here are the timings for the new function using SFRM_Materialize: ::***# select count(*) from pg_hba_settings(); ┌───┐ │ count │ ├───┤ │ 10002 │ └───┘ (1 row) Time: 31.931 ms ... ::***# select count(*) from pg_hba_settings(); ┌───┐ │ count │ ├───┤ │ 80002 │ └───┘ (1 row) Time: 277.376 ms And the total memory used for the 80k lines is about 83MB. hba parser context: 83885056 total in 22 blocks; 1322232 free (13 chunks); 82562824 used Using the n^2 approach it's: ::# select count(*) from pg_hba_settings; ┌───┐ │ count │ ├───┤ │ 10002 │ └───┘ (1 row) Time: 595.397 ms ... ::***# select count(*) from pg_hba_settings; ┌───┐ │ count │ ├───┤ │ 20002 │ └───┘ (1 row) Time: 2441.081 ms -- greg -- 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] Providing catalog view to pg_hba.conf file - Patch submission
So earlier someone commented that using lists list_nth() seemed odd and a tuplestore might be better. In fact using lists this way is O(n^2). I've done some quick tests and it doesn't start being a problem until about 10,000 lines which obviously isn't a terribly common way to use pg_hba_settings. However we have in the past had people doing multi-tenant clusters with hundreds or thousands of databases in a cluster complaining about scalability of certain operations. It would be a shame to introduce a new one. It does seem annoying to use a tuplestore as IIRC the function scan node also materializes the results in recent years. But at least it would scale linearly.
Re: [HACKERS] Providing catalog view to pg_hba.conf file - Patch submission
Greg, * Greg Stark (st...@mit.edu) wrote: On Mon, Mar 2, 2015 at 6:36 AM, Haribabu Kommi kommi.harib...@gmail.com wrote: Loading pg_hba.conf during SIGHUP in the backends will solve the problem of displaying the data which is not yet loaded. This change may produce a warning if it fails to load pg_hba.conf in the backends. This seems like the right strategy to me. It parallels pg_settings and postgresql.conf which means one less surprising quirk for future developers. Agreed. An idle thought, in the long-term it seems like it would be better to have postmaster have some shared memory where it dumps out a config data structure that backends can all see. That might help with the race-conditions we have now when reloading config data where different backends could end up with different interpretations of the same config or even see different configs if the files are being modified concurrently. I had considered this option also but threw it out almost as quickly as it came to mind.. But I think that would have to be done very carefully so the postmaster doesn't sacrifice any reliability. Due to this. I'm not sure there's any trivial way to ensure that. I suppose we could use mmap() and then mprotect() the region when we're not reloading it in the postmaster, and make sure that it's PROT_READ when we fork() as I *think* that would be preserved in the child, until/unless the child changed it. That the child could change it is certainly a concern, even if it requires a few extra steps, but it'd at least be better than nothing, and if there's no code in the child which ever calls mprotect, well, perhaps it'd be good enough. Obviously, this would be a much larger change and we'd have to use a lock-free structure to make sure that the children don't read something inconsistent and avoid having to use a lock that could cause problems for the postmaster. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] Providing catalog view to pg_hba.conf file - Patch submission
On Mon, Mar 2, 2015 at 6:36 AM, Haribabu Kommi kommi.harib...@gmail.com wrote: Loading pg_hba.conf during SIGHUP in the backends will solve the problem of displaying the data which is not yet loaded. This change may produce a warning if it fails to load pg_hba.conf in the backends. This seems like the right strategy to me. It parallels pg_settings and postgresql.conf which means one less surprising quirk for future developers. An idle thought, in the long-term it seems like it would be better to have postmaster have some shared memory where it dumps out a config data structure that backends can all see. That might help with the race-conditions we have now when reloading config data where different backends could end up with different interpretations of the same config or even see different configs if the files are being modified concurrently. But I think that would have to be done very carefully so the postmaster doesn't sacrifice any reliability. -- greg
Re: [HACKERS] Providing catalog view to pg_hba.conf file - Patch submission
On Mon, Mar 2, 2015 at 1:42 PM, Greg Stark st...@mit.edu wrote: So earlier someone commented that using lists list_nth() seemed odd and a tuplestore might be better. In fact using lists this way is O(n^2). I've done some quick tests and it doesn't start being a problem until about 10,000 lines which obviously isn't a terribly common way to use pg_hba_settings. However we have in the past had people doing multi-tenant clusters with hundreds or thousands of databases in a cluster complaining about scalability of certain operations. It would be a shame to introduce a new one. It does seem annoying to use a tuplestore as IIRC the function scan node also materializes the results in recent years. But at least it would scale linearly. So I didn't get the memo about SFRM_Materialize. Here's a rewrite of this using that interface which seems to test ok up to 100k. At that point I start running into memory errors on reading the HBA file so I guess that's an indication that's large enough to stop worrying about it. -- greg *** a/doc/src/sgml/catalogs.sgml --- b/doc/src/sgml/catalogs.sgml *** *** 7393,7398 --- 7393,7403 entryviews/entry /row + row + entrylink linkend=view-pg-hba-settingsstructnamepg_hba_settings/structname/link/entry + entryclient authentication settings/entry + /row + /tbody /tgroup /table *** *** 9696,9699 SELECT * FROM pg_locks pl LEFT JOIN pg_prepared_xacts ppx --- 9701,9786 /sect1 + sect1 id=view-pg-hba-settings + titlestructnamepg_hba_settings/structname/title + + indexterm zone=view-pg-hba-settings +primarypg_hba_settings/primary + /indexterm + + para +The read-only structnamepg_hba_settings/structname view provides +access to the client authentication configuration from pg_hba.conf. +Access to this view is limited to superusers. + /para + + table +titlestructnamepg_hba_settings/ Columns/title + +tgroup cols=3 + thead + row + entryName/entry + entryType/entry + entryDescription/entry + /row + /thead + tbody + row + entrystructfieldtype/structfield/entry + entrytypetext/type/entry + entryType of connection/entry + /row + row + entrystructfielddatabase/structfield/entry + entrytypetext[]/type/entry + entryList of database names/entry + /row + row + entrystructfielduser/structfield/entry + entrytypetext[]/type/entry + entryList of user names/entry + /row + row + entrystructfieldaddress/structfield/entry + entrytypeinet/type/entry + entryClient machine address/entry + /row + row + entrystructfieldmask/structfield/entry + entrytypeinet/type/entry + entryIP Mask/entry + /row + row + entrystructfieldcompare_method/structfield/entry + entrytypetext/type/entry + entryIP address comparison method/entry + /row + row + entrystructfieldhostname/structfield/entry + entrytypetext/type/entry + entryClient host name/entry + /row + row + entrystructfieldmethod/structfield/entry + entrytypetext/type/entry + entryAuthentication method/entry + /row + row + entrystructfieldoptions/structfield/entry + entrytypetext[]/type/entry + entryConfiguration options set for authentication method/entry + /row + row + entrystructfieldline_number/structfield/entry + entrytypeinteger/type/entry + entry +Line number within client authentication configuration file +the current value was set at + /entry + /row + /tbody +/tgroup + /table + /sect1 /chapter *** a/doc/src/sgml/client-auth.sgml --- b/doc/src/sgml/client-auth.sgml *** *** 680,685 local all @admins,+supportmd5 --- 680,690 local db1,db2,@demodbs all md5 /programlisting /example + +para + The contents of this file are reflected in the pg_hba_settings view. + See xref linkend=view-pg-hba-settings for details. +/para /sect1 sect1 id=auth-username-maps *** a/src/backend/catalog/system_views.sql --- b/src/backend/catalog/system_views.sql *** *** 414,419 CREATE RULE pg_settings_n AS --- 414,424 GRANT SELECT, UPDATE ON pg_settings TO PUBLIC; + CREATE VIEW pg_hba_settings AS + SELECT * FROM pg_hba_settings() AS A; + + REVOKE ALL on pg_hba_settings FROM public; + CREATE VIEW pg_timezone_abbrevs AS SELECT * FROM pg_timezone_abbrevs(); *** a/src/backend/libpq/hba.c --- b/src/backend/libpq/hba.c *** *** 25,38 --- 25,44 #include arpa/inet.h #include unistd.h + #include access/htup_details.h #include catalog/pg_collation.h + #include catalog/pg_type.h + #include
Re: [HACKERS] Providing catalog view to pg_hba.conf file - Patch submission
On Mon, Mar 2, 2015 at 4:36 PM, Greg Stark st...@mit.edu wrote: So I didn't get the memo about SFRM_Materialize. Here's a rewrite of this using that interface which seems to test ok up to 100k. At that point I start running into memory errors on reading the HBA file so I guess that's an indication that's large enough to stop worrying about it. Hm. I'm wondering why I'm getting out of memory errors now with just 25k lines in pg_hba.conf. It looks like the HbaLine struct is only 472 bytes so the list should only be occupying about 11MB. In fact it's occupying about a gigabyte: TopMemoryContext: 8192 total in 1 blocks; 5264 free (1 chunks); 2928 used ident parser context: 0 total in 0 blocks; 0 free (0 chunks); 0 used hba parser context: 956300288 total in 126 blocks; 1694136 free (82 chunks); 954606152 used The lines in my pg_hba.conf don't have any databases or roles listed so there shouldn't be any tokens taking up space either. I just copied this line 25,000 times: local all all trust -- greg -- 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] Providing catalog view to pg_hba.conf file - Patch submission
* Greg Stark (st...@mit.edu) wrote: On Mon, Mar 2, 2015 at 4:36 PM, Greg Stark st...@mit.edu wrote: So I didn't get the memo about SFRM_Materialize. Here's a rewrite of this using that interface which seems to test ok up to 100k. At that point I start running into memory errors on reading the HBA file so I guess that's an indication that's large enough to stop worrying about it. Hm. I'm wondering why I'm getting out of memory errors now with just 25k lines in pg_hba.conf. It looks like the HbaLine struct is only 472 bytes so the list should only be occupying about 11MB. In fact it's occupying about a gigabyte: Uh, maybe because it's trying to allocate over 1GB and palloc() doesn't support that? Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] Providing catalog view to pg_hba.conf file - Patch submission
On Mon, Mar 2, 2015 at 7:42 PM, Stephen Frost sfr...@snowman.net wrote: Uh, maybe because it's trying to allocate over 1GB and palloc() doesn't support that? Nobody's allocating anything that big. It's a list of 25,000 pointers to 472-byte structs. That should add up to about 11MB. Instead the memory context is a total of 954606152 bytes which is still under a gigabyte and the database does start up. It drives my laptop to a crawl and Autovacuum crashes when it tries to start but the postmaster is mostly happy. That's about 38k per line or about 80x as much as it should be taking. The tokenizer is run in a separate memory context which gets reset when this is done. Then during parsing it copies things from that memory context over as needed to fill in the struct. In my example pg_hba.c it shouldn't have to copy anything over though so even if there's a bug there I don't see how it would cause this. My best guess is that some functions called allocate some temporary storage. I haven't spotted them yet though I put this aside when I first sent my message. It seems like it would be safest to use a temporary memory context throughout the whole process and only copy things to the hba memory context manually. -- greg -- 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] Providing catalog view to pg_hba.conf file - Patch submission
Greg Stark wrote: Hm. I'm wondering why I'm getting out of memory errors now with just 25k lines in pg_hba.conf. It looks like the HbaLine struct is only 472 bytes so the list should only be occupying about 11MB. In fact it's occupying about a gigabyte: Maybe it's leaking heavily while parsing ... -- Á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] Providing catalog view to pg_hba.conf file - Patch submission
On Sat, Feb 28, 2015 at 11:41 AM, Stephen Frost sfr...@snowman.net wrote: Pavel, * Pavel Stehule (pavel.steh...@gmail.com) wrote: 2015-02-27 22:26 GMT+01:00 Tom Lane t...@sss.pgh.pa.us: Stephen Frost sfr...@snowman.net writes: Right, we also need a view (or function, or both) which provides what the *active* configuration of the running postmaster is. This is exactly what I was proposing (or what I was intending to, at least) with pg_hba_active, so, again, I think we're in agreement here. I think that's going to be a lot harder than you realize, and it will have undesirable security implications, in that whatever you do to expose the postmaster's internal state to backends will also make it visible to other onlookers; not to mention probably adding new failure modes. we can do copy of pg_hba.conf somewhere when postmaster starts or when it is reloaded. Please see my reply to Tom. There's no trivial way to reach into the postmaster from a backend- but we do get a copy of whatever the postmaster had when we forked, and the postmaster only reloads pg_hba.conf on a sighup and that sighup is passed down to the children, so we simply need to also reload the pg_hba.conf in the children when they get a sighup. That's how postgresql.conf is handled, which is what pg_settings is based off of, and I believe is the behavior folks are really looking for. Loading pg_hba.conf during SIGHUP in the backends will solve the problem of displaying the data which is not yet loaded. This change may produce a warning if it fails to load pg_hba.conf in the backends. Here I attached the updated patch. I didn't yet added the pg_hba_check function. I feel it is better to be a separate patch. I can share the same later. Regards, Hari Babu Fujitsu Australia Catalog_view_to_HBA_settings_patch_V6.patch Description: Binary data -- 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] Providing catalog view to pg_hba.conf file - Patch submission
* Josh Berkus (j...@agliodbs.com) wrote: On 02/27/2015 04:41 PM, Stephen Frost wrote: we can do copy of pg_hba.conf somewhere when postmaster starts or when it is reloaded. Please see my reply to Tom. There's no trivial way to reach into the postmaster from a backend- but we do get a copy of whatever the postmaster had when we forked, and the postmaster only reloads pg_hba.conf on a sighup and that sighup is passed down to the children, so we simply need to also reload the pg_hba.conf in the children when they get a sighup. That's how postgresql.conf is handled, which is what pg_settings is based off of, and I believe is the behavior folks are really looking for. I thought the patch in question just implemented reading the file from disk, and nothing else? Speaking for my uses, I would rather have just that for 9.5 than wait for something more sophisticated in 9.6. From my perspective, at least, the differences we're talking about are not enough to raise this to a 9.5-vs-9.6 issue. I can see the use cases for both (which is exactly why I suggested providing both). Having one would be better than nothing, but I foretell lots of subsequent complaints along the lines of everything looks right according to pg_hba_config, but I'm getting this error!! Now, perhaps that's the right approach to go for 9.5 since it'd more-or-less force our hand to deal with it in 9.6 properly, but, personally, I'd be happier if we moved forward with providing both because everyone agrees that it makes sense rather than waiting to see if user complaints force our hand. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] Providing catalog view to pg_hba.conf file - Patch submission
2015-02-28 3:12 GMT+01:00 Stephen Frost sfr...@snowman.net: * Josh Berkus (j...@agliodbs.com) wrote: On 02/27/2015 04:41 PM, Stephen Frost wrote: we can do copy of pg_hba.conf somewhere when postmaster starts or when it is reloaded. Please see my reply to Tom. There's no trivial way to reach into the postmaster from a backend- but we do get a copy of whatever the postmaster had when we forked, and the postmaster only reloads pg_hba.conf on a sighup and that sighup is passed down to the children, so we simply need to also reload the pg_hba.conf in the children when they get a sighup. That's how postgresql.conf is handled, which is what pg_settings is based off of, and I believe is the behavior folks are really looking for. I thought the patch in question just implemented reading the file from disk, and nothing else? Speaking for my uses, I would rather have just that for 9.5 than wait for something more sophisticated in 9.6. From my perspective, at least, the differences we're talking about are not enough to raise this to a 9.5-vs-9.6 issue. I can see the use cases for both (which is exactly why I suggested providing both). Having one would be better than nothing, but I foretell lots of subsequent complaints along the lines of everything looks right according to pg_hba_config, but I'm getting this error!! Now, perhaps that's the right approach to go for 9.5 since it'd more-or-less force our hand to deal with it in 9.6 properly, but, personally, I'd be happier if we moved forward with providing both because everyone agrees that it makes sense rather than waiting to see if user complaints force our hand. +1 Probably we can implement simple load pg_hba.conf and tab transformation early. There is a agreement and not any problem. But if we start to implement some view, then it should be fully functional without potential issues. Regards Pavel Thanks! Stephen
Re: [HACKERS] Providing catalog view to pg_hba.conf file - Patch submission
2015-02-28 1:41 GMT+01:00 Stephen Frost sfr...@snowman.net: Pavel, * Pavel Stehule (pavel.steh...@gmail.com) wrote: 2015-02-27 22:26 GMT+01:00 Tom Lane t...@sss.pgh.pa.us: Stephen Frost sfr...@snowman.net writes: Right, we also need a view (or function, or both) which provides what the *active* configuration of the running postmaster is. This is exactly what I was proposing (or what I was intending to, at least) with pg_hba_active, so, again, I think we're in agreement here. I think that's going to be a lot harder than you realize, and it will have undesirable security implications, in that whatever you do to expose the postmaster's internal state to backends will also make it visible to other onlookers; not to mention probably adding new failure modes. we can do copy of pg_hba.conf somewhere when postmaster starts or when it is reloaded. Please see my reply to Tom. There's no trivial way to reach into the postmaster from a backend- but we do get a copy of whatever the postmaster had when we forked, and the postmaster only reloads pg_hba.conf on a sighup and that sighup is passed down to the children, so we simply need to also reload the pg_hba.conf in the children when they get a sighup. That's how postgresql.conf is handled, which is what pg_settings is based off of, and I believe is the behavior folks are really looking for. It has sense for me too. Pavel Thanks, Stephen
Re: [HACKERS] Providing catalog view to pg_hba.conf file - Patch submission
2015-02-28 2:40 GMT+01:00 Tom Lane t...@sss.pgh.pa.us: Stephen Frost sfr...@snowman.net writes: I understand that there may be objections to that on the basis that it's work that's (other than for this case) basically useless, Got it in one. I'm also not terribly happy about leaving security-relevant data sitting around in backend memory 100% of the time. We have had bugs that exposed backend memory contents for reading without also granting the ability to execute arbitrary code, so I think doing this does represent a quantifiable decrease in the security of pg_hba.conf. The Stephen's proposal changes nothing in security. These data are in memory now. The only one difference is, so these data will be fresh. Regards Pavel regards, tom lane
Re: [HACKERS] Providing catalog view to pg_hba.conf file - Patch submission
Stephen Frost sfr...@snowman.net writes: I understand that there may be objections to that on the basis that it's work that's (other than for this case) basically useless, Got it in one. I'm also not terribly happy about leaving security-relevant data sitting around in backend memory 100% of the time. We have had bugs that exposed backend memory contents for reading without also granting the ability to execute arbitrary code, so I think doing this does represent a quantifiable decrease in the security of pg_hba.conf. 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] Providing catalog view to pg_hba.conf file - Patch submission
* Tom Lane (t...@sss.pgh.pa.us) wrote: Stephen Frost sfr...@snowman.net writes: I understand that there may be objections to that on the basis that it's work that's (other than for this case) basically useless, Got it in one. Meh. It's hardly all that difficult and it's not useless if the user wants to look at it. I'm also not terribly happy about leaving security-relevant data sitting around in backend memory 100% of the time. We have had bugs that exposed backend memory contents for reading without also granting the ability to execute arbitrary code, so I think doing this does represent a quantifiable decrease in the security of pg_hba.conf. How is that any different from today? The only time it's not *already* in backend memory is when the user has happened to go through and make a change and used reload (instead of restart) and then it's not so much that the security sensetive information isn't there, it's just out of date. I'm not entirely against the idea of changing how things are today, but this argument simply doesn't apply to the current state of things. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] Providing catalog view to pg_hba.conf file - Patch submission
On 02/27/2015 04:41 PM, Stephen Frost wrote: we can do copy of pg_hba.conf somewhere when postmaster starts or when it is reloaded. Please see my reply to Tom. There's no trivial way to reach into the postmaster from a backend- but we do get a copy of whatever the postmaster had when we forked, and the postmaster only reloads pg_hba.conf on a sighup and that sighup is passed down to the children, so we simply need to also reload the pg_hba.conf in the children when they get a sighup. That's how postgresql.conf is handled, which is what pg_settings is based off of, and I believe is the behavior folks are really looking for. I thought the patch in question just implemented reading the file from disk, and nothing else? Speaking for my uses, I would rather have just that for 9.5 than wait for something more sophisticated in 9.6. -- Josh Berkus PostgreSQL Experts Inc. http://pgexperts.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] Providing catalog view to pg_hba.conf file - Patch submission
All, * Tomas Vondra (tomas.von...@2ndquadrant.com) wrote: On 28.1.2015 23:01, Jim Nasby wrote: On 1/28/15 12:46 AM, Haribabu Kommi wrote: Also, what happens if someone reloads the config in the middle of running the SRF? hba entries are reloaded only in postmaster process, not in every backend. So there shouldn't be any problem with config file reload. Am i missing something? Ahh, good point. That does raise another issue though... there might well be some confusion about that. I think people are used to the varieties of how settings come into effect that perhaps this isn't an issue with pg_settings, but it's probably worth at least mentioning in the docs for a pg_hba view. I share this fear, and it's the main problem I have with this patch. Uh, yeah, agreed. Currently, the patch always does load_hba() every time pg_hba_settings is accessed, which loads the current contents of the config file into the backend process, but the postmaster still uses the original one. This makes it impossible to look at currently effective pg_hba.conf contents. Damn confusing, I guess. Indeed. At a *minimum*, we'd need to have some way to say what you're seeing isn't what's actually being used. Also, I dare to say that having a system view that doesn't actually show the system state (but contents of a config file that may not be loaded yet), is wrong. Right, we need to show what's currently active in PG-land, not just spit back whatever the on-disk contents currently are. Granted, we don't modify pg_hba.conf all that often, and it's usually followed by a SIGHUP to reload the contents, so both the backend and postmaster should see the same stuff. But the cases when I'd use this pg_hba_settings view usually turned out to be 'forgot to SIGHUP', so this would not help, actually. Agreed. What I can imagine is keeping the original list (parsed by postmaster, containing the effective pg_hba.conf contents), and keeping another list of 'current contents' within backends. And having a 'reload' flag for pg_hba_settings, determining which list to use. pg_hba_settings(reload=false) - old list (from postmaster) pg_hba_settings(reload=true) - new list (from backend) The pg_hba_settings view should use 'reload=false' (i.e. show effective contents of the hba). I don't think we actually care what the current contents are from the backend's point of view- after all, when does an individual backend ever use the contents of pg_hba.conf after it's up and running? What would make sense, to me at least, would be: pg_hba_configured() -- spits back whatever the config file has pg_hba_active() -- shows what the postmaster is using currently Or something along those lines. Note that I'd want pg_hba_configured() to throw an error if the config file can't be parsed (which would be quite handy to make sure that you didn't goof up the config..). This, combined with pg_reload_conf() would provide a good way to review changes before putting them into place. The other feature that'd be cool to have is a debugging function on top of the view, i.e. a function pg_hba_check(host, ip, db, user, pwd) showing which hba rule matched. But that's certainly nontrivial. I'm not sure that I see why, offhand, it'd be much more than trivial.. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] Providing catalog view to pg_hba.conf file - Patch submission
2015-02-27 17:59 GMT+01:00 Stephen Frost sfr...@snowman.net: All, * Tomas Vondra (tomas.von...@2ndquadrant.com) wrote: On 28.1.2015 23:01, Jim Nasby wrote: On 1/28/15 12:46 AM, Haribabu Kommi wrote: Also, what happens if someone reloads the config in the middle of running the SRF? hba entries are reloaded only in postmaster process, not in every backend. So there shouldn't be any problem with config file reload. Am i missing something? Ahh, good point. That does raise another issue though... there might well be some confusion about that. I think people are used to the varieties of how settings come into effect that perhaps this isn't an issue with pg_settings, but it's probably worth at least mentioning in the docs for a pg_hba view. I share this fear, and it's the main problem I have with this patch. Uh, yeah, agreed. yes, good notice. I was blind. Currently, the patch always does load_hba() every time pg_hba_settings is accessed, which loads the current contents of the config file into the backend process, but the postmaster still uses the original one. This makes it impossible to look at currently effective pg_hba.conf contents. Damn confusing, I guess. Indeed. At a *minimum*, we'd need to have some way to say what you're seeing isn't what's actually being used. Also, I dare to say that having a system view that doesn't actually show the system state (but contents of a config file that may not be loaded yet), is wrong. Right, we need to show what's currently active in PG-land, not just spit back whatever the on-disk contents currently are. Granted, we don't modify pg_hba.conf all that often, and it's usually followed by a SIGHUP to reload the contents, so both the backend and postmaster should see the same stuff. But the cases when I'd use this pg_hba_settings view usually turned out to be 'forgot to SIGHUP', so this would not help, actually. Agreed. What I can imagine is keeping the original list (parsed by postmaster, containing the effective pg_hba.conf contents), and keeping another list of 'current contents' within backends. And having a 'reload' flag for pg_hba_settings, determining which list to use. pg_hba_settings(reload=false) - old list (from postmaster) pg_hba_settings(reload=true) - new list (from backend) The pg_hba_settings view should use 'reload=false' (i.e. show effective contents of the hba). I don't think we actually care what the current contents are from the backend's point of view- after all, when does an individual backend ever use the contents of pg_hba.conf after it's up and running? What would make sense, to me at least, would be: pg_hba_configured() -- spits back whatever the config file has pg_hba_active() -- shows what the postmaster is using currently I disagree and I dislike this direction. It is looks like over engineering. * load every time is wrong, because you will see possibly not active data. * ignore reload is a attack to mental health of our users. It should to work like pg_settings. I need to see what is wrong in this moment in pg_hba.conf, not what was or what will be wrong. We can load any config files via admin contrib module - so there is not necessary repeat same functionality Regards Pavel Or something along those lines. Note that I'd want pg_hba_configured() to throw an error if the config file can't be parsed (which would be quite handy to make sure that you didn't goof up the config..). This, combined with pg_reload_conf() would provide a good way to review changes before putting them into place. The other feature that'd be cool to have is a debugging function on top of the view, i.e. a function pg_hba_check(host, ip, db, user, pwd) showing which hba rule matched. But that's certainly nontrivial. I'm not sure that I see why, offhand, it'd be much more than trivial.. Thanks! Stephen
Re: [HACKERS] Providing catalog view to pg_hba.conf file - Patch submission
On 27.2.2015 17:59, Stephen Frost wrote: All, * Tomas Vondra (tomas.von...@2ndquadrant.com) wrote: The other feature that'd be cool to have is a debugging function on top of the view, i.e. a function pg_hba_check(host, ip, db, user, pwd) showing which hba rule matched. But that's certainly nontrivial. I'm not sure that I see why, offhand, it'd be much more than trivial ... From time to time I have to debug why are connection attempts failing, and with moderately-sized pg_hba.conf files (e.g. on database servers shared by multiple applications) that may be tricky. Identifying the rule that matched (and rejected) the connection would be helpful. But yes, that's non-trivial and out of scope of this patch. -- Tomas Vondrahttp://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] Providing catalog view to pg_hba.conf file - Patch submission
Hi, On 28.1.2015 23:01, Jim Nasby wrote: On 1/28/15 12:46 AM, Haribabu Kommi wrote: Also, what happens if someone reloads the config in the middle of running the SRF? hba entries are reloaded only in postmaster process, not in every backend. So there shouldn't be any problem with config file reload. Am i missing something? Ahh, good point. That does raise another issue though... there might well be some confusion about that. I think people are used to the varieties of how settings come into effect that perhaps this isn't an issue with pg_settings, but it's probably worth at least mentioning in the docs for a pg_hba view. I share this fear, and it's the main problem I have with this patch. Currently, the patch always does load_hba() every time pg_hba_settings is accessed, which loads the current contents of the config file into the backend process, but the postmaster still uses the original one. This makes it impossible to look at currently effective pg_hba.conf contents. Damn confusing, I guess. Also, I dare to say that having a system view that doesn't actually show the system state (but contents of a config file that may not be loaded yet), is wrong. Granted, we don't modify pg_hba.conf all that often, and it's usually followed by a SIGHUP to reload the contents, so both the backend and postmaster should see the same stuff. But the cases when I'd use this pg_hba_settings view usually turned out to be 'forgot to SIGHUP', so this would not help, actually. What I can imagine is keeping the original list (parsed by postmaster, containing the effective pg_hba.conf contents), and keeping another list of 'current contents' within backends. And having a 'reload' flag for pg_hba_settings, determining which list to use. pg_hba_settings(reload=false) - old list (from postmaster) pg_hba_settings(reload=true) - new list (from backend) The pg_hba_settings view should use 'reload=false' (i.e. show effective contents of the hba). The other feature that'd be cool to have is a debugging function on top of the view, i.e. a function pg_hba_check(host, ip, db, user, pwd) showing which hba rule matched. But that's certainly nontrivial. -- Tomas Vondrahttp://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] Providing catalog view to pg_hba.conf file - Patch submission
Hi
It looks well, I have only one objection.
I am not sure so function hba_settings should be in file guc.c - it has
zero relation to GUC.
Maybe hba.c file is better probably.
Other opinions?
2015-02-27 7:30 GMT+01:00 Haribabu Kommi kommi.harib...@gmail.com:
On Sat, Feb 7, 2015 at 8:26 PM, Pavel Stehule pavel.steh...@gmail.com
wrote:
Hi
I am sending a review of this patch.
Thanks for the review. sorry for the delay.
4. Regress tests
test rules... FAILED -- missing info about new view
Thanks. Corrected.
My objections:
1. data type for database field should be array of name or array of
text.
When name contains a comma, then this comma is not escaped
currently: {omega,my stupid extremly, name2,my stupid name}
expected: {my stupid name,omega,my stupid extremly, name2}
Same issue I see in options field
Changed including the user column also.
2. Reload is not enough for content refresh - logout is necessary
I understand, why it is, but it is not very friendly, and can be very
stressful. It should to work with last reloaded data.
At present the view data is populated from the variable parsed_hba_lines
which contains the already parsed lines of pg_hba.conf file.
During the reload, the hba entries are reloaded only in the postmaster
process
and not in every backend, because of this reason the reloaded data is
not visible until
the session is disconnected and connected.
Instead of changing the SIGHUP behavior in the backend to load the hba
entries
also, whenever the call is made to the view, the pg_hba.conf file is
parsed to show
the latest reloaded data in the view. This way it doesn't impact the
existing behavior
but it may impact the performance because of file load into memory every
time.
your solution is ok, simply and clean. Performance for this view should not
be critical and every time will be faster than login as root and searching
pg_hba.conf
Regards
Pavel
Updated patch is attached. comments?
Regards,
Hari Babu
Fujitsu Australia
Re: [HACKERS] Providing catalog view to pg_hba.conf file - Patch submission
On 02/27/2015 10:35 AM, Stephen Frost wrote: From time to time I have to debug why are connection attempts failing, and with moderately-sized pg_hba.conf files (e.g. on database servers shared by multiple applications) that may be tricky. Identifying the rule that matched (and rejected) the connection would be helpful. To clarify, I was trying to say that writing that function didn't seem very difficult to me. I definitely think that *having* that function would be very useful. But yes, that's non-trivial and out of scope of this patch. For my 2c, I view this as somewhat up to the author. I wouldn't complain if it was included in a new version of this patch as I don't think it'd add all that much complexity and it'd be very nice, but I certainly think this patch could go in without that too. Also, a testing function could be written in userspace after this feature is added. I can imagine how to write one as a UDF. -- Josh Berkus PostgreSQL Experts Inc. http://pgexperts.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] Providing catalog view to pg_hba.conf file - Patch submission
On Fri, Feb 27, 2015 at 12:48 PM, Tomas Vondra tomas.von...@2ndquadrant.com wrote: On 27.2.2015 17:59, Stephen Frost wrote: All, * Tomas Vondra (tomas.von...@2ndquadrant.com) wrote: The other feature that'd be cool to have is a debugging function on top of the view, i.e. a function pg_hba_check(host, ip, db, user, pwd) showing which hba rule matched. But that's certainly nontrivial. I'm not sure that I see why, offhand, it'd be much more than trivial ... From time to time I have to debug why are connection attempts failing, and with moderately-sized pg_hba.conf files (e.g. on database servers shared by multiple applications) that may be tricky. Identifying the rule that matched (and rejected) the connection would be helpful. If you did actually get a rejected connection, you get that in the log (as of 9.3, iirc). Such a function would make it possible to test it without having failed first though :) -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Re: [HACKERS] Providing catalog view to pg_hba.conf file - Patch submission
* Pavel Stehule (pavel.steh...@gmail.com) wrote: 2015-02-27 17:59 GMT+01:00 Stephen Frost sfr...@snowman.net: I don't think we actually care what the current contents are from the backend's point of view- after all, when does an individual backend ever use the contents of pg_hba.conf after it's up and running? What would make sense, to me at least, would be: pg_hba_configured() -- spits back whatever the config file has pg_hba_active() -- shows what the postmaster is using currently I disagree and I dislike this direction. It is looks like over engineering. * load every time is wrong, because you will see possibly not active data. That's the point of the two functions- one to give you what a reload *now* would, and one to see what's currently active. * ignore reload is a attack to mental health of our users. Huh? It should to work like pg_settings. I need to see what is wrong in this moment in pg_hba.conf, not what was or what will be wrong. That's what pg_hba_active() would be from above, yes. We can load any config files via admin contrib module - so there is not necessary repeat same functionality That's hardly the same- I can't (easily, anyway) join the results of pg_read() to pg_hba_active() and see what's different or the same. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] Providing catalog view to pg_hba.conf file - Patch submission
* Tomas Vondra (tomas.von...@2ndquadrant.com) wrote: On 27.2.2015 17:59, Stephen Frost wrote: All, * Tomas Vondra (tomas.von...@2ndquadrant.com) wrote: The other feature that'd be cool to have is a debugging function on top of the view, i.e. a function pg_hba_check(host, ip, db, user, pwd) showing which hba rule matched. But that's certainly nontrivial. I'm not sure that I see why, offhand, it'd be much more than trivial ... From time to time I have to debug why are connection attempts failing, and with moderately-sized pg_hba.conf files (e.g. on database servers shared by multiple applications) that may be tricky. Identifying the rule that matched (and rejected) the connection would be helpful. To clarify, I was trying to say that writing that function didn't seem very difficult to me. I definitely think that *having* that function would be very useful. But yes, that's non-trivial and out of scope of this patch. For my 2c, I view this as somewhat up to the author. I wouldn't complain if it was included in a new version of this patch as I don't think it'd add all that much complexity and it'd be very nice, but I certainly think this patch could go in without that too. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] Providing catalog view to pg_hba.conf file - Patch submission
* Pavel Stehule (pavel.steh...@gmail.com) wrote: this topic should be divided, please. One part - functions for loading pg_hba and translating to some table. Can be two, can be one with one parameter. It will be used probably by advanced user, and I am able to accept it like you or Tomas proposed. I'm not sure why you'd need a parameter for the function. The function could back a view too. In general, I think you're agreeing with me that it'd be a useful capability to have. Second part is the content of view pg_hba_conf. It should be only one and should to view a active content. I mean a content that is actively used - when other session is started. I am strongly against the behave, when I have to close session to refresh a content of this view (after reload) - and I am against to see there not active content. Right, we also need a view (or function, or both) which provides what the *active* configuration of the running postmaster is. This is exactly what I was proposing (or what I was intending to, at least) with pg_hba_active, so, again, I think we're in agreement here. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] Providing catalog view to pg_hba.conf file - Patch submission
2015-02-27 19:32 GMT+01:00 Stephen Frost sfr...@snowman.net: * Pavel Stehule (pavel.steh...@gmail.com) wrote: 2015-02-27 17:59 GMT+01:00 Stephen Frost sfr...@snowman.net: I don't think we actually care what the current contents are from the backend's point of view- after all, when does an individual backend ever use the contents of pg_hba.conf after it's up and running? What would make sense, to me at least, would be: pg_hba_configured() -- spits back whatever the config file has pg_hba_active() -- shows what the postmaster is using currently I disagree and I dislike this direction. It is looks like over engineering. * load every time is wrong, because you will see possibly not active data. That's the point of the two functions- one to give you what a reload *now* would, and one to see what's currently active. * ignore reload is a attack to mental health of our users. Huh? this topic should be divided, please. One part - functions for loading pg_hba and translating to some table. Can be two, can be one with one parameter. It will be used probably by advanced user, and I am able to accept it like you or Tomas proposed. Second part is the content of view pg_hba_conf. It should be only one and should to view a active content. I mean a content that is actively used - when other session is started. I am strongly against the behave, when I have to close session to refresh a content of this view (after reload) - and I am against to see there not active content. Regards Pavel It should to work like pg_settings. I need to see what is wrong in this moment in pg_hba.conf, not what was or what will be wrong. That's what pg_hba_active() would be from above, yes. We can load any config files via admin contrib module - so there is not necessary repeat same functionality That's hardly the same- I can't (easily, anyway) join the results of pg_read() to pg_hba_active() and see what's different or the same. Thanks, Stephen
Re: [HACKERS] Providing catalog view to pg_hba.conf file - Patch submission
Stephen Frost sfr...@snowman.net writes: Right, we also need a view (or function, or both) which provides what the *active* configuration of the running postmaster is. This is exactly what I was proposing (or what I was intending to, at least) with pg_hba_active, so, again, I think we're in agreement here. I think that's going to be a lot harder than you realize, and it will have undesirable security implications, in that whatever you do to expose the postmaster's internal state to backends will also make it visible to other onlookers; not to mention probably adding new failure modes. There are also nontrivial semantic differences in this area between Windows and other platforms (ie in an EXEC_BACKEND build the current file contents *are* the active version). If you insist on two views you will need to explain why/how they act differently on different platforms. I think the proposed mechanism (ie read and return the current contents of the file) is just fine, and we should stop there rather than engineering this to death. We've survived twenty years with *no* feature of this sort, how is it suddenly essential that we expose postmaster internal state? 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] Providing catalog view to pg_hba.conf file - Patch submission
2015-02-27 22:26 GMT+01:00 Tom Lane t...@sss.pgh.pa.us: Stephen Frost sfr...@snowman.net writes: Right, we also need a view (or function, or both) which provides what the *active* configuration of the running postmaster is. This is exactly what I was proposing (or what I was intending to, at least) with pg_hba_active, so, again, I think we're in agreement here. I think that's going to be a lot harder than you realize, and it will have undesirable security implications, in that whatever you do to expose the postmaster's internal state to backends will also make it visible to other onlookers; not to mention probably adding new failure modes. we can do copy of pg_hba.conf somewhere when postmaster starts or when it is reloaded. Later, we can read this copy from child nodes. Is it a possibility? Regards Pavel There are also nontrivial semantic differences in this area between Windows and other platforms (ie in an EXEC_BACKEND build the current file contents *are* the active version). If you insist on two views you will need to explain why/how they act differently on different platforms. I think the proposed mechanism (ie read and return the current contents of the file) is just fine, and we should stop there rather than engineering this to death. We've survived twenty years with *no* feature of this sort, how is it suddenly essential that we expose postmaster internal state? regards, tom lane
Re: [HACKERS] Providing catalog view to pg_hba.conf file - Patch submission
* Tom Lane (t...@sss.pgh.pa.us) wrote: Stephen Frost sfr...@snowman.net writes: Right, we also need a view (or function, or both) which provides what the *active* configuration of the running postmaster is. This is exactly what I was proposing (or what I was intending to, at least) with pg_hba_active, so, again, I think we're in agreement here. I think that's going to be a lot harder than you realize, and it will have undesirable security implications, in that whatever you do to expose the postmaster's internal state to backends will also make it visible to other onlookers; not to mention probably adding new failure modes. I had been considering what it'd take (and certainly appreciated that it's not trivial to look at the postmaster post-fork) but had also been thinking a simpler approach might be possible (one which doesn't involve *directly* looking at the postmaster config)- we could probably reasonably consider whatever the backend has currently is the same as the active configuration, provided we reload the pg_hba.conf into the backends when a sighup is sent, just as we do with postgresql.conf. I understand that there may be objections to that on the basis that it's work that's (other than for this case) basically useless, but then again, it's not like we anticipate reloads happening with a high frequency or that we expect loading these files to take all that long. The original patch only went off of what was in place when the backend was started from the postmaster and the later patch changed it to just always show what was currently in the pg_hba.conf file, but what everyone on this thread (except those worried more about the implementation and less about the capability) expects and wants is pg_settings, but for pg_hba. The way we get that is to do it exactly the same as how we handle pg_settings. I think the proposed mechanism (ie read and return the current contents of the file) is just fine, and we should stop there rather than engineering this to death. We've survived twenty years with *no* feature of this sort, how is it suddenly essential that we expose postmaster internal state? Perhaps I'm missing something, but I really don't see it to be a huge deal to just reload pg_hba.conf in the backends when a sighup is done, which would provide pg_settings-like results (ignoring that you can set GUCs directly in the backend, of course), with two ways through the function which loads the file- one which actually updates the global setting in the backend during a sighup, and one which provides the results in variables passed in by the caller (or similar) and allows returning the contents of the current pg_hba.conf as parsed in an SRF. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] Providing catalog view to pg_hba.conf file - Patch submission
Pavel, * Pavel Stehule (pavel.steh...@gmail.com) wrote: 2015-02-27 22:26 GMT+01:00 Tom Lane t...@sss.pgh.pa.us: Stephen Frost sfr...@snowman.net writes: Right, we also need a view (or function, or both) which provides what the *active* configuration of the running postmaster is. This is exactly what I was proposing (or what I was intending to, at least) with pg_hba_active, so, again, I think we're in agreement here. I think that's going to be a lot harder than you realize, and it will have undesirable security implications, in that whatever you do to expose the postmaster's internal state to backends will also make it visible to other onlookers; not to mention probably adding new failure modes. we can do copy of pg_hba.conf somewhere when postmaster starts or when it is reloaded. Please see my reply to Tom. There's no trivial way to reach into the postmaster from a backend- but we do get a copy of whatever the postmaster had when we forked, and the postmaster only reloads pg_hba.conf on a sighup and that sighup is passed down to the children, so we simply need to also reload the pg_hba.conf in the children when they get a sighup. That's how postgresql.conf is handled, which is what pg_settings is based off of, and I believe is the behavior folks are really looking for. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] Providing catalog view to pg_hba.conf file - Patch submission
On Sat, Feb 7, 2015 at 8:26 PM, Pavel Stehule pavel.steh...@gmail.com wrote:
Hi
I am sending a review of this patch.
Thanks for the review. sorry for the delay.
4. Regress tests
test rules... FAILED -- missing info about new view
Thanks. Corrected.
My objections:
1. data type for database field should be array of name or array of text.
When name contains a comma, then this comma is not escaped
currently: {omega,my stupid extremly, name2,my stupid name}
expected: {my stupid name,omega,my stupid extremly, name2}
Same issue I see in options field
Changed including the user column also.
2. Reload is not enough for content refresh - logout is necessary
I understand, why it is, but it is not very friendly, and can be very
stressful. It should to work with last reloaded data.
At present the view data is populated from the variable parsed_hba_lines
which contains the already parsed lines of pg_hba.conf file.
During the reload, the hba entries are reloaded only in the postmaster process
and not in every backend, because of this reason the reloaded data is
not visible until
the session is disconnected and connected.
Instead of changing the SIGHUP behavior in the backend to load the hba entries
also, whenever the call is made to the view, the pg_hba.conf file is
parsed to show
the latest reloaded data in the view. This way it doesn't impact the
existing behavior
but it may impact the performance because of file load into memory every time.
Updated patch is attached. comments?
Regards,
Hari Babu
Fujitsu Australia
Catalog_view_to_HBA_settings_patch_V5.patch
Description: Binary data
--
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] Providing catalog view to pg_hba.conf file - Patch submission
Hi
I am sending a review of this patch.
1. We would this patch?
yes. It is a good idea - checking internal view is more comfortable and
faster than checking some (possibly longer) pg_hba.conf. There was no
objections.
2. Scope - does this patch, what we need?
yes. There was a discussion about altering pg_hba.conf from SQL, but we
don't need it now.
3. Patching, compilation
no warnings, no errors
4. Regress tests
test rules... FAILED -- missing info about new view
My objections:
1. data type for database field should be array of name or array of text.
When name contains a comma, then this comma is not escaped
currently: {omega,my stupid extremly, name2,my stupid name}
expected: {my stupid name,omega,my stupid extremly, name2}
Same issue I see in options field
2. Reload is not enough for content refresh - logout is necessary
I understand, why it is, but it is not very friendly, and can be very
stressful. It should to work with last reloaded data.
I have not too strong opinion on @1, but @2 should be fixed.
Regards
Pavel
2015-01-28 7:46 GMT+01:00 Haribabu Kommi kommi.harib...@gmail.com:
On Wed, Jan 28, 2015 at 9:47 AM, Jim Nasby jim.na...@bluetreble.com
wrote:
On 1/27/15 1:04 AM, Haribabu Kommi wrote:
Here I attached the latest version of the patch.
I will add this patch to the next commitfest.
Apologies if this was covered, but why isn't the IP address an inet
instead
of text?
Corrected the address type as inet instead of text. updated patch is
attached.
Also, what happens if someone reloads the config in the middle of running
the SRF?
hba entries are reloaded only in postmaster process, not in every backend.
So there shouldn't be any problem with config file reload. Am i
missing something?
Regards,
Hari Babu
Fujitsu Australia
--
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] Providing catalog view to pg_hba.conf file - Patch submission
On 01/30/2015 10:01 PM, Amit Kapila wrote: On Fri, Jan 30, 2015 at 10:58 PM, Robert Haas robertmh...@gmail.com mailto:robertmh...@gmail.com wrote: Yes. The contents of postgresql.conf are only mildly order-dependent. If you put the same setting in more than once, it matters which one is last. Apart from that, though, it doesn't really matter: wal_keep_segments=10 means the same thing if it occurs before max_connections=401 that it means after that. The same is not true of pg_hba.conf, where the order matters a lot. Do you mean to say that as authentication system uses just the first record that matches to perform authentication, it could lead to problems if an order is not maintained? Won't the same set of problems can occur if user tries to that manually and do it without proper care of such rules. Now the problem with command is that user can't see the order in which entries are being made, but it seems to me that we can provide a view or some way to user so that the order of entries is visible and the same is allowed to be manipulated via command. We *can*, yes. But the technical issues around that have not been addressed. Certainly just making the new system view respond to UPDATE/INSERT/DELETE would not be sufficient. And then once we address the technical issues, we'll need to address the security implications. I think this is worth doing; there's some tremendous utility potential in having a PostgresQL which can be 100% managed via port 5432, especially for the emerging world of container-based hosting (Docker et. al.). However, it's also going to be difficult. -- Josh Berkus PostgreSQL Experts Inc. http://pgexperts.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] Providing catalog view to pg_hba.conf file - Patch submission
On Thu, Jan 29, 2015 at 10:13 PM, Amit Kapila amit.kapil...@gmail.com wrote: I think the big problem you are mentioning can be resolved in a similar way as we have done for ALTER SYSTEM which is to have a separate file (.auto.conf) for settings done via ALTER SYSTEM command, do you see any major problem with that approach. Yes. The contents of postgresql.conf are only mildly order-dependent. If you put the same setting in more than once, it matters which one is last. Apart from that, though, it doesn't really matter: wal_keep_segments=10 means the same thing if it occurs before max_connections=401 that it means after that. The same is not true of pg_hba.conf, where the order matters a lot. This makes merging two files together much less feasible, and much more confusing. You are also a lot more likely to lock yourself out of the database by adjusting pg_hba.conf. You can do that by modifying postgresql.conf, say by putting an invalid combination of parameters in there or getting it to request more semaphores or more RAM than the system can accommodate or changing listen_addresses to 127.0.0.1, but there are lots of things that you can do that carry no such risk. This is much less true with pg_hba.conf. Even if I had a feature that would let me modify pg_hba.conf remotely, I'm not sure I'd be brave enough to use it. Overall, this seems to me like a can of worms better left unopened. -- 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] Providing catalog view to pg_hba.conf file - Patch submission
On 1/29/15 9:13 PM, Amit Kapila wrote: Aside from Tom's concern about sets not being a good way to handle this (which I agree with), the idea of editing pg_hba.conf via SQL raises all the problems that were brought up when ALTER SYSTEM was being developed. One of the big problems is a question of how you can safely modify a text file that's full of comments and what-not. You'd need to address those issues if you hope to modify pg_hba.conf via SQL. I think the big problem you are mentioning can be resolved in a similar way as we have done for ALTER SYSTEM which is to have a separate file (.auto.conf) for settings done via ALTER SYSTEM command, do you see any major problem with that approach. Yes I do. pg_hba.conf is completely depending on ordering, so there's no way you can simply toss another file into the mix. It's bad enough that we do that with postgresql.auto.conf, but at least that's a simple over-ride. With HBA a single ALTER SYSTEM could activate (or deactivate) a huge swath of pg_hba.conf. That makes for a system that's fragile, and since it's security related, dangerous. I could maybe see an interface where we allowed users to perform line-level operations on pg_hba.conf via SQL: UPDATE line X, INSERT BEFORE/AFTER line X, DELETE line X. At least that would preserve the critical nature of rules ordering. -- Jim Nasby, Data Architect, Blue Treble Consulting Data in Trouble? Get it in Treble! http://BlueTreble.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] Providing catalog view to pg_hba.conf file - Patch submission
On Fri, Jan 30, 2015 at 10:58 PM, Robert Haas robertmh...@gmail.com wrote: On Thu, Jan 29, 2015 at 10:13 PM, Amit Kapila amit.kapil...@gmail.com wrote: I think the big problem you are mentioning can be resolved in a similar way as we have done for ALTER SYSTEM which is to have a separate file (.auto.conf) for settings done via ALTER SYSTEM command, do you see any major problem with that approach. Yes. The contents of postgresql.conf are only mildly order-dependent. If you put the same setting in more than once, it matters which one is last. Apart from that, though, it doesn't really matter: wal_keep_segments=10 means the same thing if it occurs before max_connections=401 that it means after that. The same is not true of pg_hba.conf, where the order matters a lot. Do you mean to say that as authentication system uses just the first record that matches to perform authentication, it could lead to problems if an order is not maintained? Won't the same set of problems can occur if user tries to that manually and do it without proper care of such rules. Now the problem with command is that user can't see the order in which entries are being made, but it seems to me that we can provide a view or some way to user so that the order of entries is visible and the same is allowed to be manipulated via command. This makes merging two files together much less feasible, and much more confusing. You are also a lot more likely to lock yourself out of the database by adjusting pg_hba.conf. I think that could be even possible via Alter User .. password, if the password is changed then also kind of user can be locked out of database, basically it is also part of authentication mechanism. Even if I had a feature that would let me modify pg_hba.conf remotely, I'm not sure I'd be brave enough to use it. Okay, but how about via some server side utility or some other way with which users don't need to manually edit the file? It seems to be that some of the other databases like Oracle also provide a way for users to operate of similar files via commands, although in a different way [1]. Overall, this seems to me like a can of worms better left unopened. Sure, I can understand the dangers you want to highlight, however OTOH it seems to me that providing some way to users with which they can change things without manually editing file is a good move. [1] http://docs.oracle.com/cd/B28359_01/network.111/b28317/lsnrctl.htm#i553656 With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
Re: [HACKERS] Providing catalog view to pg_hba.conf file - Patch submission
On Wed, Jan 28, 2015 at 5:27 PM, Tom Lane t...@sss.pgh.pa.us wrote: =?UTF-8?Q?Fabr=C3=ADzio_de_Royes_Mello?= fabriziome...@gmail.com writes: But I'm thinking about this patch and would not be interesting to have a FDW to manipulate the hba file? Imagine if we are able to manipulate the HBA file using INSERT/UPDATE/DELETE. Since the HBA file is fundamentally order-dependent, while SQL tables are fundamentally not, that doesn't seem like a great API match. You could probably brute-force something that would work, but it would very much be a case of using a hammer to solve a screwdriver problem. Maybe, but my intention is provide an easy way to edit HBA entries. With an extension or API to edit HBA entries many developers of PostgreSQL tools (ie. pgadmin, phppgadmin, etc) will be benefited. Perhaps a fdw can't be the best choice, maybe a complete new SQL syntax to manipulate HBA entries like we did with ALTER SYSTEM. It's just some thoughts about it. Regards, -- Fabrízio de Royes Mello Consultoria/Coaching PostgreSQL Timbira: http://www.timbira.com.br Blog: http://fabriziomello.github.io Linkedin: http://br.linkedin.com/in/fabriziomello Twitter: http://twitter.com/fabriziomello Github: http://github.com/fabriziomello
Re: [HACKERS] Providing catalog view to pg_hba.conf file - Patch submission
On 1/29/15 6:19 AM, Fabrízio de Royes Mello wrote: On Wed, Jan 28, 2015 at 5:27 PM, Tom Lane t...@sss.pgh.pa.us mailto:t...@sss.pgh.pa.us wrote: =?UTF-8?Q?Fabr=C3=ADzio_de_Royes_Mello?= fabriziome...@gmail.com mailto:fabriziome...@gmail.com writes: But I'm thinking about this patch and would not be interesting to have a FDW to manipulate the hba file? Imagine if we are able to manipulate the HBA file using INSERT/UPDATE/DELETE. Since the HBA file is fundamentally order-dependent, while SQL tables are fundamentally not, that doesn't seem like a great API match. You could probably brute-force something that would work, but it would very much be a case of using a hammer to solve a screwdriver problem. Maybe, but my intention is provide an easy way to edit HBA entries. With an extension or API to edit HBA entries many developers of PostgreSQL tools (ie. pgadmin, phppgadmin, etc) will be benefited. Perhaps a fdw can't be the best choice, maybe a complete new SQL syntax to manipulate HBA entries like we did with ALTER SYSTEM. It's just some thoughts about it. Aside from Tom's concern about sets not being a good way to handle this (which I agree with), the idea of editing pg_hba.conf via SQL raises all the problems that were brought up when ALTER SYSTEM was being developed. One of the big problems is a question of how you can safely modify a text file that's full of comments and what-not. You'd need to address those issues if you hope to modify pg_hba.conf via SQL. -- Jim Nasby, Data Architect, Blue Treble Consulting Data in Trouble? Get it in Treble! http://BlueTreble.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] Providing catalog view to pg_hba.conf file - Patch submission
On 1/28/15 12:46 AM, Haribabu Kommi wrote: Also, what happens if someone reloads the config in the middle of running the SRF? hba entries are reloaded only in postmaster process, not in every backend. So there shouldn't be any problem with config file reload. Am i missing something? Ahh, good point. That does raise another issue though... there might well be some confusion about that. I think people are used to the varieties of how settings come into effect that perhaps this isn't an issue with pg_settings, but it's probably worth at least mentioning in the docs for a pg_hba view. -- Jim Nasby, Data Architect, Blue Treble Consulting Data in Trouble? Get it in Treble! http://BlueTreble.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] Providing catalog view to pg_hba.conf file - Patch submission
On Wed, Jan 28, 2015 at 4:46 AM, Haribabu Kommi kommi.harib...@gmail.com wrote: On Wed, Jan 28, 2015 at 9:47 AM, Jim Nasby jim.na...@bluetreble.com wrote: On 1/27/15 1:04 AM, Haribabu Kommi wrote: Here I attached the latest version of the patch. I will add this patch to the next commitfest. Apologies if this was covered, but why isn't the IP address an inet instead of text? Corrected the address type as inet instead of text. updated patch is attached. Also, what happens if someone reloads the config in the middle of running the SRF? hba entries are reloaded only in postmaster process, not in every backend. So there shouldn't be any problem with config file reload. Am i missing something? I don't have any comment to disagree with the patch (idea and code). But I'm thinking about this patch and would not be interesting to have a FDW to manipulate the hba file? Imagine if we are able to manipulate the HBA file using INSERT/UPDATE/DELETE. Regards, -- Fabrízio de Royes Mello Consultoria/Coaching PostgreSQL Timbira: http://www.timbira.com.br Blog: http://fabriziomello.github.io Linkedin: http://br.linkedin.com/in/fabriziomello Twitter: http://twitter.com/fabriziomello Github: http://github.com/fabriziomello
Re: [HACKERS] Providing catalog view to pg_hba.conf file - Patch submission
=?UTF-8?Q?Fabr=C3=ADzio_de_Royes_Mello?= fabriziome...@gmail.com writes: But I'm thinking about this patch and would not be interesting to have a FDW to manipulate the hba file? Imagine if we are able to manipulate the HBA file using INSERT/UPDATE/DELETE. Since the HBA file is fundamentally order-dependent, while SQL tables are fundamentally not, that doesn't seem like a great API match. You could probably brute-force something that would work, but it would very much be a case of using a hammer to solve a screwdriver problem. 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] Providing catalog view to pg_hba.conf file - Patch submission
On 1/27/15 1:04 AM, Haribabu Kommi wrote:
On Mon, Jun 30, 2014 at 5:06 PM, Abhijit Menon-Sen a...@2ndquadrant.com wrote:
I think having two columns would work. The columns could be called
database and database_list and user and user_list respectively.
The database column may contain one of all, sameuser, samegroup,
replication, but if it's empty, database_list will contain an array of
database names. Then (all, {}) and (, {all}) are easily separated.
Likewise for user and user_list.
Thanks for the review.
I corrected all the review comments except the one to add two columns
as (database, database_list and user, user_list). I feel this may cause
some confusion to the users.
Here I attached the latest version of the patch.
I will add this patch to the next commitfest.
Apologies if this was covered, but why isn't the IP address an inet instead of
text?
Also, what happens if someone reloads the config in the middle of running the
SRF? ISTM it'd be better to do something like process all of parsed_hba_lines
into a tuplestore. Obviously there's still a race condition there, but at least
it's a lot smaller, and AFAIK no worse than the pg_stats views.
--
Jim Nasby, Data Architect, Blue Treble Consulting
Data in Trouble? Get it in Treble! http://BlueTreble.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] Providing catalog view to pg_hba.conf file - Patch submission
On Wed, Jan 28, 2015 at 9:47 AM, Jim Nasby jim.na...@bluetreble.com wrote: On 1/27/15 1:04 AM, Haribabu Kommi wrote: Here I attached the latest version of the patch. I will add this patch to the next commitfest. Apologies if this was covered, but why isn't the IP address an inet instead of text? Corrected the address type as inet instead of text. updated patch is attached. Also, what happens if someone reloads the config in the middle of running the SRF? hba entries are reloaded only in postmaster process, not in every backend. So there shouldn't be any problem with config file reload. Am i missing something? Regards, Hari Babu Fujitsu Australia Catalog_view_to_HBA_settings_patch_V4.patch Description: Binary data -- 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] Providing catalog view to pg_hba.conf file - Patch submission
On Mon, Jun 30, 2014 at 5:06 PM, Abhijit Menon-Sen a...@2ndquadrant.com wrote:
I think having two columns would work. The columns could be called
database and database_list and user and user_list respectively.
The database column may contain one of all, sameuser, samegroup,
replication, but if it's empty, database_list will contain an array of
database names. Then (all, {}) and (, {all}) are easily separated.
Likewise for user and user_list.
Thanks for the review.
I corrected all the review comments except the one to add two columns
as (database, database_list and user, user_list). I feel this may cause
some confusion to the users.
Here I attached the latest version of the patch.
I will add this patch to the next commitfest.
Regards,
Hari Babu
Fujitsu Australia
Catalog_view_to_HBA_settings_patch_V3.patch
Description: Binary data
--
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] Providing catalog view to pg_hba.conf file - Patch submission
At 2014-06-29 22:25:54 +0530, a...@2ndquadrant.com wrote:
I think the really right thing to do would be to have two separate
columns, one with all, sameuser, samerole, replication, or
empty; and the other an array of database names.
After sleeping on it, I realised that the code would return '{all}' for
'all' in pg_hba.conf, but '{all}' for 'all'. So it's not exactly
ambiguous, but I don't think it's especially useful for callers.
I think having two columns would work. The columns could be called
database and database_list and user and user_list respectively.
The database column may contain one of all, sameuser, samegroup,
replication, but if it's empty, database_list will contain an array of
database names. Then (all, {}) and (, {all}) are easily separated.
Likewise for user and user_list.
I've marked this patch returned with feedback and moved it to the
August CF after discussion with Vaishnavi.
-- Abhijit
--
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] Providing catalog view to pg_hba.conf file - Patch submission
Hi Vaishnavi.
In addition to Jaime's comments about the functionality, here are are
some comments about the code.
Well, they were supposed to be comments about the code, but it turns out
I have comments about the feature as well. We need to figure out what to
do about the database and user columns. Returning an array containing
e.g. {all} won't fly. We must distinguish between all and {all}.
I don't have a good solution, other than returning two columns each: one
a string, and one an array, only one of them set for any given record.
+ int
+ GetNumHbaLines(void)
+ {
Please add a blank line before this.
+ /*
+ * Fetches the HbaLine corresponding to linenum variable.
+ * Fill the values required to build a tuple, of view pg_hba_settings, in
values pointer.
+ */
+ void
+ GetHbaLineByNum(int linenum, const char **values)
+ {
I suggest naming this function GetHbaValuesByLinenum() and changing the
comment to Fill in suitable values to build a tuple representing the
HbaLine corresponding to the given linenum.
Actually, maybe it should be hba_getvaluesbyline() for consistency with
the other functions in the file? In that case, the preceding function
should also be renamed to hba_getnumlines().
+ /* Retrieving connection type */
+ switch (hba-conntype)
+ {
The comment should be just Connection type. Generally speaking, all of
the Retrieving comments should be changed similarly. Or just removed.
+ case ctLocal:
+ values[0] = pstrdup(Local);
+ break;
I agree with Jaime: this should be lowercase. And do you really need to
pstrdup() these strings?
+ appendStringInfoString(str, {);
+ foreach(dbcell, hba-databases)
+ {
+ tok = lfirst(dbcell);
+ appendStringInfoString(str, tok-string);
+ appendStringInfoString(str, ,);
+ }
+
+ /*
+ * For any number of values inserted, separator at the end needs to be
+ * replaced by string terminator
+ */
+ if (str.len 1)
+ {
+ str.data[str.len - 1] = '\0';
+ str.len -= 1;
+ appendStringInfoString(str, });
+ values[1] = str.data;
+ }
+ else
+ values[1] = NULL;
I don't like this at all. I would write it something like this:
n = 0;
/* hba-conntype stuff */
n++;
if (list_length(hba-databases) != 0)
{
initStringInfo(str);
appendStringInfoString(str, {);
foreach(cell, hba-databases)
{
tok = lfirst(cell);
appendStringInfoString(str, tok-string);
appendStringInfoString(str, ,);
}
str.data[str.len - 1] = '}';
values[n] = str.data;
}
else
values[n] = NULL;
Note the variable n instead of using 0/1/… indexes into values, and that
I moved the call to initStringInfo from the beginning of the function.
The same applies to the other cases below.
(But this is, of course, all subject to whatever solution is found to
the all/{all} problem.)
/* Retrieving Socket address */
switch (hba-addr.ss_family)
{
case AF_INET:
len = sizeof(struct sockaddr_in);
break;
#ifdef HAVE_IPV6
case AF_INET6:
len = sizeof(struct sockaddr_in6);
break;
#endif
default:
len = sizeof(struct sockaddr_storage);
break;
}
if (getnameinfo((const struct sockaddr *) hba-addr, len, buffer,
sizeof(buffer), NULL, 0, NI_NUMERICHOST) == 0)
values[3] = pstrdup(buffer);
else
values[3] = NULL;
This should use pg_getnameinfo_all. You don't need the switch, just pass
in .salen. Use a buffer of NI_MAXHOST, not 256. Look for other calls to
pg_getnameinfo_all for examples. (Also split long lines.)
(Note: this pstrdup is OK.)
/* Retrieving socket mask */
switch (hba-mask.ss_family)
{
case AF_INET:
Same.
+ case ipCmpMask:
+ values[5] = pstrdup(Mask);
+ break;
Lowercase, no pstrdup.
+ case uaReject:
+ values[7] = pstrdup(Reject);
+ break;
Same.
+ if ((hba-usermap) (hba-auth_method == uaIdent || hba-auth_method
== uaPeer || hba-auth_method == uaCert || hba-auth_method == uaGSS ||
hba-auth_method == uaSSPI))
Split across lines.
+ appendStringInfoString(str, pstrdup(hba-ldapserver));
No need for the many, many pstrdup()s.
+ if (str.len 1)
+ {
+ str.data[str.len - 1] = '\0';
+ str.len -= 1;
+ appendStringInfoString(str, });
+ values[8] = str.data;
+ }
+ else
+ values[8] = NULL;
This should become:
if (str.len != 0)
{
str.data[str.len - 1] = '}';
values[n] = str.data;
}
else
values[n] = NULL;
+ /*
Re: [HACKERS] Providing catalog view to pg_hba.conf file - Patch submission
On Fri, Mar 14, 2014 at 12:30 AM, Prabakaran, Vaishnavi vaishna...@fast.au.fujitsu.com wrote: Hi, In connection to my previous proposal about providing catalog view to pg_hba.conf file contents , I have developed the attached patch . [...] [What this Patch does] Functionality of the attached patch is that it will provide a new view pg_hba_settings to admin users. Public access to the view is restricted. This view will display basic information about HBA setting details of postgresql cluster. Information to be shown , is taken from parsed hba lines and not directly read from pg_hba.conf files. Documentation files are also updated to include details of this new view under Chapter 47.System Catalogs. Also , a new note is added in chapter 19.1 The pg_hba.conf File A normal user can see all the info the view provide once you GRANT permissions on it. How much info should a non-superuser see from this view? currently a non-superuser can't see pg_hba info, now it can. This function should be superuser only or only show info related for current_user if it user is not superuser. Also, i think you should use lowercase values just they are in pg_hba.conf (ie: local not Local, host not Host, etc) -- Jaime Casanova www.2ndQuadrant.com Professional PostgreSQL: Soporte 24x7 y capacitación Phone: +593 4 5107566 Cell: +593 987171157 -- 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] Providing catalog view to pg_hba.conf file - Patch submission
On Friday, Mar 14, 2014 at 9:33 PM, Maganus Hagander mag...@hagander.netmailto:mag...@hagander.net wrote: Hi, In connection to my previous proposal about providing catalog view to pg_hba.conf file contents , I have developed the attached patch . [Current situation] Currently, to view the pg_hba.conf file contents, DB admin has to access the file from database server to read the settings. In case of huge and multiple hba files, finding the appropriate hba rules which are loaded will be difficult and take some time. [What this Patch does] Functionality of the attached patch is that it will provide a new view pg_hba_settings to admin users. Public access to the view is restricted. This view will display basic information about HBA setting details of postgresql cluster. Information to be shown , is taken from parsed hba lines and not directly read from pg_hba.conf files. Documentation files are also updated to include details of this new view under Chapter 47.System Catalogs. Also , a new note is added in chapter 19.1 The pg_hba.conf File [Advantage] Advantage of having this pg_hba_settings view is that the admin can check, what hba rules are loaded in runtime via database connection itself. And, thereby it will be easy and useful for admin to check all the users with their privileges in a single view to manage them. This looks like a useful feature, so make sure you register it on https://commitfest.postgresql.org/action/commitfest_view?id=22. Sure, I will add it to commitfest. I haven't looked at the actual code yet, btu I did notice one thing at a very quick lookover at the docs - it seems to be completely ignoring the key/value parameters given on a row, and stops reporting after the auth method? That seems bad. And also, probably host/mask should be using the inet style datatypes and not text? Added new column configuration_option to pg_hba_settings view to display the key/value parameter set. Attached the updated patch. Thanks Regards, Vaishnavi Fujitsu Australia Catalog_view_to_HBA_settings_patch_V2.patch Description: Catalog_view_to_HBA_settings_patch_V2.patch -- 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] Providing catalog view to pg_hba.conf file - Patch submission
From: Magnus Hagander [mailto:mag...@hagander.net] Sent: Friday, 14 March 2014 9:33 PM To: Prabakaran, Vaishnavi Cc: PostgreSQL-development Subject: Re: [HACKERS] Providing catalog view to pg_hba.conf file - Patch submission On Fri, Mar 14, 2014 at 6:30 AM, Prabakaran, Vaishnavi vaishna...@fast.au.fujitsu.commailto:vaishna...@fast.au.fujitsu.com wrote: Hi, In connection to my previous proposal about providing catalog view to pg_hba.conf file contents , I have developed the attached patch . [Current situation] Currently, to view the pg_hba.conf file contents, DB admin has to access the file from database server to read the settings. In case of huge and multiple hba files, finding the appropriate hba rules which are loaded will be difficult and take some time. [What this Patch does] Functionality of the attached patch is that it will provide a new view pg_hba_settings to admin users. Public access to the view is restricted. This view will display basic information about HBA setting details of postgresql cluster. Information to be shown , is taken from parsed hba lines and not directly read from pg_hba.conf files. Documentation files are also updated to include details of this new view under Chapter 47.System Catalogs. Also , a new note is added in chapter 19.1 The pg_hba.conf File [Advantage] Advantage of having this pg_hba_settings view is that the admin can check, what hba rules are loaded in runtime via database connection itself. And, thereby it will be easy and useful for admin to check all the users with their privileges in a single view to manage them. This looks like a useful feature, so make sure you register it on https://commitfest.postgresql.org/action/commitfest_view?id=22. I haven't looked at the actual code yet, btu I did notice one thing at a very quick lookover at the docs - it seems to be completely ignoring the key/value parameters given on a row, and stops reporting after the auth method? That seems bad. And also, probably host/mask should be using the inet style datatypes and not text? Agree, am now working on including a new column configuration_option to display the key/value parameter set. I will send the updated patch once after adding new column. Host/mask values are stored as sockaddr_storage structure in parsed_hba_lines, so I have used text datatype to display the hostname. Thanks Regards, Vaishnavi Fujitsu Australia -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Re: [HACKERS] Providing catalog view to pg_hba.conf file - Patch submission
On Fri, Mar 14, 2014 at 6:30 AM, Prabakaran, Vaishnavi vaishna...@fast.au.fujitsu.com wrote: Hi, In connection to my previous proposal about providing catalog view to pg_hba.conf file contents , I have developed the attached patch . [Current situation] Currently, to view the pg_hba.conf file contents, DB admin has to access the file from database server to read the settings. In case of huge and multiple hba files, finding the appropriate hba rules which are loaded will be difficult and take some time. [What this Patch does] Functionality of the attached patch is that it will provide a new view pg_hba_settings to admin users. Public access to the view is restricted. This view will display basic information about HBA setting details of postgresql cluster. Information to be shown , is taken from parsed hba lines and not directly read from pg_hba.conf files. Documentation files are also updated to include details of this new view under Chapter 47.System Catalogs. Also , a new note is added in chapter 19.1 The pg_hba.conf File [Advantage] Advantage of having this pg_hba_settings view is that the admin can check, what hba rules are loaded in runtime via database connection itself. And, thereby it will be easy and useful for admin to check all the users with their privileges in a single view to manage them. This looks like a useful feature, so make sure you register it on https://commitfest.postgresql.org/action/commitfest_view?id=22. I haven't looked at the actual code yet, btu I did notice one thing at a very quick lookover at the docs - it seems to be completely ignoring the key/value parameters given on a row, and stops reporting after the auth method? That seems bad. And also, probably host/mask should be using the inet style datatypes and not text? -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
[HACKERS] Providing catalog view to pg_hba.conf file - Patch submission
Hi, In connection to my previous proposal about providing catalog view to pg_hba.conf file contents , I have developed the attached patch . [Current situation] Currently, to view the pg_hba.conf file contents, DB admin has to access the file from database server to read the settings. In case of huge and multiple hba files, finding the appropriate hba rules which are loaded will be difficult and take some time. [What this Patch does] Functionality of the attached patch is that it will provide a new view pg_hba_settings to admin users. Public access to the view is restricted. This view will display basic information about HBA setting details of postgresql cluster. Information to be shown , is taken from parsed hba lines and not directly read from pg_hba.conf files. Documentation files are also updated to include details of this new view under Chapter 47.System Catalogs. Also , a new note is added in chapter 19.1 The pg_hba.conf File [Advantage] Advantage of having this pg_hba_settings view is that the admin can check, what hba rules are loaded in runtime via database connection itself. And, thereby it will be easy and useful for admin to check all the users with their privileges in a single view to manage them. Thanks Regards, Vaishnavi Fujitsu Australia Catalog_view_to_HBA_settings_patch.patch Description: Catalog_view_to_HBA_settings_patch.patch -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
