pg_dump: Simplify internal archive version handling

Started by Peter Eisentrautabout 9 years ago3 messages
#1Peter Eisentraut
peter.eisentraut@2ndquadrant.com
1 attachment(s)

I propose the attached patch to clean up some redundancy and confusion
in pg_dump.

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Attachments:

0001-pg_dump-Simplify-internal-archive-version-handling.patchtext/x-patch; name=0001-pg_dump-Simplify-internal-archive-version-handling.patchDownload
From 41fce2adb8b3051ba1b29f592d4d7b8e52aeb30a Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter_e@gmx.net>
Date: Thu, 13 Oct 2016 12:00:00 -0400
Subject: [PATCH] pg_dump: Simplify internal archive version handling

The ArchiveHandle structure contained the archive format version number
twice, once as a single field and once split into components.  Simplify
that by just keeping the single field and adding some macros to extract
the components.  Also introduce some macros for composing version
numbers, to eliminate the repeated use of magic formulas.
---
 src/bin/pg_dump/pg_backup_archiver.c | 52 ++++++++++++++++-----------------
 src/bin/pg_dump/pg_backup_archiver.h | 56 ++++++++++++++++++------------------
 2 files changed, 53 insertions(+), 55 deletions(-)

diff --git a/src/bin/pg_dump/pg_backup_archiver.c b/src/bin/pg_dump/pg_backup_archiver.c
index e237b4a..0e20985 100644
--- a/src/bin/pg_dump/pg_backup_archiver.c
+++ b/src/bin/pg_dump/pg_backup_archiver.c
@@ -1105,7 +1105,8 @@ PrintTOCSummary(Archive *AHX)
 			fmtName = "UNKNOWN";
 	}
 
-	ahprintf(AH, ";     Dump Version: %d.%d-%d\n", AH->vmaj, AH->vmin, AH->vrev);
+	ahprintf(AH, ";     Dump Version: %d.%d-%d\n",
+			 ARCHIVE_MAJOR(AH->version), ARCHIVE_MINOR(AH->version), ARCHIVE_REV(AH->version));
 	ahprintf(AH, ";     Format: %s\n", fmtName);
 	ahprintf(AH, ";     Integer: %d bytes\n", (int) AH->intSize);
 	ahprintf(AH, ";     Offset: %d bytes\n", (int) AH->offSize);
