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 9:12 PM, Stephen Frost <sfrost@snowman.net> wrote:
> You will not find me argueing to allow that in normal operation, or most
> direct-catalog hacking.  I'd be much happier if we had all the ALTER,
> etc, options necessary to prevent any need to ever touch the catalogs.
> Similairly, it'd be nice to have more permission options which reduce
> the need for anyone to be superuser.

Sure, I agree.  But you can't burden this patch with the job of
conforming to what you and I think the project policy ought to be, but
isn't.  Right now, as things stand today, the superuser is one step
short of God Almighty.  The only way in which we even attempt to
restrict the superuser is to try to keep her from hijacking the
postgres login account.  But we don't even try to do that very
thoroughly, as has recently been pointed out on other threads; there
are several plausible attack vectors for her to do just that.  This
patch fits right into that existing philosophy.  If we take this
patch, and I think we should, and later change our policy, then the
permissions around this may require adjustment to fit the new policy.
Fine!  But that policy change is properly a separate discussion.

>> 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.
>
> That category of error is much more likely to get caught by the sysadmin
> doing the restart after the update..

If you edit postgresql.conf manually today, you will have no warning
of ANY sort of error you might make.  With a feature like this, we can
catch a large subset of things you might do wrong *before we even
modify the file*.  Yes, there will be some that slip through, but an
imperfect mechanism for catching mistakes is better than no mechanism
at all.

> ALTER SYSTEM doesn't provide any
> way to restart the DB to make sure it worked.
> That, in turn, encourages
> modification of the config parameters w/o a restart, which is a pretty
> bad idea, imv.

"vi" doesn't provide any way to restart the DB to make sure it worked,
either, and never will.  However, ALTER SYSTEM could be extended in
exactly that way: we could have ALTER SYSTEM RESTART.  Of course some
people are likely to argue that's a bad idea, so we'd have to have
that discussion and think carefully about the issues, but there's
nothing intrinsic that restricts us from giving ALTER SYSTEM any
arbitrary set of capabilities we might want it to have.  Even if we
don't ever do that, we're no worse off than today.

Well, with one exception.  If we make it easier to modify the
configuration file, then people are more likely to do it, and thus
more likely to do it wrong.  But you could use that argument against
any change that improves ease-of-use.  And ease-of-use is a feature,
not a bug.

>> 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.
>
> This is only true if -contrib is installed, which a *lot* of admins
> refuse to do, specifically because of such insane modules as this.
> Therefore, I would argue that it's not nearly as small a change wrt
> exposure as you believe.  Strictly speaking, I don't believe it's a huge
> security hole or something, but I do think it's going to surprise admins
> who aren't following the lists.

If we take the patch, it will be in the release notes, just like any
other feature.  I suspect there will be more people *pleasantly*
surprised than anything else.  If there's anything I've learned about
ease-of-use as a 10+-year veteran of PostgreSQL, it's that being able
to do everything via a database connection is really, really, really
convenient.  That's why foreign data wrappers are awesome.  Instead of
installing a foreign data wrapper for Oracle and pointing it at an
Oracle server and then sucking data down over the link to store or
process or whatever, you put all that logic in a client which knows
how to talk to both PostgreSQL and Oracle.  It turns out, though, that
having that capability *in the server* is really valuable to people.
It doesn't let you do anything you couldn't do otherwise, but it's
extremely convenient.  It's also why we keep hearing requests to put a
scheduler in core.  It's not that you *can't* schedule tasks using
cron or Scheduled Tasks on Windows; it's that it's much more
convenient to be able to do it all in one place, namely PostgreSQL.
It is 100% possible for us to tell all of our users, sorry, you've got
to keep using vi to change configuration settings, because we don't
trust you to do that via SQL.  But I think it's the WRONG decision.
Users like being trusted.  And it's not that hard for the DBA to
disable this or any other feature if he or she feels a real need to do
so.

> This wouldn't be the first time we've said "do it externally and when
> we figure out a better answer then we'll put it into core."

No, but I don't think there's anything terribly wrong with *this*
answer.  It's the product of a lot of people hashing out a lot of
issues, and I think it's pretty good.  Pushing things out externally
is appropriate when the best design is unclear and especially when
little or no core support is required, but I think we have a pretty
broad consensus on the general contours of how this should work, and
some core support is needed.  Someone could certainly do something
better than adminpack without core integration, but what's being
proposed here is better still.

>> 1. disabling the feature by default, and providing no way for it to be
>> turned on remotely, and
>
> Yes, that's quite intentional because it would make someone knowledgable
> about the *overall system* be aware of this rather than having it sprung
> on them when they discover some change has made the database unable to
> start.

We add new features in every release.  I don't see that this one is
dangerous enough that it needs to be hedged about with disclaimers we
don't attach to any of our other features.  My favorite example is the
release where we changed the default behavior of TRUNCATE so that it
cascades to child tables instead of affecting only the parent.  Talk
about springing changes on people that could have catastrophic
consequences.  People have to read the release notes before upgrading
to new major versions, or they will get surprised.  That's why we have
release notes, and that's why we have stable minor versions.

I would also just like to note in passing that I believe the shared
memory changes in 9.3 eliminate the #1 reason for "my database won't
start after a config change", which IME has always been that the
change pushed up the shared memory allocation just beyond what the OS
would allow.  That hazard affected a large number of different
configuration parameters, and it doesn't exist any more.  You can
still bump max_connections or max_autovacuum_workers or the new
max_worker_processes high enough to eat up all the system's
semaphores, and I'm sure someone has killed their system that way
before, but I've personally not run into it.  You might also increase
the size of shared memory so much that you actually overrun what's
available on the box, rather than just overrunning the limit on System
V shared memory, but that requires a much larger adjustment to the
parameter values and is therefore likely to be the result of abject
stupidity rather than simple bad luck.

> While I appreciate that the original thrust behind this may be trying to
> replace adminpack, I find that module to be utterly horrendous in what
> it does and if we don't replicate all that it can do, so much the
> better.  Consider a similar proposal as ALTER SYSTEM which simply
> rewrites pg_hba.conf, ala how adminpack could.  Would we punt on the
> hard questions around things like how to specify the order in which
> entries in pg_hba.conf are maintained?  We could have an ALTER HBA today
> which takes a line number to insert a string of text into, but is it a
> good idea or design?  Certainly not.  My point here is simply that
> focusing on replacing adminpack is not the basis for a feature or
> design.

I partially agree with that, but the point of mentioning adminpack is
that the horse is already halfway out of the barn.  I think this
feature is pretty good as designed and not half-baked at all, but even
if you feel otherwise it's surely a heck of a lot better than what
exists 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.
>
> Too often do we consider sloppy handling like this, which is great for a
> dev or testing environment, to be appropriate for production systems. :(

Well, IMHO, the solution to that is to stop relying on hand-edited
files.  I've never really liked the fact that postgresql.conf is a
text file rather than somehow part of the database guts, requiring a
special tool (that can do the appropriate validation) to make changes.But that ship has already sailed and doesn't seem
likelyto reverse
 
course any time soon.

...Robert