QA work following the docs quickstart (#198)

* Don't error out when PGPORT does not exists in the environment.

We want to use the environment value as a fallback when the user is not
providing us with --pgport, but we don't require the value to be there.

* Forward the absolute PGDATA change.

We need to change to be reflected so that the postgresql configuration file
we prepare contains the absolute path to the OpenSSL self-signed
certificate, when we create one.

* Review the FOPEN_FLAGS_A to include O_RDWR.

Otherwise we get an error to create the startup.log file with the initial
logs from starting Postgres.

* Fix pg_autoctl show uri on the monitor.

* Quickstart review with the new code.

In particular we need to be careful about using --auth and --ssl-self-signed
options when creating the monitor and every other node.

* Adding compat write open mode O_TRUNC.

docs/quickstart.rst

@@ -52,6 +52,7 @@ distinctive port (6000):
 
    pg_autoctl create monitor   \
      --auth trust              \
+     --ssl-self-signed         \
      --pgdata ./monitor        \
      --pgport 6000
 
@@ -90,10 +91,11 @@ we’ll store the primary node’s data files in a small temporary filesystem.
    # initialize on that disk
    pg_autoctl create postgres         \
      --auth trust                     \
+     --ssl-self-signed                \
      --pgdata /mnt/node_a/data        \
      --pgport 6010                    \
      --pgctl /usr/pgsql-11/bin/pg_ctl \
-     --monitor postgres://autoctl_node@127.0.0.1:6000/pg_auto_failover
+     --monitor postgres://autoctl_node@127.0.0.1:6000/pg_auto_failover?sslmode=require
 
 It creates the database in "/mnt/node_a/data" using the port and node
 specified. Notice the user and database name in the monitor connection
@@ -123,10 +125,11 @@ created the primary:
 
    pg_autoctl create postgres         \
      --auth trust                     \
+     --ssl-self-signed                \
      --pgdata ./node_b                \
      --pgport 6011                    \
      --pgctl /usr/pgsql-11/bin/pg_ctl \
-     --monitor postgres://autoctl_node@127.0.0.1:6000/pg_auto_failover
+     --monitor postgres://autoctl_node@127.0.0.1:6000/pg_auto_failover?sslmode=require
 
    pg_autoctl run --pgdata ./node_b
 
@@ -215,7 +218,7 @@ To discover the url to use in our case, the following command can be used:
 
 .. code-block:: bash
 
-   pg_autoctl show uri --formation default --pgdata ./monitor
+   pg_autoctl show uri --pgdata ./monitor
    
 
             Type |    Name | Connection String
@@ -229,7 +232,7 @@ writes:
 .. code-block:: bash
 
    psql \
-     'postgres://127.0.0.1:6010,127.0.0.1:6011/?target_session_attrs=read-write'
+     'postgres://localhost:6010,localhost:6011/postgres?target_session_attrs=read-write&sslmode=require'
 
 When nodes A and B were both running, psql would connect to node A
 because B would be read-only. However now that A is offline and B is

src/bin/pg_autoctl/cli_show.c

@@ -495,23 +495,73 @@ cli_show_uri(int argc, char **argv)
 static void
 cli_show_formation_uri(int argc, char **argv)
 {
-	KeeperConfig config = keeperOptions;
-	Monitor monitor = { 0 };
-	bool monitorDisabledIsOk = false;
+	KeeperConfig kconfig = keeperOptions;
 
-	if (!keeper_config_read_file_skip_pgsetup(&config, monitorDisabledIsOk))
+	switch (ProbeConfigurationFileRole(kconfig.pathnames.config))
 	{
-		/* errors have already been logged */
-		exit(EXIT_CODE_BAD_CONFIG);
-	}
+		case PG_AUTOCTL_ROLE_MONITOR:
+		{
+			Monitor monitor = { 0 };
+			MonitorConfig mconfig = { 0 };
 
-	if (!monitor_init(&monitor, config.monitor_pguri))
-	{
-		/* errors have already been logged */
-		exit(EXIT_CODE_BAD_CONFIG);
-	}
+			bool missingPgdataIsOk = true;
+			bool pgIsNotRunningIsOk = true;
+			char connInfo[MAXCONNINFO];
 
-	(void) print_monitor_and_formation_uri(&config, &monitor, stdout);
+			if (!monitor_config_init_from_pgsetup(&mconfig,
+												  &kconfig.pgSetup,
+												  missingPgdataIsOk,
+												  pgIsNotRunningIsOk))
+			{
+				/* errors have already been logged */
+				exit(EXIT_CODE_PGCTL);
+			}
+
+			if (!monitor_config_get_postgres_uri(&mconfig, connInfo, MAXCONNINFO))
+			{
+				/* errors have already been logged */
+				exit(EXIT_CODE_BAD_CONFIG);
+			}
+
+			if (!monitor_init(&monitor, connInfo))
+			{
+				/* errors have already been logged */
+				exit(EXIT_CODE_BAD_CONFIG);
+			}
+
+			(void) print_monitor_and_formation_uri(&kconfig, &monitor, stdout);
+			break;
+		}
+
+		case PG_AUTOCTL_ROLE_KEEPER:
+		{
+			Monitor monitor = { 0 };
+			bool monitorDisabledIsOk = false;
+
+			if (!keeper_config_read_file_skip_pgsetup(&kconfig,
+													  monitorDisabledIsOk))
+			{
+				/* errors have already been logged */
+				exit(EXIT_CODE_BAD_CONFIG);
+			}
+
+			if (!monitor_init(&monitor, kconfig.monitor_pguri))
+			{
+				/* errors have already been logged */
+				exit(EXIT_CODE_BAD_CONFIG);
+			}
+
+			(void) print_monitor_and_formation_uri(&kconfig, &monitor, stdout);
+			break;
+		}
+
+		default:
+		{
+			log_fatal("Unrecognized configuration file \"%s\"",
+					  kconfig.pathnames.config);
+			exit(EXIT_CODE_INTERNAL_ERROR);
+		}
+	}
 }
 
 

