Merge lp:~chad.smith/charms/precise/postgresql/postgresql-manual-tune-kernel-params-fix into lp:charms/postgresql

[Chad Smith]

Work in progress branch for initial review. Please let me know what you guys think before I go too far down this route. I didn't want to cause any issues by using local "trial" tests instead of juju integration tests, but it seems with a timeout of 15 minutes, the 5 existing integration tests were taking 1hr 15 mins to complete (during error conditions)

This branch does the following:
  - adds basic framework for functional unit tests of hooks.
         - these tests don't involve time-consuming juju unit deployment like the existing integration tests and provide
           local testing of hook functions
  - fixes some of the kernel_shmall, and kernel_shmmax config parameters, so ensure they properly written when
    performance-tuning == "manual"
  - Some cleanup to reduce use of globals and reduce the use of local variable names that match global variable names to avoid confusion. Removing globals also better enabled the local unit testing framework by avoiding a global call to
    hookenv.config() during module import

If we think this is a good initial approach, it should be straight forward for us to add more unit tests to get coverage of most hook functions defined within. All thoughts appreciated.

[lp:~chad.smith/charms/precise/postgresql/postgresql-manual-tune-kernel-params-fix]

87. By Chad Smith on 2014-01-07

uncomment integration tests

88. By Chad Smith on 2014-01-07

linting

89. By Chad Smith on 2014-01-07

revision bump

90. By Chad Smith on 2014-01-07

include python-psutil in install dependencies

[David Britton (dpb)]

Makefile:

[0] make two targets, test and integration-test
[1] remove ls-lint, as that is a landscape specific bzr plugin

[lp:~chad.smith/charms/precise/postgresql/postgresql-manual-tune-kernel-params-fix]

91. By Chad Smith on 2014-01-07

silly comma, I've got you this time

[David Britton (dpb)]

[3] put in Makefile target for "devel" or "build-devel", where it will install appropriate packages

like: sudo apt-get install -y python-mocker python-twisted-core

[4] get rid of the revision file

I agree with the general approach. It's very light, but I think can be expanded as you go forward.

+1

review: Approve

[Stuart Bishop (stub)]

This all looks good. The cleanups are much needed, as are the retrofitted unittests. And those globals... thanks for killing some more of them.

The checks for "wal_buffers = -1" are unnecessary, as this setting is not related to replication.

review: Approve

[Stuart Bishop (stub)]

The file 'revision' needs to be removed before landing

[Stuart Bishop (stub)]

Integration tests pass with the local provider (but still flaky due to the usual juju race conditions).

Merged.

=== added file 'Makefile'
--- Makefile	1970-01-01 00:00:00 +0000
+++ Makefile	2014-01-07 21:59:36 +0000
@@ -0,0 +1,9 @@
+CHARM_DIR := $(shell pwd)
+
+test:
+	cd hooks && CHARM_DIR=$(CHARM_DIR) trial test_hooks.py
+	echo "Integration tests using Juju deployed units"
+	TEST_TIMEOUT=900 ./test.py -v
+
+lint:
+	bzr ls-lint
 
=== modified file 'README.md'
--- README.md	2013-12-23 10:35:30 +0000
+++ README.md	2014-01-07 21:59:36 +0000
@@ -134,3 +134,17 @@
 - `state`: 'standalone', 'master' or 'hot standby'
 - `allowed-units`: space separated list of allowed clients (unit name).
   You should check this to determine if you can connect to the database yet.
+
+### For clustered support
+In order for client charms to support replication the client will need to be
+aware when relation-list reports > 1 unit of postgresql related:
+  - When > 1 postgresql units are related:
+    - if the client charm needs database write access, they will ignore
+      all "standalone", "hot standby" and "failover" states as those will
+      likely come from a standby unit (read-only) during standby install,
+      setup or teardown
+    - If read-only access is needed for a client, acting on
+      db-admin-relation-changed "hot standby" state will provide you with a
+      readonly replicated copy of the db
+  - When 1 postgresql unit is related:
+    - watch for updates to the db-admin-relation-changed with "standalone" state
 
=== modified file 'config.yaml'
--- config.yaml	2013-11-13 10:07:41 +0000
+++ config.yaml	2014-01-07 21:59:36 +0000
@@ -160,7 +160,7 @@
     type: boolean
     description: |
         Hot standby or warm standby. When True, queries can be run against
-        the database is in recovery or standby mode (ie. replicated).
+        the database when in recovery or standby mode (ie. replicated).
         Overridden by juju when master/slave relations are used.
   hot_standby_feedback:
     default: False
@@ -230,28 +230,40 @@
     type: string
     description: |
         Possible values here are "auto" or "manual". If we set "auto" then the
-        charm will attempt to automatically tune all the performance paramaters
-        as below. If manual, then it will use the defaults below unless
-        overridden. "auto" gathers information about the node you're deployed
-        on and tries to make intelligent guesses about what tuning parameters
-        to set based on available RAM and CPU under the assumption that it's
-        the only significant service running on this node.
+        charm will attempt to automatically tune all the performance parameters
+        for kernel_shmall, kernel_shmmax, shared_buffers and
+        effective_cache_size below, unless those config values are explicitly
+        set. If manual, then it will use the defaults below unless set.
+        "auto" gathers information about the node on which you are deployed and
+        tries to make intelligent guesses about what tuning parameters to set
+        based on available RAM and CPU under the assumption that it's the only
+        significant service running on this node.
   kernel_shmall:
     default: 0
     type: int
