Re: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: Proposal for Allow postgresql.conf values to be changed via SQL [review]) - Mailing list pgsql-hackers

On Thu, Aug 29, 2013 at 1:42 PM, Stephen Frost <sfrost@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