src/bin/pg_autoctl/defaults.h

@@ -115,10 +115,10 @@
 /*
  * This opens file write only and creates if it doesn't exist.
  */
-#define FOPEN_FLAGS_W O_WRONLY | O_CREAT
+#define FOPEN_FLAGS_W O_WRONLY | O_TRUNC | O_CREAT
 /*
  * This opens the file in append mode and creates it if it doesn't exist.
  */
-#define FOPEN_FLAGS_A O_APPEND | O_CREAT
+#define FOPEN_FLAGS_A O_APPEND | O_RDWR | O_CREAT
 
 #endif /* DEFAULTS_H */

src/bin/pg_autoctl/env_utils.c

@@ -26,7 +26,8 @@ env_found_empty(const char *name)
 	char *envvalue = NULL;
 	if (name == NULL || strlen(name) == 0)
 	{
-		log_error("Failed to get environment setting. NULL or empty variable name is provided");
+		log_error("Failed to get environment setting. "
+				  "NULL or empty variable name is provided");
 		return false;
 	}
 
@@ -50,7 +51,8 @@ env_exists(const char *name)
 {
 	if (name == NULL || strlen(name) == 0)
 	{
-		log_error("Failed to get environment setting. NULL or empty variable name is provided");
+		log_error("Failed to get environment setting. "
+				  "NULL or empty variable name is provided");
 		return false;
 	}
 
@@ -78,13 +80,15 @@ get_env_copy_with_fallback(const char *name, char *result, int maxLength,
 
 	if (name == NULL || strlen(name) == 0)
 	{
-		log_error("Failed to get environment setting. NULL or empty variable name is provided");
+		log_error("Failed to get environment setting. "
+				  "NULL or empty variable name is provided");
 		return false;
 	}
 
 	if (result == NULL)
 	{
-		log_error("Failed to get environment setting. Tried to store in NULL pointer");
+		log_error("Failed to get environment setting. "
+				  "Tried to store in NULL pointer");
 		return false;
 	}
 

src/bin/pg_autoctl/file_utils.c

@@ -173,8 +173,8 @@ fopen_read_only(const char *filePath)
 bool
 write_file(char *data, long fileSize, const char *filePath)
 {
-
 	FILE *fileStream = fopen_with_umask(filePath, "wb", FOPEN_FLAGS_W, 0644);
+
 	if (fileStream == NULL)
 	{
 		/* errors have already been logged */
@@ -207,6 +207,7 @@ bool
 append_to_file(char *data, long fileSize, const char *filePath)
 {
 	FILE *fileStream = fopen_with_umask(filePath, "ab", FOPEN_FLAGS_A, 0644);
+
 	if (fileStream == NULL)
 	{
 		/* errors have already been logged */

src/bin/pg_autoctl/monitor_config.c

@@ -578,11 +578,10 @@ monitor_config_set_setting(MonitorConfig *config,
 bool
 monitor_config_update_with_absolute_pgdata(MonitorConfig *config)
 {
-	PostgresSetup pgSetup = config->pgSetup;
+	PostgresSetup *pgSetup = &(config->pgSetup);
 
-	if (pg_setup_set_absolute_pgdata(&pgSetup))
+	if (pg_setup_set_absolute_pgdata(pgSetup))
 	{
-		strlcpy(config->pgSetup.pgdata, pgSetup.pgdata, MAXPGPATH);
 		if (!monitor_config_write_file(config))
 		{
 			/* errors have already been logged */

src/bin/pg_autoctl/monitor_pg_init.c

@@ -68,15 +68,15 @@ bool
 monitor_pg_init(Monitor *monitor, MonitorConfig *config)
 {
 	char configFilePath[MAXPGPATH];
-	PostgresSetup pgSetup = config->pgSetup;
+	PostgresSetup *pgSetup = &(config->pgSetup);
 
-	if (directory_exists(pgSetup.pgdata))
+	if (directory_exists(pgSetup->pgdata))
 	{
 		PostgresSetup existingPgSetup = { 0 };
 		bool missing_pgdata_is_ok = true;
 		bool pg_is_not_running_is_ok = true;
 
-		if (!pg_setup_init(&existingPgSetup, &pgSetup,
+		if (!pg_setup_init(&existingPgSetup, pgSetup,
 						   missing_pgdata_is_ok,
 						   pg_is_not_running_is_ok))
 		{
@@ -89,7 +89,7 @@ monitor_pg_init(Monitor *monitor, MonitorConfig *config)
 		{
 			log_info("Installing pg_auto_failover monitor in existing "
 					 "PostgreSQL instance at \"%s\" running on port %d",
-					 pgSetup.pgdata, existingPgSetup.pidFile.port);
+					 pgSetup->pgdata, existingPgSetup.pidFile.port);
 
 			if (!monitor_install(config->nodename, existingPgSetup, true))
 			{
@@ -105,15 +105,15 @@ monitor_pg_init(Monitor *monitor, MonitorConfig *config)
 		if (pg_setup_pgdata_exists(&existingPgSetup))
 		{
 			log_fatal("PGDATA directory \"%s\" already exists, skipping",
-					  pgSetup.pgdata);
+					  pgSetup->pgdata);
 			return false;
 		}
 	}
 
-	if (!pg_ctl_initdb(pgSetup.pg_ctl, pgSetup.pgdata))
+	if (!pg_ctl_initdb(pgSetup->pg_ctl, pgSetup->pgdata))
 	{
 		log_fatal("Failed to initialise a PostgreSQL instance at \"%s\", "
-				  "see above for details", pgSetup.pgdata);
+				  "see above for details", pgSetup->pgdata);
 		return false;
 	}
 
@@ -131,24 +131,24 @@ monitor_pg_init(Monitor *monitor, MonitorConfig *config)
 	 * We just did the initdb ourselves, so we know where the configuration
 	 * file is to be found Also, we didn't start PostgreSQL yet.
 	 */
-	join_path_components(configFilePath, pgSetup.pgdata, "postgresql.conf");
+	join_path_components(configFilePath, pgSetup->pgdata, "postgresql.conf");
 
 	/*
 	 * When --ssl-self-signed has been used, now is the time to build a
 	 * self-signed certificate for the server. We place the certificate and
 	 * private key in $PGDATA/server.key and $PGDATA/server.crt
 	 */
-	if (pgSetup.ssl.createSelfSignedCert)
+	if (pgSetup->ssl.createSelfSignedCert)
 	{
-		if (!pg_create_self_signed_cert(&pgSetup, config->nodename))
+		if (!pg_create_self_signed_cert(pgSetup, config->nodename))
 		{
 			log_error("Failed to create SSL self-signed certificate, "
 					  "see above for details");
 			return false;
 		}
 	}
 
-	if (!pg_add_auto_failover_default_settings(&pgSetup, configFilePath,
+	if (!pg_add_auto_failover_default_settings(pgSetup, configFilePath,
 											   monitor_default_settings))
 	{
 		log_error("Failed to add default settings to \"%s\": couldn't "
@@ -157,14 +157,16 @@ monitor_pg_init(Monitor *monitor, MonitorConfig *config)
 		return false;
 	}
 
-	if (!pg_ctl_start(pgSetup.pg_ctl,
-					  pgSetup.pgdata, pgSetup.pgport, pgSetup.listen_addresses))
+	if (!pg_ctl_start(pgSetup->pg_ctl,
+					  pgSetup->pgdata,
+					  pgSetup->pgport,
+					  pgSetup->listen_addresses))
 	{
 		log_error("Failed to start postgres, see above");
 		return false;
 	}
 
-	if (!monitor_install(config->nodename, pgSetup, false))
+	if (!monitor_install(config->nodename, *pgSetup, false))
 	{
 		return false;
 	}

src/bin/pg_autoctl/pgsetup.c

@@ -1151,7 +1151,7 @@ pgsetup_get_pgport()
 	char pgport_env[NAMEDATALEN];
 	int pgport = 0;
 
-	if (get_env_copy("PGPORT", pgport_env, NAMEDATALEN))
+	if (env_exists("PGPORT") && get_env_copy("PGPORT", pgport_env, NAMEDATALEN))
 	{
 		if (stringToInt(pgport_env, &pgport) && pgport > 0)
 		{