-    description: Kernel/shmall
+    description: Total amount of shared memory available, in bytes.
   kernel_shmmax:
     default: 0
     type: int
-    description: Kernel/shmmax
+    description: The maximum size, in bytes, of a shared memory segment.
   shared_buffers:
     default: ""
     type: string
-    description: Shared buffers
+    description: |
+        The amount of memory the database server uses for shared memory
+        buffers. This string should be of the format '###MB'.
+  effective_cache_size:
+    default: ""
+    type: string
+    description: |
+        Effective cache size is an estimate of how much memory is available for
+        disk caching within the database. (50% to 75% of system memory). This
+        string should be of the format '###MB'.
   temp_buffers:
     default: "1MB"
     type: string
-    description: Temp buffers
+    description: |
+        The maximum number of temporary buffers used by each database session.
   wal_buffers:
     default: "-1"
     type: string
@@ -265,10 +277,6 @@
     default: 4.0
     type: float
     description: Random page cost
-  effective_cache_size:
-    default: ""
-    type: string
-    description: Effective cache size
   #------------------------------------------------------------------------
   # Volume management
   #   volume-map, volume-dev_regexp are only used 
 
=== modified file 'hooks/hooks.py'
--- hooks/hooks.py	2013-12-23 10:35:30 +0000
+++ hooks/hooks.py	2014-01-07 21:59:36 +0000
@@ -48,7 +48,7 @@
     '''
     myname = hookenv.local_unit().replace('/', '-')
     ts = time.strftime('%Y-%m-%d %H:%M:%S', time.gmtime())
-    with open('/var/log/juju/{}-debug.log'.format(myname), 'a') as f:
+    with open('{}/{}-debug.log'.format(juju_log_dir, myname), 'a') as f:
         f.write('{} {}: {}\n'.format(ts, lvl, msg))
     hookenv.log(msg, lvl)
 
@@ -121,7 +121,7 @@
 def volume_get_volid_from_volume_map():
     volume_map = {}
     try:
-        volume_map = yaml.load(config_data['volume-map'].strip())
+        volume_map = yaml.load(hookenv.config('volume-map').strip())
         if volume_map:
             return volume_map.get(os.environ['JUJU_UNIT_NAME'])
     except ConstructorError as e:
@@ -154,7 +154,7 @@
 # @returns  volid
 #           None    config state is invalid - we should not serve
 def volume_get_volume_id():
-    ephemeral_storage = config_data['volume-ephemeral-storage']
+    ephemeral_storage = hookenv.config('volume-ephemeral-storage')
     volid = volume_get_volid_from_volume_map()
     juju_unit_name = hookenv.local_unit()
     if ephemeral_storage in [True, 'yes', 'Yes', 'true', 'True']:
@@ -195,6 +195,7 @@
 
 
 def postgresql_autostart(enabled):
+    postgresql_config_dir = _get_postgresql_config_dir()
     startup_file = os.path.join(postgresql_config_dir, 'start.conf')
     if enabled:
         log("Enabling PostgreSQL startup in {}".format(startup_file))
@@ -202,8 +203,8 @@
     else:
         log("Disabling PostgreSQL startup in {}".format(startup_file))
         mode = 'manual'
-    contents = Template(open("templates/start_conf.tmpl").read()).render(
-        {'mode': mode})
+    template_file = "{}/templates/start_conf.tmpl".format(hookenv.charm_dir())
+    contents = Template(open(template_file).read()).render({'mode': mode})
     host.write_file(
         startup_file, contents, 'postgres', 'postgres', perms=0o644)
 
@@ -229,7 +230,7 @@
     if status != 0:
         return False
     # e.g. output: "Running clusters: 9.1/main"
-    vc = "%s/%s" % (config_data["version"], config_data["cluster_name"])
+    vc = "%s/%s" % (hookenv.config("version"), hookenv.config("cluster_name"))
     return vc in output.decode('utf8').split()
 
 
@@ -253,7 +254,7 @@
             # success = host.service_restart('postgresql')
             try:
                 run('pg_ctlcluster -force {version} {cluster_name} '
-                    'restart'.format(**config_data))
+                    'restart'.format(**hookenv.config()))
                 success = True
             except subprocess.CalledProcessError:
                 success = False
@@ -327,11 +328,11 @@
     return success
 
 
-def get_service_port(postgresql_config):
+def get_service_port(config_file):
     '''Return the port PostgreSQL is listening on.'''
-    if not os.path.exists(postgresql_config):
+    if not os.path.exists(config_file):
         return None
-    postgresql_config = open(postgresql_config, 'r').read()
+    postgresql_config = open(config_file, 'r').read()
     port = re.search("port.*=(.*)", postgresql_config).group(1).strip()
     try:
         return int(port)
@@ -339,14 +340,26 @@
         return None
 
 
-def create_postgresql_config(postgresql_config):
+def _get_system_ram():
+    """ Return the system ram in Megabytes """
+    import psutil
+    return psutil.phymem_usage()[0] / (1024 ** 2)
+
+
+def _get_page_size():
+    """ Return the operating system's configured PAGE_SIZE """
+    return int(run("getconf PAGE_SIZE"))   # frequently 4096
+
+
+def create_postgresql_config(config_file):
     '''Create the postgresql.conf file'''
+    config_data = hookenv.config()
     if config_data["performance_tuning"] == "auto":
         # Taken from:
         # http://wiki.postgresql.org/wiki/Tuning_Your_PostgreSQL_Server
         # num_cpus is not being used ... commenting it out ... negronjl
         #num_cpus = run("cat /proc/cpuinfo | grep processor | wc -l")
-        total_ram = run("free -m | grep Mem | awk '{print $2}'")
+        total_ram = _get_system_ram()
         if not config_data["effective_cache_size"]:
             config_data["effective_cache_size"] = \
                 "%sMB" % (int(int(total_ram) * 0.75),)
@@ -357,15 +370,22 @@
             else:
                 config_data["shared_buffers"] = \
                     "%sMB" % (int(int(total_ram) * 0.15),)
-        # XXX: This is very messy - should probably be a subordinate charm
-        conf_file = open("/etc/sysctl.d/50-postgresql.conf", "w")
-        conf_file.write("kernel.sem = 250 32000 100 1024\n")
-        conf_file.write("kernel.shmall = %s\n" %
-                        ((int(total_ram) * 1024 * 1024) + 1024),)
-        conf_file.write("kernel.shmmax = %s\n" %
-                        ((int(total_ram) * 1024 * 1024) + 1024),)
-        conf_file.close()
-        run("sysctl -p /etc/sysctl.d/50-postgresql.conf")
+        config_data["kernel_shmmax"] = (int(total_ram) * 1024 * 1024) + 1024
+        config_data["kernel_shmall"] = config_data["kernel_shmmax"]
+
+    # XXX: This is very messy - should probably be a subordinate charm
+    lines = ["kernel.sem = 250 32000 100 1024\n"]
+    if config_data["kernel_shmall"] > 0:
+        # Convert config kernel_shmall (bytes) to pages
+        page_size = _get_page_size()
+        num_pages = config_data["kernel_shmall"] / page_size
+        if (config_data["kernel_shmall"] % page_size) > 0:
+            num_pages += 1
+        lines.append("kernel.shmall = %s\n" % num_pages)
+    if config_data["kernel_shmmax"] > 0:
+        lines.append("kernel.shmmax = %s\n" % config_data["kernel_shmmax"])
+    host.write_file(postgresql_sysctl, ''.join(lines), perms=0600)
+    run("sysctl -p {}".format(postgresql_sysctl))
 
     # If we are replicating, some settings may need to be overridden to
     # certain minimum levels.
@@ -383,28 +403,31 @@
 
     # Send config data to the template
     # Return it as pg_config
+    charm_dir = hookenv.charm_dir()
+    template_file = "{}/templates/postgresql.conf.tmpl".format(charm_dir)
     pg_config = Template(
-        open("templates/postgresql.conf.tmpl").read()).render(config_data)
+        open(template_file).read()).render(config_data)
     host.write_file(
-        postgresql_config, pg_config,
+        config_file, pg_config,
         owner="postgres",  group="postgres", perms=0600)
 
     local_state['saved_config'] = config_data
     local_state.save()
 
 
-def create_postgresql_ident(postgresql_ident):
+def create_postgresql_ident(output_file):
     '''Create the pg_ident.conf file.'''
     ident_data = {}
-    pg_ident_template = Template(
-        open("templates/pg_ident.conf.tmpl").read())
+    charm_dir = hookenv.charm_dir()
+    template_file = "{}/templates/pg_ident.conf.tmpl".format(charm_dir)
+    pg_ident_template = Template(open(template_file).read())
     host.write_file(
-        postgresql_ident, pg_ident_template.render(ident_data),
+        output_file, pg_ident_template.render(ident_data),
         owner="postgres", group="postgres", perms=0600)
 
 
 def generate_postgresql_hba(
-        postgresql_hba, user=None, schema_user=None, database=None):
+        output_file, user=None, schema_user=None, database=None):
     '''Create the pg_hba.conf file.'''
 
     # Per Bug #1117542, when generating the postgresql_hba file we
@@ -429,6 +452,7 @@
                 return output.rstrip(".")  # trailing dot
             return addr
 
+    config_data = hookenv.config()
     allowed_units = set()
     relation_data = []
     relids = hookenv.relation_ids('db') + hookenv.relation_ids('db-admin')
@@ -525,9 +549,10 @@
                 'private-address': munge_address(admin_ip)}
             relation_data.append(admin_host)
 
-    pg_hba_template = Template(open("templates/pg_hba.conf.tmpl").read())
+    template_file = "{}/templates/pg_hba.conf.tmpl".format(hookenv.charm_dir())
+    pg_hba_template = Template(open(template_file).read())
     host.write_file(
-        postgresql_hba, pg_hba_template.render(access_list=relation_data),
+        output_file, pg_hba_template.render(access_list=relation_data),
         owner="postgres", group="postgres", perms=0600)
     postgresql_reload()
 
@@ -539,27 +564,36 @@
             relid, {"allowed-units": " ".join(unit_sorted(allowed_units))})
 
 
-def install_postgresql_crontab(postgresql_ident):
+def install_postgresql_crontab(output_file):
     '''Create the postgres user's crontab'''
