Re: proposal: a validator for configuration files

Hi!

On Wed, Mar 30, 2011 at 8:40 AM, Alexey Klyukin <alexk(at)commandprompt(dot)com> wrote:

> I'd like to add an ability to validate the corectness of PostgreSQL
> configuration files, i.e. postgresql.conf, pg_hba.conf, pg_ident.conf without
> actually applying them. The idea is that it would be a command-line option to
> postgres for a stand-alone case, or a user-callable function when postmaster
> is running.
>
> Per the former discussion of a validator for PostgreSQL configuration files
> (see http://archives.postgresql.org/pgsql-hackers/2008-08/msg00048.php),
> here's an implementation proposal.

I did a little bit of work on this, and we discussed it here:

http://archives.postgresql.org/pgsql-hackers/2009-03/msg00345.php
http://archives.postgresql.org/pgsql-hackers/2009-03/msg01142.php

Probably there's a bit of bitrot in there.

> The development plan consists of 2 parts.
> The first one is to add new code that would allow running the checks in both a
> stand-alone process, when postmaster is not running, and as a function call
> from a backend postgres process. Initially the code would simply loads
> configuration files, without performing any of the validation checks. The
> second part consists of adding specific checks.

Cool! Mine was only going to work if the system started up or was reloaded.

> I think most of the code related to this feature should be put into the
> auxiliary process. The rationale is that we already have a stand-alone
> CheckerProcess, which nowadays only parses postgresql.conf, runs BaseInit and
> exists. We can easily load pg_hba.conf and pg_ident.conf and run all the
> necessary checks there. Moreover, the same process can be used when checks are
> launched from a built-in function. In that case, it would save the postgres
> backend from reloading postgresql.conf, pg_hba.conf and pg_ident.conf
> internally and restoring the previous configuration options when the function
> exists. Below is a more detailed description of implementation steps:
>
> 1.1. Stand-alone process (postmaster is not running):
>
> - Add a new option (--check-config) to main.c. Run AuxiliaryProcessMain with
>  auxType= CheckerProcess if this option is specified.
> - In CheckerModeMain load pg_hba.conf, pg_ident.conf
>
>
> 1.2. Built-in function, called from a backend process.
> - Add a new built-in function
> - Add LastCheckState shared flag to denote whether the check has been
>  successful or failed
> - Add a new PMSignalReason
> - From the function call SendPostmasterSignal with the reason added on the
>  previous step.
> - In the postmaster add a check for this reason in sigusr1_handler, spawning
>  a checker process. Set LastCheckStatus to InProgress.
> - Store current configuration options in AuxillaryProcessMain before reloading
>  configuration files to avoid checking for options that haven't changed.
> - In AuxillaryProcessMain, modify SelectConfigFiles invocation to call it
>  if IsUnderPostmaster = true if auxType process type is CheckerProcess.
> - In the CheckerModeMain, set LastCheckState to either success or failure at the
>  end of all checks.
> - The built-in function would wait until LastCheckState changes its value from
>  InProgress to either Succcess or Failure, or until some predefined timeout
>  expires, in which case it would emit an error.
>
> 2. Add following configuration checks:
>
> postgresql.conf:
>
> Check that:
>  1. postgres can bind to listen address:port
>  2. unix_socket_directory has proper permissions (n/a on Windows).
>  3. unix_socket_group group exists on a target system (n/a Windows).
>  4. unix_socket_permissions are valid (write permission is not revoked from
>        the owner, or a group, if group is different from the user).
>  5. server.crt and server.key exist in the data directory if ssl is
>        set to true (and server is compiled with openssl)
>  6. krb_server_keyfile is readable by the server (if  set)
>  7. server is not requesting more shared memory than it's available in the system.
>  8. shared_preload_libraries and local_preload_libraries exist.
>  9. synchronous_standby_names are not empty and max_wal_senders
>        are > 0 if synchronous_replication = true
>  10. all enable_*  query planner parameters are on.
>  11. seq_page_cost <= random_page_cost, and cpu_ costs are lower than page_ costs.
>  12. effective_cache_size is less than the amount of physical memory available.
>  13. geqo is turned on
>  14. default_statistics_target is >= 100
>  15. logging collector is enabled if log destination is stderr
>  16. log directory either exists and writable or that the parent
>         directory allows creating subdris
>  17. track counts is on if autovacuum is enabled
>  18. stats_temp_directory is writeable
>  19. default tablespace exists (if set)
>  20. temp_tablespace exists (if set)
>  21. statement_timeout is 0 (disabled)
>  22. dynamic_library_path exists
>  23. sql_inheritance is off
>  24. zero_damaged_pages is off

Some of this seems not like things that postgres itself should be
doing, but more something that an external validation program (like
what Greg Smith maintains) would do.

> pg_hba.conf:
> Check that:
>  1. the server is compiled with ssl if hostssl is specified
>  2. ssl is not required if hostnossl is specified
>
> - Add a Warning value to LastCheckState for cases when configuration
> files were loaded successfully, but one or more validation checks have failed.
>
> Note that these are basic checks don't require the checker process to have
> access to the database, Also, with this implementation, a client won't receive
> an exact error message in case validation is unsuccessful, however, it would
> receive a status (success, failure, warnings), and an exact error message
> would be available in the server's log. It's possible to address these
> shortcomings in the future.
>
> Ideas, suggestions are welcome.

As I said above, some of what you've suggested seems more like a
non-postgres core thing.. maybe an extension? Or maybe offer the
option to read

My idea was to just check that settings were *valid* not that they met
some other, more subjective criteria.

-selena

--
http://chesnok.com