Configuration Parameter/GUC value validation hook
Hi, Right now postgres can't prevent users setting certain configuration parameters or GUCs (like shared_buffers, temp_buffers, work_mem, maintenance_work_mem, max_stack_depth, temp_file_limit, max_worker_processes, other worker processes settings, effective_io_concurrency and so on) to unreasonable values, say shared_buffers to 80% of available memory. What happens is that the server comes up initially and but soon it ends up crashing or some other PANICs or errors. IMO, we all have to agree with the fact that the users setting these parameters aren't always familiar with the consequences of unreasonable values and come to the vendors to bring back their server up after it crashed and went down. Mostly, these parameters, that worry the vendors, are some or the other way platform/Virtual Machine configuration (vcores, RAM, OS, disk) dependent and vary offering to offering. Of course, each postgres vendor can implement their own solution in their control plane or somewhere in the service stack before allowing users to set these values, but that involves looking at the parameters and their type which isn't good from maintainability and extensibility (if the server adds a new GUC or changes data type of a certain parameter) perspective and it might be difficult to do it right as well. Is there any hook or a way in postgres today, to address the above problem? One way, I can think of to use is to have a ProcessUtility_hook and see if the statement is T_AlterSystemStmt or T_VariableSetStmt or T_AlterDatabaseSetStmt or T_AlterRoleSetStmt type and check for the interested GUC params and allow or reject based on the value (but we might have to do some extra stuff to know the GUC data type and parse the value). And this solution doesn't cover extensions setting the server GUCs or custom GUCs. I propose to add a simple new hook in set_config_option (void set_config_option_hook(struct config_generic *record);) and the vendors can implement their own platform-dependent extensions to accept or reject certain parameters (based on platform/VM configuration) that are of interest to them. Thoughts? Regards, Bharath Rupireddy.
Re: Configuration Parameter/GUC value validation hook
Bharath Rupireddy writes: > I propose to add a simple new hook in set_config_option (void > set_config_option_hook(struct config_generic *record);) and the > vendors can implement their own platform-dependent extensions to > accept or reject certain parameters (based on platform/VM > configuration) that are of interest to them. This seems entirely useless. Vendors are unlikely to have any better idea than we do about what are "reasonable" values. Moreover, if they did, modifying the source code directly would be an easier route to introducing their code than making use of a hook (which'd require finding a way to ensure that some extension is loaded). In general, I think you need a much more concrete use-case than this before proposing a new hook. Otherwise we're going to have tons of hooks that we don't know whether they're actually useful or being used by anyone. regards, tom lane
Re: Configuration Parameter/GUC value validation hook
On Mon, May 2, 2022 at 10:54 AM Tom Lane wrote: > Bharath Rupireddy writes: > > I propose to add a simple new hook in set_config_option (void > > set_config_option_hook(struct config_generic *record);) and the > > vendors can implement their own platform-dependent extensions to > > accept or reject certain parameters (based on platform/VM > > configuration) that are of interest to them. > > This seems entirely useless. Vendors are unlikely to have any better > idea than we do about what are "reasonable" values. Moreover, if they > did, modifying the source code directly would be an easier route to > introducing their code than making use of a hook (which'd require > finding a way to ensure that some extension is loaded). I don't think we should be in the business of encouraging vendors to fork the source code, and I think it is quite likely that a vendor will have better ideas than we do about what values they want to allow their users to set. It's far easier to know what is reasonable in a particular context than it is to make a statement about reasonableness in general. I have some desire here to see us solve this problem not just for service providers, but for users in general. You don't have to be a service provider to want to disallow SET work_mem = '1TB' -- you just need to be a DBA on a system where such a setting will cause bad things to happen. But, if you are a DBA on some random system, you won't likely find a hook to be a particularly useful way of controlling this sort of thing. I feel like Alice wants to do something like GRANT work_mem BETWEEN '1MB' AND '2GB' to bob, not that I'm proposing that particular syntax. I also don't have a clear idea for what to do about GUCs where a range constraint isn't useful. One could allow a list of permissible values, but that might not be powerful enough. One could maybe allow a PL validation function for a value, but that might be complicated to make work. In the end, I don't think providing a hook here is particularly unreasonable. I would be a little sad if it ended up being the only thing we provided, but I'm also not a huge believer in the idea of forcing people to write the patch that I want written as a condition of writing the patch that they want written. -- Robert Haas EDB: http://www.enterprisedb.com
Re: Configuration Parameter/GUC value validation hook
Robert Haas writes: > I have some desire here to see us solve this problem not just for > service providers, but for users in general. You don't have to be a > service provider to want to disallow SET work_mem = '1TB' -- you just > need to be a DBA on a system where such a setting will cause bad > things to happen. But, if you are a DBA on some random system, you > won't likely find a hook to be a particularly useful way of > controlling this sort of thing. Yeah, I think this is a more realistic point. I too am not sure what a good facility would look like. I guess an argument in favor of providing a hook is that we could then leave it to extension authors to try to devise a facility that's useful to end users, rather than having to write an in-core feature. regards, tom lane
Re: Configuration Parameter/GUC value validation hook
On Tue, May 3, 2022 at 11:45 AM Tom Lane wrote: > Robert Haas writes: > > I have some desire here to see us solve this problem not just for > > service providers, but for users in general. You don't have to be a > > service provider to want to disallow SET work_mem = '1TB' -- you just > > need to be a DBA on a system where such a setting will cause bad > > things to happen. But, if you are a DBA on some random system, you > > won't likely find a hook to be a particularly useful way of > > controlling this sort of thing. > > Yeah, I think this is a more realistic point. I too am not sure what > a good facility would look like. I guess an argument in favor of > providing a hook is that we could then leave it to extension authors > to try to devise a facility that's useful to end users, rather than > having to write an in-core feature. RIght. The counter-argument is that if we just do that, then what will likely happen is that people who buy PostgreSQL services from Microsoft, Amazon, EDB, Crunchy, etc. will end up with reasonable options in this area, and people who download the source code from the Internet probably won't. As an open-source project, we might hope to avoid a scenario where it doesn't work unless you buy something. On the third hand, half a loaf is better than nothing. -- Robert Haas EDB: http://www.enterprisedb.com
Re: Configuration Parameter/GUC value validation hook
On Tue, May 3, 2022 at 10:43 PM Robert Haas wrote: > > On Tue, May 3, 2022 at 11:45 AM Tom Lane wrote: > > Robert Haas writes: > > > I have some desire here to see us solve this problem not just for > > > service providers, but for users in general. You don't have to be a > > > service provider to want to disallow SET work_mem = '1TB' -- you just > > > need to be a DBA on a system where such a setting will cause bad > > > things to happen. But, if you are a DBA on some random system, you > > > won't likely find a hook to be a particularly useful way of > > > controlling this sort of thing. > > > > Yeah, I think this is a more realistic point. I too am not sure what > > a good facility would look like. I guess an argument in favor of > > providing a hook is that we could then leave it to extension authors > > to try to devise a facility that's useful to end users, rather than > > having to write an in-core feature. > > RIght. The counter-argument is that if we just do that, then what will > likely happen is that people who buy PostgreSQL services from > Microsoft, Amazon, EDB, Crunchy, etc. will end up with reasonable > options in this area, and people who download the source code from the > Internet probably won't. As an open-source project, we might hope to > avoid a scenario where it doesn't work unless you buy something. On > the third hand, half a loaf is better than nothing. Thanks Tom and Robert for your responses. How about we provide a sample extension (limiting some important parameters say shared_buffers, work_mem and so on to some "reasonable/recommended" limits) in the core along with the set_config_option_hook? This way, all the people using open source postgres out-of-the-box will benefit and whoever wants, can modify that sample extension to suit their needs. The sampe extension can also serve as an example to implement set_config_option_hook. Thoughts? Regards, Bharath Rupireddy.
Re: Configuration Parameter/GUC value validation hook
On Wed, May 4, 2022 at 7:12 AM Bharath Rupireddy wrote: > Thanks Tom and Robert for your responses. > > How about we provide a sample extension (limiting some important > parameters say shared_buffers, work_mem and so on to some > "reasonable/recommended" limits) in the core along with the > set_config_option_hook? This way, all the people using open source > postgres out-of-the-box will benefit and whoever wants, can modify > that sample extension to suit their needs. The sampe extension can > also serve as an example to implement set_config_option_hook. > > Thoughts? Well, it's better than just adding a hook and stopping, but I'm not really sure that it's as good as what I'd like to have. -- Robert Haas EDB: http://www.enterprisedb.com
Re: Configuration Parameter/GUC value validation hook
On Wed, May 4, 2022, at 8:12 AM, Bharath Rupireddy wrote: > How about we provide a sample extension (limiting some important > parameters say shared_buffers, work_mem and so on to some > "reasonable/recommended" limits) in the core along with the > set_config_option_hook? This way, all the people using open source > postgres out-of-the-box will benefit and whoever wants, can modify > that sample extension to suit their needs. The sampe extension can > also serve as an example to implement set_config_option_hook. I agree with Robert that providing a feature for it instead of a hook is the way to go. It is not just one or two vendors that will benefit from it but almost or if not all vendors will use this feature. Hooks should be used for niche features; that's not the case here. The commit a0ffa885e47 introduced the GRANT SET ON PARAMETER command. It could be used for this purpose. Despite of accepting GRANT on PGC_USERSET GUCs, it has no use. It doesn't mean that additional properties couldn't be added to the current syntax. This additional use case should be enforced before or while executing set_config_option(). Is it ok to extend this SQL command? The syntax could be: GRANT SET ON PARAMETER work_mem (MIN '1MB', MAX '512MB') TO bob; NULL keyword can be used to remove the MIN and MAX limit. The idea is to avoid a verbose syntax (add an "action" to MIN/MAX -- ADD MIN 1, DROP MAX 234, SET MIN 456). The other alternative is to ALTER USER SET and ALTER DATABASE SET. The current user can set parameter for himself and he could adjust the limits. Besides that the purpose of these SQL commands are to apply initial settings for a combination of user/database. I'm afraid it is out of scope to check after the session is established. -- Euler Taveira EDB https://www.enterprisedb.com/
Re: Configuration Parameter/GUC value validation hook
On Sat, May 7, 2022 at 12:11 AM Euler Taveira wrote: > > On Wed, May 4, 2022, at 8:12 AM, Bharath Rupireddy wrote: > > How about we provide a sample extension (limiting some important > parameters say shared_buffers, work_mem and so on to some > "reasonable/recommended" limits) in the core along with the > set_config_option_hook? This way, all the people using open source > postgres out-of-the-box will benefit and whoever wants, can modify > that sample extension to suit their needs. The sampe extension can > also serve as an example to implement set_config_option_hook. > > I agree with Robert that providing a feature for it instead of a hook is the > way to go. It is not just one or two vendors that will benefit from it but > almost or if not all vendors will use this feature. Hooks should be used for > niche features; that's not the case here. > > The commit a0ffa885e47 introduced the GRANT SET ON PARAMETER command. It could > be used for this purpose. Despite of accepting GRANT on PGC_USERSET GUCs, it > has no use. It doesn't mean that additional properties couldn't be added to > the > current syntax. This additional use case should be enforced before or while > executing set_config_option(). Is it ok to extend this SQL command? > > The syntax could be: > > GRANT SET ON PARAMETER work_mem (MIN '1MB', MAX '512MB') TO bob; > > NULL keyword can be used to remove the MIN and MAX limit. The idea is to avoid > a verbose syntax (add an "action" to MIN/MAX -- ADD MIN 1, DROP MAX 234, SET > MIN 456). > > The other alternative is to ALTER USER SET and ALTER DATABASE SET. The current > user can set parameter for himself and he could adjust the limits. Besides > that > the purpose of these SQL commands are to apply initial settings for a > combination of user/database. I'm afraid it is out of scope to check after the > session is established. Thanks for providing thoughts. I'm personally not in favour of adding any new syntax, as the new syntax would require some education and changes to other layers. I see some downsides with new syntax: 1) It will be a bit difficult to deal with the parameters that don't have ranges (as pointed out by Robert upthread). 2) It will be a bit difficult to enforce platform specific configurations at run time - (say the user has scaled-up the host system/VM, now has more vcores, RAM and now they will have more memory and number of workers to use for their setting). 3) If someone wants to disallow users setting some core/extension configuration parameters which can make the server unmanageable (block setting full_page_writes to off, zero_damaged_pages to on, fsync to off, log levels to debug5, huge_pages to on, all the command options (archive_command, restore_command etc.). IMO, the hook and a sample extension in the core helps greatly to achieve the above. Thoughts? Regards, Bharath Rupireddy.
Re: Configuration Parameter/GUC value validation hook
On Mon, May 9, 2022 at 3:44 AM Bharath Rupireddy wrote: > Thanks for providing thoughts. I'm personally not in favour of adding > any new syntax, as the new syntax would require some education and > changes to other layers. I see some downsides with new syntax: > 1) It will be a bit difficult to deal with the parameters that don't > have ranges (as pointed out by Robert upthread). > 2) It will be a bit difficult to enforce platform specific > configurations at run time - (say the user has scaled-up the host > system/VM, now has more vcores, RAM and now they will have more memory > and number of workers to use for their setting). > 3) If someone wants to disallow users setting some core/extension > configuration parameters which can make the server unmanageable (block > setting full_page_writes to off, zero_damaged_pages to on, fsync to > off, log levels to debug5, huge_pages to on, all the command options > (archive_command, restore_command etc.). > > IMO, the hook and a sample extension in the core helps greatly to > achieve the above. I don't think that any of these are very fundamental objections. Every feature requires education, many require changes to various layers, and the fact that some parameters don't have ranges is a topic to think about how to handle, not a reason to give up on the idea. (2) may mean that some users - large service providers, in particular - prefer the hook to the SQL syntax, but that's not a reason not to have SQL syntax. (3) basically seems like an argument that people my do dumb things with it, but that's true of every feature. I'm not sure that a hook and sample extension is unacceptable; it might be fine. But I think it is not saying anything other than the truth to say that this will benefit large service providers while leaving the corresponding problem unsolved for ordinary end users. And I remain of the opinion that that's not great. -- Robert Haas EDB: http://www.enterprisedb.com