+    config_data = hookenv.config()
     crontab_data = {
         'backup_schedule': config_data["backup_schedule"],
         'scripts_dir': postgresql_scripts_dir,
         'backup_days': config_data["backup_retention_count"],
     }
+    charm_dir = hookenv.charm_dir()
+    template_file = "{}/templates/postgres.cron.tmpl".format(charm_dir)
     crontab_template = Template(
-        open("templates/postgres.cron.tmpl").read()).render(crontab_data)
-    host.write_file('/etc/cron.d/postgres', crontab_template, perms=0600)
+        open(template_file).read()).render(crontab_data)
+    host.write_file(output_file, crontab_template, perms=0600)
 
 
 def create_recovery_conf(master_host, restart_on_change=False):
+    version = hookenv.config('version')
+    cluster_name = hookenv.config('cluster_name')
+    postgresql_cluster_dir = os.path.join(
+        postgresql_data_dir, version, cluster_name)
+
     recovery_conf_path = os.path.join(postgresql_cluster_dir, 'recovery.conf')
     if os.path.exists(recovery_conf_path):
         old_recovery_conf = open(recovery_conf_path, 'r').read()
     else:
         old_recovery_conf = None
 
-    recovery_conf = Template(
-        open("templates/recovery.conf.tmpl").read()).render({
+    charm_dir = hookenv.charm_dir()
+    template_file = "{}/templates/recovery.conf.tmpl".format(charm_dir)
+    recovery_conf = Template(open(template_file).read()).render({
         'host': master_host,
         'password': local_state['replication_password']})
     log(recovery_conf, DEBUG)
@@ -578,9 +612,9 @@
 #                          Returns a string containing the postgresql config or
 #                          None
 #------------------------------------------------------------------------------
-def load_postgresql_config(postgresql_config):
-    if os.path.isfile(postgresql_config):
-        return(open(postgresql_config).read())
+def load_postgresql_config(config_file):
+    if os.path.isfile(config_file):
+        return(open(config_file).read())
     else:
         return(None)
 
@@ -677,7 +711,11 @@
 #     - manipulate /var/lib/postgresql/VERSION/CLUSTER symlink
 #------------------------------------------------------------------------------
 def config_changed_volume_apply():
-    data_directory_path = postgresql_cluster_dir
+    version = hookenv.config('version')
+    cluster_name = hookenv.config('cluster_name')
+    data_directory_path = os.path.join(
+        postgresql_data_dir, version, cluster_name)
+
     assert(data_directory_path)
     volid = volume_get_volume_id()
     if volid:
@@ -698,7 +736,7 @@
         mount_point = volume_mount_point_from_volid(volid)
         new_pg_dir = os.path.join(mount_point, "postgresql")
         new_pg_version_cluster_dir = os.path.join(
-            new_pg_dir, config_data["version"], config_data["cluster_name"])
+            new_pg_dir, version, cluster_name)
         if not mount_point:
             log(
                 "invalid mount point from volid = {}, "
@@ -724,7 +762,7 @@
         #   /var/lib/postgresql/9.1/main
         curr_dir_stat = os.stat(data_directory_path)
         for new_dir in [new_pg_dir,
-                        os.path.join(new_pg_dir, config_data["version"]),
+                        os.path.join(new_pg_dir, version),
                         new_pg_version_cluster_dir]:
             if not os.path.isdir(new_dir):
                 log("mkdir %s".format(new_dir))
@@ -781,7 +819,8 @@
 
 @hooks.hook()
 def config_changed(force_restart=False):
-    update_repos_and_packages()
+    config_data = hookenv.config()
+    update_repos_and_packages(config_data["version"])
 
     # Trigger volume initialization logic for permanent storage
     volid = volume_get_volume_id()
@@ -813,10 +852,17 @@
                 "Disabled and stopped postgresql service "
                 "(config_changed_volume_apply failure)", ERROR)
             sys.exit(1)
+
+    postgresql_config_dir = _get_postgresql_config_dir(config_data)
+    postgresql_config = os.path.join(postgresql_config_dir, "postgresql.conf")
+    postgresql_hba = os.path.join(postgresql_config_dir, "pg_hba.conf")
+    postgresql_ident = os.path.join(postgresql_config_dir, "pg_ident.conf")
+
     current_service_port = get_service_port(postgresql_config)
     create_postgresql_config(postgresql_config)
     generate_postgresql_hba(postgresql_hba)
     create_postgresql_ident(postgresql_ident)
+
     updated_service_port = config_data["listen_port"]
     update_service_port(current_service_port, updated_service_port)
     update_nrpe_checks()
@@ -832,8 +878,8 @@
             if os.path.isfile(f) and os.access(f, os.X_OK):
                 subprocess.check_call(['sh', '-c', f])
 
-    update_repos_and_packages()
-
+    config_data = hookenv.config()
+    update_repos_and_packages(config_data["version"])
     if not 'state' in local_state:
         # Fresh installation. Because this function is invoked by both
         # the install hook and the upgrade-charm hook, we need to guard
@@ -848,6 +894,10 @@
         run("pg_createcluster --locale='{}' --encoding='{}' 9.1 main".format(
             config_data['locale'], config_data['encoding']))
 
+    postgresql_backups_dir = (
+        config_data['backup_dir'].strip() or
+        os.path.join(postgresql_data_dir, 'backups'))
+
     host.mkdir(postgresql_backups_dir, owner="postgres", perms=0o755)
     host.mkdir(postgresql_scripts_dir, owner="postgres", perms=0o755)
     host.mkdir(postgresql_logs_dir, owner="postgres", perms=0o755)
@@ -857,10 +907,11 @@
         'scripts_dir': postgresql_scripts_dir,
         'logs_dir': postgresql_logs_dir,
     }
