pg_basebackup against older server versions

Started by Devrim Gündüzalmost 7 years ago7 messages
#1Devrim Gündüz
devrim@gunduz.org

Hi,

Apologies if this has been discussed before: When I run pg_basebackup in git
head against v11 server, it treats v11 as v12: Does not create recovery.conf,
adds recovery parameters to postgresql.auto.conf, and also creates
standby.signal file. Is this expected, or a bug?

Regards,
--
Devrim Gündüz
Open Source Solution Architect, Red Hat Certified Engineer
Twitter: @DevrimGunduz , @DevrimGunduzTR

#2Michael Paquier
michael@paquier.xyz
In reply to: Devrim Gündüz (#1)
Re: pg_basebackup against older server versions

On Wed, Mar 06, 2019 at 11:55:12AM +0300, Devrim Gündüz wrote:

Apologies if this has been discussed before: When I run pg_basebackup in git
head against v11 server, it treats v11 as v12: Does not create recovery.conf,
adds recovery parameters to postgresql.auto.conf, and also creates
standby.signal file. Is this expected, or a bug?

You are right, this is a bug. Compatibility with past server versions
should be preserved, and we made an effort to do so in the past (see
the switch to pg_wal/ for example). Fortunately, maintaining the
compatibility is simple enough as the connection information is close
by so that we just need to change postgresql.auto.conf to
recovery.conf, and avoid the creation of standby.signal.
--
Michael

In reply to: Michael Paquier (#2)
1 attachment(s)
Re: pg_basebackup against older server versions

Hello

My fault. I thought pg_basebackup works only with same major version, sorry.
How about attached patch?

regards, Sergei

Attachments:

fix_pg_basebackup_against_older_server_versions.patchtext/x-diff; name=fix_pg_basebackup_against_older_server_versions.patchDownload
diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c
index 3d2d4cd0b9..e1aa2c1cfc 100644
--- a/src/bin/pg_basebackup/pg_basebackup.c
+++ b/src/bin/pg_basebackup/pg_basebackup.c
@@ -66,6 +66,11 @@ typedef struct TablespaceList
  */
 #define MINIMUM_VERSION_FOR_TEMP_SLOTS 100000
 
+/*
+ * recovery.conf has been moved into GUC system in version 12
+ */
+#define MINIMUM_VERSION_FOR_RECOVERY_GUC 120000
+
 /*
  * Different ways to include WAL
  */
@@ -974,6 +979,7 @@ ReceiveTarFile(PGconn *conn, PGresult *res, int rownum)
 	bool		basetablespace = PQgetisnull(res, rownum, 0);
 	bool		in_tarhdr = true;
 	bool		skip_file = false;
+	bool		is_recovery_guc_supported = true;
 	bool		is_postgresql_auto_conf = false;
 	bool		found_postgresql_auto_conf = false;
 	int			file_padding_len = 0;
@@ -984,6 +990,9 @@ ReceiveTarFile(PGconn *conn, PGresult *res, int rownum)
 	gzFile		ztarfile = NULL;
 #endif
 
+	if (PQserverVersion(conn) < MINIMUM_VERSION_FOR_RECOVERY_GUC)
+		is_recovery_guc_supported = false;
+
 	if (basetablespace)
 	{
 		/*
@@ -1130,11 +1139,15 @@ ReceiveTarFile(PGconn *conn, PGresult *res, int rownum)
 			{
 				char		header[512];
 
-				if (!found_postgresql_auto_conf)
+				/*
+				 * we need write new postgresql.auto.conf if missing or
+				 * recovery.conf for old systems
+				 */
+				if (!found_postgresql_auto_conf || !is_recovery_guc_supported)
 				{
 					int			padding;
 
-					tarCreateHeader(header, "postgresql.auto.conf", NULL,
+					tarCreateHeader(header, is_recovery_guc_supported ? "postgresql.auto.conf" : "recovery.conf", NULL,
 									recoveryconfcontents->len,
 									pg_file_create_mode, 04000, 02000,
 									time(NULL));
@@ -1147,13 +1160,16 @@ ReceiveTarFile(PGconn *conn, PGresult *res, int rownum)
 						WRITE_TAR_DATA(zerobuf, padding);
 				}
 
-				tarCreateHeader(header, "standby.signal", NULL,
-								0,	/* zero-length file */
-								pg_file_create_mode, 04000, 02000,
-								time(NULL));
+				if (is_recovery_guc_supported)
+				{
+					tarCreateHeader(header, "standby.signal", NULL,
+									0,	/* zero-length file */
+									pg_file_create_mode, 04000, 02000,
+									time(NULL));
 
-				WRITE_TAR_DATA(header, sizeof(header));
-				WRITE_TAR_DATA(zerobuf, 511);
+					WRITE_TAR_DATA(header, sizeof(header));
+					WRITE_TAR_DATA(zerobuf, 511);
+				}
 			}
 
 			/* 2 * 512 bytes empty data at end of file */
@@ -1253,15 +1269,25 @@ ReceiveTarFile(PGconn *conn, PGresult *res, int rownum)
 						 * look at the file metadata: we may want append
 						 * recovery info into postgresql.auto.conf and skip
 						 * standby.signal file.  In both cases we must
-						 * calculate tar padding
+						 * calculate tar padding. Also we need skip
+						 * recovery.conf file for old cluster versions
 						 */
-						skip_file = (strcmp(&tarhdr[0], "standby.signal") == 0);
-						is_postgresql_auto_conf = (strcmp(&tarhdr[0], "postgresql.auto.conf") == 0);
+						if (is_recovery_guc_supported)
+						{
+							skip_file = (strcmp(&tarhdr[0], "standby.signal") == 0);
+							is_postgresql_auto_conf = (strcmp(&tarhdr[0], "postgresql.auto.conf") == 0);
+						}
+						else
+						{
+							skip_file = (strcmp(&tarhdr[0], "recovery.conf") == 0);
+						}
 
 						filesz = read_tar_number(&tarhdr[124], 12);
 						file_padding_len = ((filesz + 511) & ~511) - filesz;
 
-						if (is_postgresql_auto_conf && writerecoveryconf)
+						if (is_recovery_guc_supported &&
+							is_postgresql_auto_conf &&
+							writerecoveryconf)
 						{
 							/* replace tar header */
 							char		header[512];
@@ -1313,7 +1339,9 @@ ReceiveTarFile(PGconn *conn, PGresult *res, int rownum)
 						pos += bytes2write;
 						filesz -= bytes2write;
 					}
-					else if (is_postgresql_auto_conf && writerecoveryconf)
+					else if (is_recovery_guc_supported &&
+							 is_postgresql_auto_conf &&
+							 writerecoveryconf)
 					{
 						/* append recovery config to postgresql.auto.conf */
 						int			padding;
@@ -1690,6 +1718,9 @@ GenerateRecoveryConf(PGconn *conn)
 		exit(1);
 	}
 
+	if (PQserverVersion(conn) < MINIMUM_VERSION_FOR_RECOVERY_GUC)
+		appendPQExpBufferStr(recoveryconfcontents, "standby_mode = 'on'\n");
+
 	connOptions = PQconninfo(conn);
 	if (connOptions == NULL)
 	{
@@ -1765,9 +1796,11 @@ WriteRecoveryConf(void)
 	char		filename[MAXPGPATH];
 	FILE	   *cf;
 
-	snprintf(filename, MAXPGPATH, "%s/%s", basedir, "postgresql.auto.conf");
+	snprintf(filename, MAXPGPATH, "%s/%s", basedir,
+			 PQserverVersion(conn) < MINIMUM_VERSION_FOR_RECOVERY_GUC ?
+			 "recovery.conf" : "postgresql.auto.conf");
 
-	cf = fopen(filename, "a");
+	cf = fopen(filename, PQserverVersion(conn) < MINIMUM_VERSION_FOR_RECOVERY_GUC ? "w" : "a");
 	if (cf == NULL)
 	{
 		fprintf(stderr, _("%s: could not open file \"%s\": %s\n"), progname, filename, strerror(errno));
@@ -1784,15 +1817,18 @@ WriteRecoveryConf(void)
 
 	fclose(cf);
 
-	snprintf(filename, MAXPGPATH, "%s/%s", basedir, "standby.signal");
-	cf = fopen(filename, "w");
-	if (cf == NULL)
+	if (PQserverVersion(conn) >= MINIMUM_VERSION_FOR_RECOVERY_GUC)
 	{
-		fprintf(stderr, _("%s: could not create file \"%s\": %s\n"), progname, filename, strerror(errno));
-		exit(1);
-	}
+		snprintf(filename, MAXPGPATH, "%s/%s", basedir, "standby.signal");
+		cf = fopen(filename, "w");
+		if (cf == NULL)
+		{
+			fprintf(stderr, _("%s: could not create file \"%s\": %s\n"), progname, filename, strerror(errno));
+			exit(1);
+		}
 
-	fclose(cf);
+		fclose(cf);
+	}
 }
 
 
#4Michael Paquier
michael@paquier.xyz
In reply to: Sergei Kornilov (#3)
1 attachment(s)
Re: pg_basebackup against older server versions

On Wed, Mar 06, 2019 at 01:42:16PM +0300, Sergei Kornilov wrote:

My fault. I thought pg_basebackup works only with same major version, sorry.
How about attached patch?

No problem. Thanks for the patch, the logic looks good and I made
some adjustments as attached. Does that look fine to you?
--
Michael

Attachments:

fix_pg_basebackup_against_older_server_versions_2.patchtext/x-diff; charset=us-asciiDownload
diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c
index 3d2d4cd0b9..d56c8740d4 100644
--- a/src/bin/pg_basebackup/pg_basebackup.c
+++ b/src/bin/pg_basebackup/pg_basebackup.c
@@ -66,6 +66,11 @@ typedef struct TablespaceList
  */
 #define MINIMUM_VERSION_FOR_TEMP_SLOTS 100000
 
+/*
+ * recovery.conf is integrated into postgresql.conf from version 12.
+ */
+#define MINIMUM_VERSION_FOR_RECOVERY_GUC 120000
+
 /*
  * Different ways to include WAL
  */
@@ -974,6 +979,7 @@ ReceiveTarFile(PGconn *conn, PGresult *res, int rownum)
 	bool		basetablespace = PQgetisnull(res, rownum, 0);
 	bool		in_tarhdr = true;
 	bool		skip_file = false;
+	bool		is_recovery_guc_supported = true;
 	bool		is_postgresql_auto_conf = false;
 	bool		found_postgresql_auto_conf = false;
 	int			file_padding_len = 0;
@@ -984,6 +990,10 @@ ReceiveTarFile(PGconn *conn, PGresult *res, int rownum)
 	gzFile		ztarfile = NULL;
 #endif
 
+	/* recovery.conf is integrated into postgresql.conf in 12 and newer */
+	if (PQserverVersion(conn) < MINIMUM_VERSION_FOR_RECOVERY_GUC)
+		is_recovery_guc_supported = false;
+
 	if (basetablespace)
 	{
 		/*
@@ -1130,11 +1140,22 @@ ReceiveTarFile(PGconn *conn, PGresult *res, int rownum)
 			{
 				char		header[512];
 
-				if (!found_postgresql_auto_conf)
+				/*
+				 * If postgresql.auto.conf has not been found in the streamed
+				 * data, add recovery configuration to postgresql.auto.conf if
+				 * recovery parameters are GUCs.  If the instance connected to
+				 * is older than 12, create recovery.conf with this data
+				 * otherwise.
+				 */
+				if (!found_postgresql_auto_conf || !is_recovery_guc_supported)
 				{
 					int			padding;
+					char	   *conffile = is_recovery_guc_supported ?
+					"postgresql.auto.conf" : "recovery.conf";
 
-					tarCreateHeader(header, "postgresql.auto.conf", NULL,
+					tarCreateHeader(header,
+									conffile,
+									NULL,
 									recoveryconfcontents->len,
 									pg_file_create_mode, 04000, 02000,
 									time(NULL));
@@ -1142,18 +1163,26 @@ ReceiveTarFile(PGconn *conn, PGresult *res, int rownum)
 					padding = ((recoveryconfcontents->len + 511) & ~511) - recoveryconfcontents->len;
 
 					WRITE_TAR_DATA(header, sizeof(header));
-					WRITE_TAR_DATA(recoveryconfcontents->data, recoveryconfcontents->len);
+					WRITE_TAR_DATA(recoveryconfcontents->data,
+								   recoveryconfcontents->len);
 					if (padding)
 						WRITE_TAR_DATA(zerobuf, padding);
 				}
 
-				tarCreateHeader(header, "standby.signal", NULL,
-								0,	/* zero-length file */
-								pg_file_create_mode, 04000, 02000,
-								time(NULL));
+				/*
+				 * standby.signal is supported only if recovery parameters are
+				 * GUCs.
+				 */
+				if (is_recovery_guc_supported)
+				{
+					tarCreateHeader(header, "standby.signal", NULL,
+									0,	/* zero-length file */
+									pg_file_create_mode, 04000, 02000,
+									time(NULL));
 
-				WRITE_TAR_DATA(header, sizeof(header));
-				WRITE_TAR_DATA(zerobuf, 511);
+					WRITE_TAR_DATA(header, sizeof(header));
+					WRITE_TAR_DATA(zerobuf, 511);
+				}
 			}
 
 			/* 2 * 512 bytes empty data at end of file */
@@ -1252,16 +1281,24 @@ ReceiveTarFile(PGconn *conn, PGresult *res, int rownum)
 						 * We have the complete header structure in tarhdr,
 						 * look at the file metadata: we may want append
 						 * recovery info into postgresql.auto.conf and skip
-						 * standby.signal file.  In both cases we must
-						 * calculate tar padding
+						 * standby.signal file if recovery parameters are
+						 * integrated as GUCs, and recovery.conf otherwise. In
+						 * both cases we must calculate tar padding.
 						 */
-						skip_file = (strcmp(&tarhdr[0], "standby.signal") == 0);
-						is_postgresql_auto_conf = (strcmp(&tarhdr[0], "postgresql.auto.conf") == 0);
+						if (is_recovery_guc_supported)
+						{
+							skip_file = (strcmp(&tarhdr[0], "standby.signal") == 0);
+							is_postgresql_auto_conf = (strcmp(&tarhdr[0], "postgresql.auto.conf") == 0);
+						}
+						else
+							skip_file = (strcmp(&tarhdr[0], "recovery.conf") == 0);
 
 						filesz = read_tar_number(&tarhdr[124], 12);
 						file_padding_len = ((filesz + 511) & ~511) - filesz;
 
-						if (is_postgresql_auto_conf && writerecoveryconf)
+						if (is_recovery_guc_supported &&
+							is_postgresql_auto_conf &&
+							writerecoveryconf)
 						{
 							/* replace tar header */
 							char		header[512];
@@ -1313,7 +1350,9 @@ ReceiveTarFile(PGconn *conn, PGresult *res, int rownum)
 						pos += bytes2write;
 						filesz -= bytes2write;
 					}
-					else if (is_postgresql_auto_conf && writerecoveryconf)
+					else if (is_recovery_guc_supported &&
+							 is_postgresql_auto_conf &&
+							 writerecoveryconf)
 					{
 						/* append recovery config to postgresql.auto.conf */
 						int			padding;
@@ -1690,6 +1729,13 @@ GenerateRecoveryConf(PGconn *conn)
 		exit(1);
 	}
 
+	/*
+	 * In PostgreSQL 12 and newer versions, standby_mode is gone, replaced by
+	 * standby.signal to trigger a standby state at recovery.
+	 */
+	if (PQserverVersion(conn) < MINIMUM_VERSION_FOR_RECOVERY_GUC)
+		appendPQExpBufferStr(recoveryconfcontents, "standby_mode = 'on'\n");
+
 	connOptions = PQconninfo(conn);
 	if (connOptions == NULL)
 	{
@@ -1756,21 +1802,30 @@ GenerateRecoveryConf(PGconn *conn)
 
 /*
  * Write the configuration file into the directory specified in basedir,
- * with the contents already collected in memory.
- * Then write the signal file into the basedir also.
+ * with the contents already collected in memory appended.  Then write
+ * the signal file into the basedir.  If the server does not support
+ * recovery parameters as GUCs, the signal file is not necessary, and
+ * configuration is written to recovery.conf.
  */
 static void
 WriteRecoveryConf(void)
 {
 	char		filename[MAXPGPATH];
 	FILE	   *cf;
+	bool		is_recovery_guc_supported = true;
 
-	snprintf(filename, MAXPGPATH, "%s/%s", basedir, "postgresql.auto.conf");
+	if (PQserverVersion(conn) < MINIMUM_VERSION_FOR_RECOVERY_GUC)
+		is_recovery_guc_supported = false;
 
-	cf = fopen(filename, "a");
+	snprintf(filename, MAXPGPATH, "%s/%s", basedir,
+			 is_recovery_guc_supported ?
+			 "postgresql.auto.conf" : "recovery.conf");
+
+	cf = fopen(filename, is_recovery_guc_supported ? "a" : "w");
 	if (cf == NULL)
 	{
-		fprintf(stderr, _("%s: could not open file \"%s\": %s\n"), progname, filename, strerror(errno));
+		fprintf(stderr, _("%s: could not open file \"%s\": %s\n"),
+				progname, filename, strerror(errno));
 		exit(1);
 	}
 
@@ -1784,15 +1839,18 @@ WriteRecoveryConf(void)
 
 	fclose(cf);
 
-	snprintf(filename, MAXPGPATH, "%s/%s", basedir, "standby.signal");
-	cf = fopen(filename, "w");
-	if (cf == NULL)
+	if (is_recovery_guc_supported)
 	{
-		fprintf(stderr, _("%s: could not create file \"%s\": %s\n"), progname, filename, strerror(errno));
-		exit(1);
-	}
+		snprintf(filename, MAXPGPATH, "%s/%s", basedir, "standby.signal");
+		cf = fopen(filename, "w");
+		if (cf == NULL)
+		{
+			fprintf(stderr, _("%s: could not create file \"%s\": %s\n"), progname, filename, strerror(errno));
+			exit(1);
+		}
 
-	fclose(cf);
+		fclose(cf);
+	}
 }
 
 
In reply to: Michael Paquier (#4)
Re: pg_basebackup against older server versions

Hi

No problem. Thanks for the patch, the logic looks good and I made
some adjustments as attached. Does that look fine to you?

Looks fine, thanks. I tested against HEAD and v11.2 with and without -R in both plain and tar formats.

regards, Sergei

#6Michael Paquier
michael@paquier.xyz
In reply to: Sergei Kornilov (#5)
Re: pg_basebackup against older server versions

On Thu, Mar 07, 2019 at 10:57:46AM +0300, Sergei Kornilov wrote:

Looks fine, thanks. I tested against HEAD and v11.2 with and without
-R in both plain and tar formats.

Same here, so I have committed the patch.
--
Michael

In reply to: Michael Paquier (#6)
Re: pg_basebackup against older server versions

<br />Hi<div>Great, thank you! </div><div><br /></div><div>regards, Sergei</div>