On Thu, Aug 29, 2013 at 1:42 PM, Stephen Frost <sfr...@snowman.net> wrote: > To be honest, I don't find the arguments of "pgAdmin does it badly" > nor "psql users want this ability" (which I find difficult to believe) > to be particularlly compelling reasons to have a dangerous > implementation (even if it's better than 'adminpack' today) in core.
I don't understand how you can find that difficult to believe. I'm a psql user and I want it. Josh Berkus is a psql user and he wants it. And there are numerous statements of support on these threads from other people as well. The sheer volume of discussion on this topic, and the fact that it has not gone away after years of wrangling, is a clear indication that people do, in fact, want it. To be honest, I think the argument that this is dangerous is pretty ridiculous. AFAICS, the only argument anyone's advanced for this being dangerous is the theory that you might accidentally put something in your postgresql.conf file that makes the server not start. However, the reality is that the superuser has MANY, MANY ways of killing the database cluster as things stand. Consider the ever-popular "DELETE FROM pg_proc". That will not only render your database unusable, but it's a hell of a lot harder to recover from than anything you might do to postgresql.conf. Therefore, from where I'm sitting, this is like telling a head of state with the ability to launch nuclear weapons which will destroy the planet that he isn't allowed to bring his swiss army knife on board an aircraft. Now, you can argue that people are more likely to render the database nonfunctional by changing GUC settings than they are to do it by corrupting the system catalogs, but I'm not sure I believe it. We can, after all, validate that any setting a user supplies is a valid value for that setting before writing it out to the configuration file. It might still make the system fail to start if - for example - it causes too many semaphores to be reserved, or something like that. But why should we think that such mistakes will be common? If anything, it sounds less error-prone to me than hand-editing the configuration file, where typing something like on_exit_error=maybe will make the server fail to start. We can eliminate that whole category of error, at least for people who choose to use the new tools. If you're using the term "dangerous" to refer to a security exposure, I concede there is some incremental exposure from allowing this by default. But it's not a terribly large additional exposure. It's already the case that if adminpack is available the super-user can do whatever he or she wants, because she or he can write to arbitrary files inside the data directory. Even if not, for most intents and purposes, ALTER DATABASE my_main_database SET whatever = thing is functionally equivalent to modifying postgresql.conf. Some settings can't be modified that way, but so what? AFAICS, about the worst thing the user can do is ALTER SYSTEM SET shared_preload_libraries = 'rootkit'. But if the user has the ability to install rootkit.so, the sysadmin is already screwed. And aside from that sort of thing, I don't really see what can happen that's any worse than what a rogue superuser can already do as things stand today. > If it's in core rather than in contrib it's going to be deployed a great > deal farther with a large increase in userbase. I've already stated > that if this is in contrib that my concerns are much less. I don't really see a compelling reason why it can't or shouldn't be in core. But having something better in contrib would still be an improvement on the status quo. >> I think the goals of this patch should be to (1) let users of other >> clients manipulate the startup configuration just as easily as we can >> already do using pgAdmin, > > Which could be done already through use of adminpack.. The capabilities > exposed there could be used by other clients. The fact that none of the > other clients have chosen to do so could be an indication that this > ability isn't terribly 'in demand'. Huh? The problem with adminpack is that it doesn't let you modify individual configuration settings. All you can do is rewrite an entire file. I guess somebody could write a specialized client that just uses that infrastructure to rewrite postgresql.conf. For all I know, someone has. Even if not, I don't think that you can use that to prove that people don't care about this feature. If nobody cares, why are there 400 emails on this topic?! >> and (2) remove some of the concurrency >> hazards that pgAdmin introduces, for example by using locking and >> atomic renames. > > Why can't adminpack do that..? It could. But it doesn't. We could improve it to do that, and that might be worthwhile, but it still wouldn't be as nice as what's being proposed here. >> Restricting the functionality to something less than >> what pgAdmin provides will just cause people to ignore the new >> mechanism and use the one that already exists and, by and large, >> works. And trying to revise other aspects of how GUCs and config >> files work as part of this effort is not reasonably related to this >> patch, and should be kept out of the discussion. > > We're talking about modifying config files through an interface and you > wish to exclude all discussion about how those config files are handled. > That leads to a result that only an adminpack-like solution is an > option, to which I respond "use and improve adminpack then". If we want > something that works well in *core* then I don't think we can exclude > how core reads and handles config files from the discussion. We need a > real solution, not another adminpack-like hack. I'm not sure what you mean about "all discussion about how those config files are handled". I think it's entirely reasonable to discuss how where the automatically-written configuration settings will be stored, and how they'll interact with manually-generated files. But IIUC you have proposed: 1. disabling the feature by default, and providing no way for it to be turned on remotely, and 2. even when the feature is enabled, only allowing a subset of the settings to be changed with it, and 3. also changing the interpretation of the config files so that we have a first-wins rules instead of a last-wins rule. If we do either #1 or #2, this won't be a plausible substitute for the functionality that adminpack offers today. #3 is a bad idea in my book - it would break some of my existing benchmarking scripts, which do initdb; cat >>$PGDATA/postgresql.conf <<EOM. But even if it were a good idea, it isn't a necessary prerequisite for this patch. -- 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