-    dump_script = Template(
-        open("templates/dump-pg-db.tmpl").read()).render(paths)
-    backup_job = Template(
-        open("templates/pg_backup_job.tmpl").read()).render(paths)
+    charm_dir = hookenv.charm_dir()
+    template_file = "{}/templates/dump-pg-db.tmpl".format(charm_dir)
+    dump_script = Template(open(template_file).read()).render(paths)
+    template_file = "{}/templates/pg_backup_job.tmpl".format(charm_dir)
+    backup_job = Template(open(template_file).read()).render(paths)
     host.write_file(
         '{}/dump-pg-db'.format(postgresql_scripts_dir),
         dump_script, perms=0755)
@@ -1176,6 +1227,7 @@
     log("Client relations {}".format(local_state['client_relations']))
     local_state.publish()
 
+    postgresql_hba = os.path.join(_get_postgresql_config_dir(), "pg_hba.conf")
     generate_postgresql_hba(postgresql_hba, user=user,
                             schema_user=schema_user,
                             database=database)
@@ -1213,6 +1265,7 @@
     log("Client relations {}".format(local_state['client_relations']))
     local_state.publish()
 
+    postgresql_hba = os.path.join(_get_postgresql_config_dir(), "pg_hba.conf")
     generate_postgresql_hba(postgresql_hba)
 
     snapshot_relations()
