Обсуждение: 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.

Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> 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

On Mon, May 2, 2022 at 10:54 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> 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

Robert Haas <robertmhaas@gmail.com> 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

On Tue, May 3, 2022 at 11:45 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> 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

On Tue, May 3, 2022 at 10:43 PM Robert Haas <robertmhaas@gmail.com> wrote:
>
> On Tue, May 3, 2022 at 11:45 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > Robert Haas <robertmhaas@gmail.com> 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.

On Wed, May 4, 2022 at 7:12 AM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> 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

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.

On Sat, May 7, 2022 at 12:11 AM Euler Taveira <euler@eulerto.com> 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.

On Mon, May 9, 2022 at 3:44 AM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> 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