Hi
Consider the following cascading standby setup with PostgreSQL 12:
- there exists a running primary "A"
- standby "B" is cloned from primary "A" using "pg_basebackup
--write-recovery-conf"
- standby "C" is cloned from standby "B" using "pg_basebackup
--write-recovery-conf"
So far, so good, everything works as expected.
Now, for whatever reason, the user wishes standby "C" to follow another upstream
node (which one is not important here), so the user, in the comfort of their
own psql
command line (no more pesky recovery.conf editing!) issues the following:
ALTER SYSTEM SET primary_conninfo = 'host=someothernode';
and restarts the instance, and... it stays connected to the original upstream
node.
Which is unexpected.
Examining the the restarted instance, "SHOW primary_conninfo" still displays
the original value for "primary_conninfo". Mysteriouser and mysteriouser.
What has happened here is that with the option "--write-recovery-conf", Pg12's
pg_basebackup (correctly IMHO) appends the recovery settings to
"postgresql.auto.conf".
However, on standby "C", pg_basebackup has dutifully copied over standby "B"'s
existing "postgresql.auto.conf", which already contains standby "B"'s
replication configuration, and appended standby "C"'s replication configuration
to that, which (before "ALTER SYSTEM" was invoked) looked something like this:
# Do not edit this file manually!
# It will be overwritten by the ALTER SYSTEM command.
primary_conninfo = 'host=node_A'
primary_conninfo = 'host=node_B'
which is expected, and works because the last entry in the file is evaluated, so
on startup, standby "C" follows standby "B".
However, executing "ALTER SYSTEM SET primary_conninfo = 'host=someothernode'"
left
standby "C"'s "postgresql.auto.conf" file looking like this:
# Do not edit this file manually!
# It will be overwritten by the ALTER SYSTEM command.
primary_conninfo = 'host=someothernode'
primary_conninfo = 'host=node_B'
which seems somewhat broken, to say the least.
As-is, the user will either need to repeatedly issue "ALTER SYSTEM RESET
primary_conninfo"
until the duplicates are cleared (which means "ALTER SYSTEM RESET ..." doesn't
work as
advertised, and is not an obvious solution anyway), or ignore the "Do not edit this
file manually!"
warning and remove the offending entry/entries (which, if done safely, should
involve
shutting down the instance first).
Note this issue is not specific to pg_basebackup, primary_conninfo (or any
other settings
formerly in recovery.conf), it has just manifested itself as the built-in
toolset as of now
provides a handy way of getting into this situation without too much effort
(and any
utilities which clone standbys and append the replication configuration to
"postgresql.auto.conf" in lieu of creating "recovery.conf" will be prone to
running into
the same situation).
I had previously always assumed that ALTER SYSTEM would change the *last*
occurrence for
the parameter in "postgresql.auto.conf", in the same way you'd need to be sure
to change
the last occurrence in the normal configuration files, however this actually
not the case -
as per replace_auto_config_value() ( src/backend/utils/misc/guc.c ):
/* Search the list for an existing match (we assume there's only one) */
the *first* match is replaced.
Attached patch attempts to rectify this situation by having
replace_auto_config_value()
deleting any duplicate entries first, before making any changes to the last
entry.
Arguably it might be sufficient (and simpler) to just scan the list for the last
entry, but removing preceding duplicates seems cleaner, as it's pointless
(and a potential source of confusion) keeping entries around which will never
be used.
Also attached is a set of TAP tests to check ALTER SYSTEM works as expected (or
at least as seems correct to me).
Regards
Ian Barwick
--
Ian Barwick https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
new file mode 100644
index 1208eb9..5287004
*** a/src/backend/utils/misc/guc.c
--- b/src/backend/utils/misc/guc.c
*************** write_auto_conf_file(int fd, const char
*** 7845,7894 ****
/*
* Update the given list of configuration parameters, adding, replacing
* or deleting the entry for item "name" (delete if "value" == NULL).
*/
static void
replace_auto_config_value(ConfigVariable **head_p, ConfigVariable **tail_p,
const char *name, const char *value)
{
! ConfigVariable *item,
! *prev = NULL;
! /* Search the list for an existing match (we assume there's only one) */
! for (item = *head_p; item != NULL; item = item->next)
{
! if (strcmp(item->name, name) == 0)
{
! /* found a match, replace it */
! pfree(item->value);
! if (value != NULL)
{
! /* update the parameter value */
! item->value = pstrdup(value);
}
else
{
! /* delete the configuration parameter from list */
! if (*head_p == item)
! *head_p = item->next;
! else
! prev->next = item->next;
! if (*tail_p == item)
! *tail_p = prev;
!
! pfree(item->name);
! pfree(item->filename);
! pfree(item);
}
! return;
! }
! prev = item;
}
/* Not there; no work if we're trying to delete it */
if (value == NULL)
return;
! /* OK, append a new entry */
item = palloc(sizeof *item);
item->name = pstrdup(name);
item->value = pstrdup(value);
--- 7845,7927 ----
/*
* Update the given list of configuration parameters, adding, replacing
* or deleting the entry for item "name" (delete if "value" == NULL).
+ * If more than entry for item "name" is found, all entries except the
+ * last one will be deleted.
*/
static void
replace_auto_config_value(ConfigVariable **head_p, ConfigVariable **tail_p,
const char *name, const char *value)
{
! ConfigVariable *item = NULL, *item_iter;
! /*
! * Find the last match, so if there's more than one match we can
! * delete any preceding matches.
! */
! for (item_iter = *head_p; item_iter != NULL; item_iter = item_iter->next)
{
! if (strcmp(item_iter->name, name) == 0)
{
! item = item_iter;
! }
! }
!
! /*
! * At least one match was found, update or delete that as necessary,
! * and remove any preceding duplicates. We'll return from this loop
! * when the last match is processed.
! */
! if (item != NULL)
! {
! ConfigVariable *prev = NULL;
! item_iter = *head_p;
!
! do
! {
! ConfigVariable *item_next = item_iter->next;
!
! if (strcmp(item_iter->name, name) == 0)
{
! pfree(item_iter->value);
!
! if (item_iter == item && value != NULL)
! {
! /* Update the parameter value. */
! item->value = pstrdup(value);
! }
! else if (item_iter != item || (item_iter == item && value == NULL))
! {
! /* Delete a preceding duplicate, or the item itself if required. */
! if (*head_p == item_iter)
! *head_p = item_iter->next;
! else
! prev->next = item_iter->next;
! if (*tail_p == item_iter)
! *tail_p = prev;
!
! pfree(item_iter->name);
! pfree(item_iter->filename);
! pfree(item_iter);
! }
!
! /* We've processed the last item, nothing else to do. */
! if (item_iter == item)
! return;
}
else
{
! prev = item_iter;
}
!
! item_iter = item_next;
! } while (item_iter != NULL);
}
/* Not there; no work if we're trying to delete it */
if (value == NULL)
return;
! /* No existing item found, append a new entry */
item = palloc(sizeof *item);
item->name = pstrdup(name);
item->value = pstrdup(value);
diff --git a/src/test/Makefile b/src/test/Makefile
new file mode 100644
index efb206a..441d954
*** a/src/test/Makefile
--- b/src/test/Makefile
*************** subdir = src/test
*** 12,18 ****
top_builddir = ../..
include $(top_builddir)/src/Makefile.global
! SUBDIRS = perl regress isolation modules authentication recovery subscription
# Test suites that are not safe by default but can be run if selected
# by the user via the whitespace-separated list in variable
--- 12,19 ----
top_builddir = ../..
include $(top_builddir)/src/Makefile.global
! SUBDIRS = perl regress isolation modules authentication recovery subscription \
! configuration
# Test suites that are not safe by default but can be run if selected
# by the user via the whitespace-separated list in variable
diff --git a/src/test/configuration/.gitignore b/src/test/configuration/.gitignore
new file mode 100644
index ...871e943
*** a/src/test/configuration/.gitignore
--- b/src/test/configuration/.gitignore
***************
*** 0 ****
--- 1,2 ----
+ # Generated by test suite
+ /tmp_check/
diff --git a/src/test/configuration/Makefile b/src/test/configuration/Makefile
new file mode 100644
index ...69e9697
*** a/src/test/configuration/Makefile
--- b/src/test/configuration/Makefile
***************
*** 0 ****
--- 1,25 ----
+ #-------------------------------------------------------------------------
+ #
+ # Makefile for src/test/configuration
+ #
+ # Portions Copyright (c) 1996-2019, PostgreSQL Global Development Group
+ # Portions Copyright (c) 1994, Regents of the University of California
+ #
+ # src/test/configuration/Makefile
+ #
+ #-------------------------------------------------------------------------
+
+ EXTRA_INSTALL=contrib/test_decoding
+
+ subdir = src/test/recovery
+ top_builddir = ../../..
+ include $(top_builddir)/src/Makefile.global
+
+ check:
+ $(prove_check)
+
+ installcheck:
+ $(prove_installcheck)
+
+ clean distclean maintainer-clean:
+ rm -rf tmp_check
diff --git a/src/test/configuration/README b/src/test/configuration/README
new file mode 100644
index ...1796f5c
*** a/src/test/configuration/README
--- b/src/test/configuration/README
***************
*** 0 ****
--- 1,25 ----
+ src/test/configuration/README
+
+ Regression tests for configuration files
+ ========================================
+
+ This directory contains a test suite for configuration files.
+
+ Running the tests
+ =================
+
+ NOTE: You must have given the --enable-tap-tests argument to configure.
+ Also, to use "make installcheck", you must have built and installed
+ contrib/test_decoding in addition to the core code.
+
+ Run
+ make check
+ or
+ make installcheck
+ You can use "make installcheck" if you previously did "make install".
+ In that case, the code in the installation tree is tested. With
+ "make check", a temporary installation tree is built from the current
+ sources and then tested.
+
+ Either way, this test initializes, starts, and stops several test Postgres
+ clusters.
diff --git a/src/test/configuration/t/001_alter_system.pl b/src/test/configuration/t/001_alter_system.pl
new file mode 100644
index ...f0975e7
*** a/src/test/configuration/t/001_alter_system.pl
--- b/src/test/configuration/t/001_alter_system.pl
***************
*** 0 ****
--- 1,140 ----
+ # Test that ALTER SYSTEM updates postgresql.auto.conf as expected
+
+ use strict;
+ use warnings;
+ use PostgresNode;
+ use TestLib;
+ use Test::More tests => 8;
+
+ our $postgresql_auto_conf = 'postgresql.auto.conf';
+
+ my ($expected, $result, $auto_conf, $vacuum_cost_delay_value);
+
+ # Initialize a single node
+
+ my $node = get_new_node('primary');
+ $node->init();
+ $node->start;
+
+ # 1. Check default postgresql.auto.conf contents
+ # ----------------------------------------------
+
+ my $expected_default = <<'EO_CONF';
+ # Do not edit this file manually!
+ # It will be overwritten by the ALTER SYSTEM command.
+ EO_CONF
+
+ $auto_conf = $node->read_conf($postgresql_auto_conf);
+
+ is($auto_conf, $expected_default, 'check default contents of postgresql.auto.conf');
+
+ # 2. Check ALTER SYSTEM SET foo = 'bar'
+ # --------------------------------------
+ #
+ # Verify postgresql.auto.conf updated as expected
+
+ $vacuum_cost_delay_value = '1ms';
+
+ $node->safe_psql('postgres',
+ "ALTER SYSTEM SET vacuum_cost_delay = '$vacuum_cost_delay_value'");
+
+ $expected = sprintf(
+ <<'EO_CONF',
+ # Do not edit this file manually!
+ # It will be overwritten by the ALTER SYSTEM command.
+ vacuum_cost_delay = '%s'
+ EO_CONF
+ $vacuum_cost_delay_value
+ );
+
+ $auto_conf = $node->read_conf($postgresql_auto_conf);
+
+ is($auto_conf, $expected, "check ALTER SYSTEM SET foo = 'bar'");
+
+ # 3. Check changed value is registered
+ # ------------------------------------
+
+ $node->safe_psql('postgres',
+ "SELECT pg_catalog.pg_reload_conf()");
+
+ $result = $node->safe_psql('postgres',
+ "SELECT setting FROM pg_catalog.pg_settings WHERE name = 'vacuum_cost_delay'");
+
+ is($result, '1', 'Check changed value is registered');
+
+ # 4. Check ALTER SYSTEM RESET foo
+ # -------------------------------
+ #
+ # postgresql.auto.conf should be reset to the default
+
+ $node->safe_psql('postgres',
+ "ALTER SYSTEM RESET vacuum_cost_delay");
+
+ $auto_conf = $node->read_conf($postgresql_auto_conf);
+
+ is($auto_conf, $expected_default, "check ALTER SYSTEM RESET foo");
+
+ # 5. Check behaviour with duplicate values
+ # ----------------------------------------
+
+ my $duplicate_values = <<'EO_CONF';
+ vacuum_cost_delay = '2ms'
+ vacuum_cost_page_hit = '2'
+ vacuum_cost_delay = '1ms'
+ vacuum_cost_delay = '0ms'
+ EO_CONF
+
+ $node->safe_psql('postgres',
+ "SELECT pg_catalog.pg_reload_conf()");
+
+ $node->append_conf($postgresql_auto_conf, $duplicate_values);
+
+ $node->safe_psql('postgres',
+ "SELECT pg_catalog.pg_reload_conf()");
+
+ $result = $node->safe_psql('postgres',
+ "SELECT setting FROM pg_catalog.pg_settings WHERE name = 'vacuum_cost_delay'");
+
+ is($result, '0', 'If duplicates present, check last value present is registered');
+
+ # 6. Check ALTER SYSTEM works as expected when duplicate values present
+ # ---------------------------------------------------------------------
+
+ $vacuum_cost_delay_value = '3ms';
+
+ $node->safe_psql('postgres',
+ "ALTER SYSTEM SET vacuum_cost_delay = '$vacuum_cost_delay_value'");
+
+ $node->safe_psql('postgres',
+ "SELECT pg_catalog.pg_reload_conf()");
+
+ $result = $node->safe_psql('postgres',
+ "SELECT setting FROM pg_catalog.pg_settings WHERE name = 'vacuum_cost_delay'");
+
+ is($result, '3', 'If duplicates present, check ALTER SYSTEM SET works as expected');
+
+ # 7. Check ALTER SYSTEM RESET works as expected when duplicate values present
+ # ---------------------------------------------------------------------------
+
+ $expected = <<'EO_CONF';
+ # Do not edit this file manually!
+ # It will be overwritten by the ALTER SYSTEM command.
+ vacuum_cost_page_hit = '2'
+ EO_CONF
+
+ $node->safe_psql('postgres',
+ "ALTER SYSTEM RESET vacuum_cost_delay");
+
+ $auto_conf = $node->read_conf($postgresql_auto_conf);
+
+ is($auto_conf, $expected, "check ALTER SYSTEM RESET works when duplicate values present");
+
+ # 8. Check ALTER SYSTEM RESET ALL works as expected
+ # -------------------------------------------------
+
+ $node->safe_psql('postgres',
+ "ALTER SYSTEM RESET ALL");
+
+ $auto_conf = $node->read_conf($postgresql_auto_conf);
+
+ is($auto_conf, $expected_default, "check ALTER SYSTEM RESET ALL works as expected");
diff --git a/src/test/perl/PostgresNode.pm b/src/test/perl/PostgresNode.pm
new file mode 100644
index 8d5ad6b..0d3d3f4
*** a/src/test/perl/PostgresNode.pm
--- b/src/test/perl/PostgresNode.pm
*************** sub append_conf
*** 538,543 ****
--- 538,562 ----
=pod
+ =item $node->read_conf(filename)
+
+ A shortcut method to read the contents of a configuration file.
+
+ Does no validation, parsing or sanity checking.
+
+ =cut
+
+ sub read_conf
+ {
+ my ($self, $filename, $str) = @_;
+
+ my $conffile = $self->data_dir . '/' . $filename;
+
+ return TestLib::slurp_file($conffile);
+ }
+
+ =pod
+
=item $node->backup(backup_name)
Create a hot backup with B<pg_basebackup> in subdirectory B<backup_name> of