@@ -1245,6 +1298,7 @@
         run_sql_as_postgres(sql, AsIs(quote_identifier(database)),
                             AsIs(quote_identifier(user + "_schema")))
 
+    postgresql_hba = os.path.join(_get_postgresql_config_dir(), "pg_hba.conf")
     generate_postgresql_hba(postgresql_hba)
 
     # Cleanup our local state.
@@ -1261,13 +1315,14 @@
             sql = "ALTER USER %s NOSUPERUSER"
             run_sql_as_postgres(sql, AsIs(quote_identifier(user)))
 
+    postgresql_hba = os.path.join(_get_postgresql_config_dir(), "pg_hba.conf")
     generate_postgresql_hba(postgresql_hba)
 
     # Cleanup our local state.
     snapshot_relations()
 
 
-def update_repos_and_packages():
+def update_repos_and_packages(version):
     extra_repos = hookenv.config('extra_archives')
     extra_repos_added = local_state.setdefault('extra_repos_added', set())
     if extra_repos:
@@ -1284,10 +1339,12 @@
     # It might have been better for debversion and plpython to only get
     # installed if they were listed in the extra-packages config item,
     # but they predate this feature.
-    packages = ["postgresql-%s" % config_data["version"],
-                "postgresql-contrib-%s" % config_data["version"],
-                "postgresql-plpython-%s" % config_data["version"],
-                "postgresql-%s-debversion" % config_data["version"],
+    packages = ["python-psutil",  # to obtain system RAM from python
+                "libc-bin",       # for getconf
+                "postgresql-%s" % version,
+                "postgresql-contrib-%s" % version,
+                "postgresql-plpython-%s" % version,
+                "postgresql-%s-debversion" % version,
                 "python-jinja2", "syslinux", "python-psycopg2"]
     packages.extend((hookenv.config('extra-packages') or '').split())
     packages = fetch.filter_installed_packages(packages)
@@ -1351,6 +1408,11 @@
 
 def promote_database():
     '''Take the database out of recovery mode.'''
+    config_data = hookenv.config()
+    version = config_data['version']
+    cluster_name = config_data['cluster_name']
+    postgresql_cluster_dir = os.path.join(
+        postgresql_data_dir, version, cluster_name)
     recovery_conf = os.path.join(postgresql_cluster_dir, 'recovery.conf')
     if os.path.exists(recovery_conf):
         # Rather than using 'pg_ctl promote', we do the promotion
@@ -1554,6 +1616,8 @@
             del local_state['paused_at_failover']
 
         publish_hot_standby_credentials()
+        postgresql_hba = os.path.join(
+            _get_postgresql_config_dir(), "pg_hba.conf")
         generate_postgresql_hba(postgresql_hba)
 
     local_state.publish()
@@ -1706,7 +1770,7 @@
     lock.
     '''
     import psycopg2
-    key = long(config_data['advisory_lock_restart_key'])
+    key = long(hookenv.config('advisory_lock_restart_key'))
     if exclusive:
         lock_function = 'pg_advisory_lock'
     else:
@@ -1749,6 +1813,12 @@
         postgresql_stop()
         log("Cloning master {}".format(master_unit))
 
+        config_data = hookenv.config()
+        version = config_data['version']
+        cluster_name = config_data['cluster_name']
+        postgresql_cluster_dir = os.path.join(
+            postgresql_data_dir, version, cluster_name)
+        postgresql_config_dir = _get_postgresql_config_dir(config_data)
         cmd = [
             'sudo', '-E',  # -E needed to locate pgpass file.
             '-u', 'postgres', 'pg_basebackup', '-D', postgresql_cluster_dir,
@@ -1802,6 +1872,11 @@
 
 
 def postgresql_is_in_backup_mode():
+    version = hookenv.config('version')
+    cluster_name = hookenv.config('cluster_name')
+    postgresql_cluster_dir = os.path.join(
+        postgresql_data_dir, version, cluster_name)
+
     return os.path.exists(
         os.path.join(postgresql_cluster_dir, 'backup_label'))
 
@@ -1919,30 +1994,28 @@
         host.service_reload('nagios-nrpe-server')
 
 
+def _get_postgresql_config_dir(config_data=None):
+    """ Return the directory path of the postgresql configuration files. """
+    if config_data == None:
+        config_data = hookenv.config()
+    version = config_data['version']
+    cluster_name = config_data['cluster_name']
+    return os.path.join("/etc/postgresql", version, cluster_name)
+
 ###############################################################################
 # Global variables
 ###############################################################################
-config_data = hookenv.config()
-version = config_data['version']
-cluster_name = config_data['cluster_name']
 postgresql_data_dir = "/var/lib/postgresql"
-postgresql_cluster_dir = os.path.join(
-    postgresql_data_dir, version, cluster_name)
-postgresql_bin_dir = os.path.join('/usr/lib/postgresql', version, 'bin')
-postgresql_config_dir = os.path.join("/etc/postgresql", version, cluster_name)
-postgresql_config = os.path.join(postgresql_config_dir, "postgresql.conf")
-postgresql_ident = os.path.join(postgresql_config_dir, "pg_ident.conf")
-postgresql_hba = os.path.join(postgresql_config_dir, "pg_hba.conf")
+postgresql_scripts_dir = os.path.join(postgresql_data_dir, 'scripts')
+postgresql_logs_dir = os.path.join(postgresql_data_dir, 'logs')
+
+postgresql_sysctl = "/etc/sysctl.d/50-postgresql.conf"
 postgresql_crontab = "/etc/cron.d/postgresql"
 postgresql_service_config_dir = "/var/run/postgresql"
-postgresql_scripts_dir = os.path.join(postgresql_data_dir, 'scripts')
-postgresql_backups_dir = (
-    config_data['backup_dir'].strip() or
-    os.path.join(postgresql_data_dir, 'backups'))
-postgresql_logs_dir = os.path.join(postgresql_data_dir, 'logs')
-hook_name = os.path.basename(sys.argv[0])
 replication_relation_types = ['master', 'slave', 'replication']
 local_state = State('local_state.pickle')
+hook_name = os.path.basename(sys.argv[0])
+juju_log_dir = "/var/log/juju"
 
 
 if __name__ == '__main__':
 
=== added file 'hooks/test_hooks.py'
--- hooks/test_hooks.py	1970-01-01 00:00:00 +0000
+++ hooks/test_hooks.py	2014-01-07 21:59:36 +0000
@@ -0,0 +1,354 @@
+import mocker
+import hooks
+
+
+class TestJujuHost(object):
+    """
+    Testing object to intercept charmhelper calls and inject data, or make sure
+    certain data is set.
+    """
+    def write_file(self, file_path, contents, owner=None, group=None,
+                   perms=None):
+        """
+        Only write the file as requested. owner, group and perms untested.
+        """
+        with open(file_path, 'w') as target:
+            target.write(contents)
+
+    def mkdir(self, dir_path, owner, group, perms):
+        """Not yet tested"""
+        pass
+
+    def service_start(self, service_name):
+        """Not yet tested"""
+        pass
+
+    def service_reload(self, service_name):
+        """Not yet tested"""
+        pass
+
+    def service_pwgen(self, service_name):
+        """Not yet tested"""
+        return ""
+
+    def service_stop(self, service_name):
+        """Not yet tested"""
+        pass
+
+
+class TestJuju(object):
+    """
+    Testing object to intercept juju calls and inject data, or make sure
+    certain data is set.
+    """
+
+    _relation_data = {}
+    _relation_ids = {}
+    _relation_list = ("postgres/0",)
+
+    def __init__(self):
+        self._config = {
+            "admin_addresses": "",
+            "locale": "C",
+            "encoding": "UTF-8",
+            "extra_packages": "",
+            "dumpfile_location": "None",
+            "config_change_command": "reload",
+            "version": "9.1",
+            "cluster_name": "main",
+            "listen_ip": "*",
+            "listen_port": "5432",
+            "max_connections": "100",
+            "ssl": "True",
+            "log_min_duration_statement": -1,
+            "log_checkpoints": False,
+            "log_connections": False,
+            "log_disconnections": False,
+            "log_line_prefix": "%t ",
+            "log_lock_waits": False,
+            "log_timezone": "UTC",
+            "autovacuum": True,
+            "log_autovacuum_min_duration": -1,
+            "autovacuum_analyze_threshold": 50,
+            "autovacuum_vacuum_scale_factor": 0.2,
+            "autovacuum_analyze_scale_factor": 0.1,
+            "autovacuum_vacuum_cost_delay": "20ms",
+            "search_path": "\"$user\",public",
+            "standard_conforming_strings": True,
+            "hot_standby": False,
+            "hot_standby_feedback": False,
+            "wal_level": "minimal",
+            "max_wal_senders": 0,
+            "wal_keep_segments": 0,
+            "replicated_wal_keep_segments": 5000,
+            "archive_mode": False,
+            "archive_command": "",
+            "work_mem": "1MB",
+            "maintenance_work_mem": "1MB",
+            "performance_tuning": "auto",
+            "kernel_shmall": 0,
+            "kernel_shmmax": 0,
+            "shared_buffers": "",
+            "effective_cache_size": "",
+            "temp_buffers": "1MB",
+            "wal_buffers": "-1",
+            "checkpoint_segments": 3,
+            "random_page_cost": 4.0,
+            "volume_ephemeral_storage": True,
+            "volume_map": "",
+            "volume_dev_regexp": "/dev/db[b-z]",
+            "backup_dir": "/var/lib/postgresql/backups",
+            "backup_schedule": "13 4 * * *",
+            "backup_retention_count": 7,
+            "nagios_context": "juju",
+            "extra_archives": "",
+            "advisory_lock_restart_key": 765}
+
+    def relation_set(self, *args, **kwargs):
+        """
+        Capture result of relation_set into _relation_data, which
+        can then be checked later.
+        """
+        if "relation_id" in kwargs:
+            del kwargs["relation_id"]
+        self._relation_data = dict(self._relation_data, **kwargs)
+        for arg in args:
+            (key, value) = arg.split("=")
+            self._relation_data[key] = value
+
+    def relation_ids(self, relation_name="db-admin"):
+        """
+        Return expected relation_ids for tests.  Feel free to expand
+        as more tests are added.
+        """
+        return [self._relation_ids[name] for name in self._relation_ids.keys()
+                if name.find(relation_name) == 0]
+
+    def related_units(self, relid="db-admin:5"):
+        """
+        Return expected relation_ids for tests.  Feel free to expand
+        as more tests are added.
+        """
+        return [name for name, value in self._relation_ids.iteritems()
+                if value == relid]
+
+    def relation_list(self):
+        """
+        Hardcode expected relation_list for tests.  Feel free to expand
+        as more tests are added.
+        """
+        return list(self._relation_list)
+
+    def unit_get(self, *args):
+        """
+        for now the only thing this is called for is "public-address",
+        so it's a simplistic return.
+        """
+        return "localhost"
+
+    def local_unit(self):
+        return hooks.os.environ["JUJU_UNIT_NAME"]
+
+    def charm_dir(self):
+        return hooks.os.environ["CHARM_DIR"]
+
+    def juju_log(self, *args, **kwargs):
+        pass
+
+    def log(self, *args, **kwargs):
+        pass
+
+    def config_get(self, scope=None):
+        if scope is None:
+            return self.config
+        else:
+            return self.config[scope]
+
+    def relation_get(self, scope=None, unit_name=None, relation_id=None):
+        pass
+
+
+class TestHooks(mocker.MockerTestCase):
+
+    def setUp(self):
+        hooks.hookenv = TestJuju()
+        hooks.host = TestJujuHost()
+        hooks.juju_log_dir = self.makeDir()
+        hooks.hookenv.config = lambda: hooks.hookenv._config
+        #hooks.hookenv.localunit = lambda: "localhost"
+        hooks.os.environ["JUJU_UNIT_NAME"] = "landscape/1"
+        hooks.postgresql_sysctl = self.makeFile()
+        hooks._get_system_ram = lambda: 1024   # MB
+        hooks._get_page_size = lambda: 1024 * 1024  # bytes
+        self.maxDiff = None
+
+    def assertFileContains(self, filename, lines):
+        """Make sure strings exist in a file."""
+        with open(filename, "r") as fp:
+            contents = fp.read()
+        for line in lines:
+            self.assertIn(line, contents)
+
+    def assertNotFileContains(self, filename, lines):
+        """Make sure strings do not exist in a file."""
+        with open(filename, "r") as fp:
+            contents = fp.read()
+        for line in lines:
+            self.assertNotIn(line, contents)
+
+    def assertFilesEqual(self, file1, file2):
+        """Given two filenames, compare them."""
+        with open(file1, "r") as fp1:
+            contents1 = fp1.read()
+        with open(file2, "r") as fp2:
+            contents2 = fp2.read()
+        self.assertEqual(contents1, contents2)
+
+
+class TestHooksService(TestHooks):
+
+    def test_create_postgresql_config_wal_no_replication(self):
+        """
+        When postgresql is in C{standalone} mode, and participates in no
+        C{replication} relations, default wal settings will be present.
+        """
+        config_outfile = self.makeFile()
+        run = self.mocker.replace(hooks.run)
+        run("sysctl -p %s" % hooks.postgresql_sysctl)
+        self.mocker.result(True)
+        self.mocker.replay()
+        hooks.create_postgresql_config(config_outfile)
+        self.assertFileContains(
+            config_outfile,
+            ["wal_buffers = -1", "wal_level = minimal", "max_wal_senders = 0",
+             "wal_keep_segments = 0"])
+
+    def test_create_postgresql_config_wal_with_replication(self):
+        """
+        When postgresql is in C{replicated} mode, and participates in a
+        C{replication} relation, C{hot_standby} will be set to C{on},
+        C{wal_level} will be enabled as C{hot_standby} and the
+        C{max_wall_senders} will match the count of replication relations.
+        The value of C{wal_keep_segments} will be the maximum of the configured
+        C{wal_keep_segments} and C{replicated_wal_keep_segments}.
+        """
+        self.addCleanup(
+            setattr, hooks.hookenv, "_relation_ids", {})
+        hooks.hookenv._relation_ids = {
+            "replication/0": "db-admin:5", "replication/1": "db-admin:6"}
+        config_outfile = self.makeFile()
+        run = self.mocker.replace(hooks.run)
+        run("sysctl -p %s" % hooks.postgresql_sysctl)
+        self.mocker.result(True)
+        self.mocker.replay()
+        hooks.create_postgresql_config(config_outfile)
+        self.assertFileContains(
+            config_outfile,
+            ["hot_standby = on", "wal_buffers = -1", "wal_level = hot_standby",
+             "max_wal_senders = 2", "wal_keep_segments = 5000"])
+
+    def test_create_postgresql_config_wal_with_replication_max_override(self):
+        """
+        When postgresql is in C{replicated} mode, and participates in a
+        C{replication} relation, C{hot_standby} will be set to C{on},
+        C{wal_level} will be enabled as C{hot_standby}. The written value for
+        C{max_wal_senders} will be the maximum of replication slave count and
+        the configuration value for C{max_wal_senders}.
+        The written value of C{wal_keep_segments} will be
+        the maximum of the configuration C{wal_keep_segments} and
+        C{replicated_wal_keep_segments}.
+        """
+        self.addCleanup(
+            setattr, hooks.hookenv, "_relation_ids", ())
+        hooks.hookenv._relation_ids = {
+            "replication/0": "db-admin:5", "replication/1": "db-admin:6"}
+        hooks.hookenv._config["max_wal_senders"] = "3"
+        hooks.hookenv._config["wal_keep_segments"] = 1000
+        hooks.hookenv._config["replicated_wal_keep_segments"] = 999
+        config_outfile = self.makeFile()
+        run = self.mocker.replace(hooks.run)
+        run("sysctl -p %s" % hooks.postgresql_sysctl)
+        self.mocker.result(True)
+        self.mocker.replay()
+        hooks.create_postgresql_config(config_outfile)
+        self.assertFileContains(
+            config_outfile,
+            ["hot_standby = on", "wal_buffers = -1", "wal_level = hot_standby",
+             "max_wal_senders = 3", "wal_keep_segments = 1000"])
+
+    def test_create_postgresql_config_performance_tune_auto_large_ram(self):
+        """
+        When configuration attribute C{performance_tune} is set to C{auto} and
+        total RAM on a system is > 1023MB. It will automatically calculate
+        values for the following attributes if these attributes were left as
+        default values:
+           - C{effective_cache_size} set to 75% of total RAM in MegaBytes
+           - C{shared_buffers} set to 25% of total RAM in MegaBytes
+           - C{kernel_shmmax} set to total RAM in bytes
+           - C{kernel_shmall} equal to kernel_shmmax in pages
+        """
+        config_outfile = self.makeFile()
+        run = self.mocker.replace(hooks.run)
+        run("sysctl -p %s" % hooks.postgresql_sysctl)
+        self.mocker.result(True)
+        self.mocker.replay()
+        hooks.create_postgresql_config(config_outfile)
+        self.assertFileContains(
+            config_outfile,
+            ["shared_buffers = 256MB", "effective_cache_size = 768MB"])
+        self.assertFileContains(
+            hooks.postgresql_sysctl,
+            ["kernel.shmall = 1025\nkernel.shmmax = 1073742848"])
+
+    def test_create_postgresql_config_performance_tune_auto_small_ram(self):
+        """
+        When configuration attribute C{performance_tune} is set to C{auto} and
+        total RAM on a system is <= 1023MB. It will automatically calculate
+        values for the following attributes if these attributes were left as
+        default values:
+           - C{effective_cache_size} set to 75% of total RAM in MegaBytes
+           - C{shared_buffers} set to 15% of total RAM in MegaBytes
+           - C{kernel_shmmax} set to total RAM in bytes
+           - C{kernel_shmall} equal to kernel_shmmax in pages
+        """
+        hooks._get_system_ram = lambda: 1023   # MB
+        config_outfile = self.makeFile()
+        run = self.mocker.replace(hooks.run)
+        run("sysctl -p %s" % hooks.postgresql_sysctl)
+        self.mocker.result(True)
+        self.mocker.replay()
+        hooks.create_postgresql_config(config_outfile)
+        self.assertFileContains(
+            config_outfile,
+            ["shared_buffers = 153MB", "effective_cache_size = 767MB"])
+        self.assertFileContains(
+            hooks.postgresql_sysctl,
+            ["kernel.shmall = 1024\nkernel.shmmax = 1072694272"])
+
+    def test_create_postgresql_config_performance_tune_auto_overridden(self):
+        """
+        When configuration attribute C{performance_tune} is set to C{auto} any
+        non-default values for the configuration parameters below will be used
+        instead of the automatically calculated values.
+           - C{effective_cache_size}
+           - C{shared_buffers}
+           - C{kernel_shmmax}
+           - C{kernel_shmall}
+        """
+        hooks.hookenv._config["effective_cache_size"] = "999MB"
+        hooks.hookenv._config["shared_buffers"] = "101MB"
+        hooks.hookenv._config["kernel_shmmax"] = 50000
+        hooks.hookenv._config["kernel_shmall"] = 500
+        hooks._get_system_ram = lambda: 1023   # MB
+        config_outfile = self.makeFile()
+        run = self.mocker.replace(hooks.run)
+        run("sysctl -p %s" % hooks.postgresql_sysctl)
+        self.mocker.result(True)
+        self.mocker.replay()
+        hooks.create_postgresql_config(config_outfile)
+        self.assertFileContains(
+            config_outfile,
+            ["shared_buffers = 101MB", "effective_cache_size = 999MB"])
+        self.assertFileContains(
+            hooks.postgresql_sysctl,
+            ["kernel.shmall = 1024\nkernel.shmmax = 1072694272"])
 
=== added file 'revision'
--- revision	1970-01-01 00:00:00 +0000
+++ revision	2014-01-07 21:59:36 +0000
@@ -0,0 +1,1 @@
+2
 
=== modified file 'templates/postgresql.conf.tmpl'
--- templates/postgresql.conf.tmpl	2013-01-24 11:28:39 +0000
+++ templates/postgresql.conf.tmpl	2014-01-07 21:59:36 +0000
@@ -40,7 +40,7 @@
 {% if shared_buffers != "" -%}
 shared_buffers = {{shared_buffers}}
 {% endif -%}
-{% if temp_buffers!= "" -%}
+{% if temp_buffers != "" -%}
 temp_buffers = {{temp_buffers}}
 {% endif -%}
 {% if work_mem != "" -%}