@@ -2106,6 +2107,7 @@ _discoverArchiveFormat(ArchiveHandle *AH)
 	if (strncmp(sig, "PGDMP", 5) == 0)
 	{
 		int			byteread;
+		char		vmaj, vmin, vrev;
 
 		/*
 		 * Finish reading (most of) a custom-format header.
@@ -2115,31 +2117,30 @@ _discoverArchiveFormat(ArchiveHandle *AH)
 		if ((byteread = fgetc(fh)) == EOF)
 			READ_ERROR_EXIT(fh);
 
-		AH->vmaj = byteread;
+		vmaj = byteread;
 
 		if ((byteread = fgetc(fh)) == EOF)
 			READ_ERROR_EXIT(fh);
 
-		AH->vmin = byteread;
+		vmin = byteread;
 
 		/* Save these too... */
-		AH->lookahead[AH->lookaheadLen++] = AH->vmaj;
-		AH->lookahead[AH->lookaheadLen++] = AH->vmin;
+		AH->lookahead[AH->lookaheadLen++] = vmaj;
+		AH->lookahead[AH->lookaheadLen++] = vmin;
 
 		/* Check header version; varies from V1.0 */
-		if (AH->vmaj > 1 || ((AH->vmaj == 1) && (AH->vmin > 0)))		/* Version > 1.0 */
+		if (vmaj > 1 || (vmaj == 1 && vmin > 0))		/* Version > 1.0 */
 		{
 			if ((byteread = fgetc(fh)) == EOF)
 				READ_ERROR_EXIT(fh);
 
-			AH->vrev = byteread;
-			AH->lookahead[AH->lookaheadLen++] = AH->vrev;
+			vrev = byteread;
+			AH->lookahead[AH->lookaheadLen++] = vrev;
 		}
 		else
-			AH->vrev = 0;
+			vrev = 0;
 
-		/* Make a convenient integer <maj><min><rev>00 */
-		AH->version = ((AH->vmaj * 256 + AH->vmin) * 256 + AH->vrev) * 256 + 0;
+		AH->version = MAKE_ARCHIVE_VERSION(vmaj, vmin, vrev);
 
 		if ((AH->intSize = fgetc(fh)) == EOF)
 			READ_ERROR_EXIT(fh);
@@ -2234,12 +2235,7 @@ _allocAH(const char *FileSpec, const ArchiveFormat fmt,
 
 	/* AH->debugLevel = 100; */
 
-	AH->vmaj = K_VERS_MAJOR;
-	AH->vmin = K_VERS_MINOR;
-	AH->vrev = K_VERS_REV;
-
-	/* Make a convenient integer <maj><min><rev>00 */
-	AH->version = ((AH->vmaj * 256 + AH->vmin) * 256 + AH->vrev) * 256 + 0;
+	AH->version = K_VERS_SELF;
 
 	/* initialize for backwards compatible string processing */
 	AH->public.encoding = 0;	/* PG_SQL_ASCII */
@@ -3528,9 +3524,9 @@ WriteHead(ArchiveHandle *AH)
 	struct tm	crtm;
 
 	(*AH->WriteBufPtr) (AH, "PGDMP", 5);		/* Magic code */
-	(*AH->WriteBytePtr) (AH, AH->vmaj);
-	(*AH->WriteBytePtr) (AH, AH->vmin);
-	(*AH->WriteBytePtr) (AH, AH->vrev);
+	(*AH->WriteBytePtr) (AH, ARCHIVE_MAJOR(AH->version));
+	(*AH->WriteBytePtr) (AH, ARCHIVE_MINOR(AH->version));
+	(*AH->WriteBytePtr) (AH, ARCHIVE_REV(AH->version));
 	(*AH->WriteBytePtr) (AH, AH->intSize);
 	(*AH->WriteBytePtr) (AH, AH->offSize);
 	(*AH->WriteBytePtr) (AH, AH->format);
@@ -3563,24 +3559,26 @@ ReadHead(ArchiveHandle *AH)
 	 */
 	if (!AH->readHeader)
 	{
+		char		vmaj, vmin, vrev;
+
 		(*AH->ReadBufPtr) (AH, tmpMag, 5);
 
 		if (strncmp(tmpMag, "PGDMP", 5) != 0)
 			exit_horribly(modulename, "did not find magic string in file header\n");
 
-		AH->vmaj = (*AH->ReadBytePtr) (AH);
-		AH->vmin = (*AH->ReadBytePtr) (AH);
+		vmaj = (*AH->ReadBytePtr) (AH);
+		vmin = (*AH->ReadBytePtr) (AH);
 
-		if (AH->vmaj > 1 || ((AH->vmaj == 1) && (AH->vmin > 0)))		/* Version > 1.0 */
-			AH->vrev = (*AH->ReadBytePtr) (AH);
+		if (vmaj > 1 || (vmaj == 1 && vmin > 0))		/* Version > 1.0 */
+			vrev = (*AH->ReadBytePtr) (AH);
 		else
-			AH->vrev = 0;
+			vrev = 0;
 
-		AH->version = ((AH->vmaj * 256 + AH->vmin) * 256 + AH->vrev) * 256 + 0;
+		AH->version = MAKE_ARCHIVE_VERSION(vmaj, vmin, vrev);
 
 		if (AH->version < K_VERS_1_0 || AH->version > K_VERS_MAX)
 			exit_horribly(modulename, "unsupported version (%d.%d) in file header\n",
-						  AH->vmaj, AH->vmin);
+						  vmaj, vmin);
 
 		AH->intSize = (*AH->ReadBytePtr) (AH);
 		if (AH->intSize > 32)
diff --git a/src/bin/pg_dump/pg_backup_archiver.h b/src/bin/pg_dump/pg_backup_archiver.h
index 97d34a5..2fae79e 100644
--- a/src/bin/pg_dump/pg_backup_archiver.h
+++ b/src/bin/pg_dump/pg_backup_archiver.h
@@ -62,37 +62,40 @@ typedef struct _z_stream
 typedef z_stream *z_streamp;
 #endif
 
-/* Current archive version number (the format we can output) */
-#define K_VERS_MAJOR 1
-#define K_VERS_MINOR 12
-#define K_VERS_REV 0
-
 /* Data block types */
 #define BLK_DATA 1
 #define BLK_BLOBS 3
 
+/* Encode version components into a convenient integer <maj><min><rev>00 */
+#define MAKE_ARCHIVE_VERSION(major, minor, rev) (( ((major) * 256 + (minor)) * 256 + (rev)) * 256 + 0)
+
+#define ARCHIVE_MAJOR(version) ((version)/256/256/256 & 255)
+#define ARCHIVE_MINOR(version) ((version)/256/256 & 255)
+#define ARCHIVE_REV(version)   ((version)/256 & 255)
+
 /* Historical version numbers (checked in code) */
-#define K_VERS_1_0 (( (1 * 256 + 0) * 256 + 0) * 256 + 0)
-#define K_VERS_1_2 (( (1 * 256 + 2) * 256 + 0) * 256 + 0)		/* Allow No ZLIB */
-#define K_VERS_1_3 (( (1 * 256 + 3) * 256 + 0) * 256 + 0)		/* BLOBs */
-#define K_VERS_1_4 (( (1 * 256 + 4) * 256 + 0) * 256 + 0)		/* Date & name in header */
-#define K_VERS_1_5 (( (1 * 256 + 5) * 256 + 0) * 256 + 0)		/* Handle dependencies */
-#define K_VERS_1_6 (( (1 * 256 + 6) * 256 + 0) * 256 + 0)		/* Schema field in TOCs */
-#define K_VERS_1_7 (( (1 * 256 + 7) * 256 + 0) * 256 + 0)		/* File Offset size in
-																 * header */
-#define K_VERS_1_8 (( (1 * 256 + 8) * 256 + 0) * 256 + 0)		/* change interpretation
-																 * of ID numbers and
-																 * dependencies */
-#define K_VERS_1_9 (( (1 * 256 + 9) * 256 + 0) * 256 + 0)		/* add default_with_oids
-																 * tracking */
-#define K_VERS_1_10 (( (1 * 256 + 10) * 256 + 0) * 256 + 0)		/* add tablespace */
-#define K_VERS_1_11 (( (1 * 256 + 11) * 256 + 0) * 256 + 0)		/* add toc section
-																 * indicator */
-#define K_VERS_1_12 (( (1 * 256 + 12) * 256 + 0) * 256 + 0)		/* add separate BLOB
-																 * entries */
+#define K_VERS_1_0	MAKE_ARCHIVE_VERSION(1, 0, 0)
+#define K_VERS_1_2	MAKE_ARCHIVE_VERSION(1, 2, 0)	/* Allow No ZLIB */
+#define K_VERS_1_3	MAKE_ARCHIVE_VERSION(1, 3, 0)	/* BLOBs */
+#define K_VERS_1_4	MAKE_ARCHIVE_VERSION(1, 4, 0)	/* Date & name in header */
+#define K_VERS_1_5	MAKE_ARCHIVE_VERSION(1, 5, 0)	/* Handle dependencies */
+#define K_VERS_1_6	MAKE_ARCHIVE_VERSION(1, 6, 0)	/* Schema field in TOCs */
+#define K_VERS_1_7	MAKE_ARCHIVE_VERSION(1, 7, 0)	/* File Offset size in header */
+#define K_VERS_1_8	MAKE_ARCHIVE_VERSION(1, 8, 0)	/* change interpretation of ID
+													   numbers and dependencies */
+#define K_VERS_1_9	MAKE_ARCHIVE_VERSION(1, 9, 0)	/* add default_with_oids tracking */
+#define K_VERS_1_10	MAKE_ARCHIVE_VERSION(1, 10, 0)	/* add tablespace */
+#define K_VERS_1_11	MAKE_ARCHIVE_VERSION(1, 11, 0)	/* add toc section indicator */
+#define K_VERS_1_12	MAKE_ARCHIVE_VERSION(1, 12, 0)	/* add separate BLOB entries */
+
+/* Current archive version number (the format we can output) */
+#define K_VERS_MAJOR 1
+#define K_VERS_MINOR 12
+#define K_VERS_REV 0
+#define K_VERS_SELF	MAKE_ARCHIVE_VERSION(K_VERS_MAJOR, K_VERS_MINOR, K_VERS_REV);
 
 /* Newest format we can read */
-#define K_VERS_MAX (( (1 * 256 + 12) * 256 + 255) * 256 + 0)
+#define K_VERS_MAX	MAKE_ARCHIVE_VERSION(K_VERS_MAJOR, K_VERS_MINOR, 255)
 
 
 /* Flags to indicate disposition of offsets stored in files */
@@ -205,10 +208,7 @@ typedef enum
 struct _archiveHandle
 {
 	Archive		public;			/* Public part of archive */
-	char		vmaj;			/* Version of file */
-	char		vmin;
-	char		vrev;
-	int			version;		/* Conveniently formatted version */
+	int			version;		/* Version of file */
 
 	char	   *archiveRemoteVersion;	/* When reading an archive, the
 										 * version of the dumped DB */
-- 
2.10.1

#2Tom Lane
tgl@sss.pgh.pa.us
In reply to: Peter Eisentraut (#1)
Re: pg_dump: Simplify internal archive version handling

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:

I propose the attached patch to clean up some redundancy and confusion
in pg_dump.

Looks sane, though I'd suggest coding the access macros with shifts
not multiplies/divides.

Since these numbers are purely internal and not stored in this form in the
file, I wonder whether we shouldn't drop the rightmost zero byte, ie make
it just <maj><min><rev> not <maj><min><rev>00. That would save at least
one divide/shift when accessing.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#3Peter Eisentraut
peter.eisentraut@2ndquadrant.com
In reply to: Tom Lane (#2)
Re: pg_dump: Simplify internal archive version handling

On 10/13/16 9:13 AM, Tom Lane wrote:

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:

I propose the attached patch to clean up some redundancy and confusion
in pg_dump.

Looks sane, though I'd suggest coding the access macros with shifts
not multiplies/divides.

Done.

Since these numbers are purely internal and not stored in this form in the
file, I wonder whether we shouldn't drop the rightmost zero byte, ie make
it just <maj><min><rev> not <maj><min><rev>00. That would save at least
one divide/shift when accessing.

Done and committed. Thanks.

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers