Move logic from postgres/pageChecksum to command/backup/pageChecksum().

The postgres/pageChecksum module was designed as an interface to the C structs for the Perl code. The new C code can do this directly so no need for an interface. Move the remaining test for pgPageChecksum() into the postgres/interface test module.
pgbackrest · Mar 5, 2020 · e55443c · e55443c
1 parent 3796b74
commit e55443c
Show file tree

Hide file tree

Showing 10 changed files with 81 additions and 176 deletions.
diff --git a/src/Makefile.in b/src/Makefile.in
@@ -155,7 +155,6 @@ SRCS = \
 	postgres/interface/v100.c \
 	postgres/interface/v110.c \
 	postgres/interface/v120.c \
-	postgres/pageChecksum.c \
 	protocol/client.c \
 	protocol/command.c \
 	protocol/helper.c \
@@ -243,7 +242,7 @@ command/backup/common.o: command/backup/common.c build.auto.h command/backup/com
 command/backup/file.o: command/backup/file.c build.auto.h command/backup/file.h command/backup/pageChecksum.h common/assert.h common/compress/gz/common.h common/compress/gz/compress.h common/compress/gz/decompress.h common/crypto/cipherBlock.h common/crypto/common.h common/crypto/hash.h common/debug.h common/error.auto.h common/error.h common/io/filter/filter.h common/io/filter/group.h common/io/filter/size.h common/io/io.h common/io/read.h common/io/write.h common/log.h common/logLevel.h common/memContext.h common/regExp.h common/stackTrace.h common/time.h common/type/buffer.h common/type/convert.h common/type/keyValue.h common/type/list.h common/type/param.h common/type/string.h common/type/stringList.h common/type/stringz.h common/type/variant.h common/type/variantList.h postgres/interface.h storage/helper.h storage/info.h storage/read.h storage/storage.h storage/write.h
 	$(CC) $(CPPFLAGS) $(CFLAGS) $(CMAKE) -c command/backup/file.c -o command/backup/file.o
 
-command/backup/pageChecksum.o: command/backup/pageChecksum.c build.auto.h command/backup/pageChecksum.h common/assert.h common/debug.h common/error.auto.h common/error.h common/io/filter/filter.h common/io/filter/filter.intern.h common/io/filter/group.h common/io/read.h common/io/write.h common/log.h common/logLevel.h common/macro.h common/memContext.h common/object.h common/stackTrace.h common/time.h common/type/buffer.h common/type/convert.h common/type/keyValue.h common/type/list.h common/type/param.h common/type/string.h common/type/stringList.h common/type/stringz.h common/type/variant.h common/type/variantList.h postgres/interface.h postgres/pageChecksum.h storage/info.h storage/read.h storage/storage.h storage/write.h
+command/backup/pageChecksum.o: command/backup/pageChecksum.c build.auto.h command/backup/pageChecksum.h common/assert.h common/debug.h common/error.auto.h common/error.h common/io/filter/filter.h common/io/filter/filter.intern.h common/io/filter/group.h common/io/read.h common/io/write.h common/log.h common/logLevel.h common/macro.h common/memContext.h common/object.h common/stackTrace.h common/time.h common/type/buffer.h common/type/convert.h common/type/keyValue.h common/type/list.h common/type/param.h common/type/string.h common/type/stringList.h common/type/stringz.h common/type/variant.h common/type/variantList.h postgres/interface.h postgres/interface/static.auto.h storage/info.h storage/read.h storage/storage.h storage/write.h
 	$(CC) $(CPPFLAGS) $(CFLAGS) $(CMAKE) -c command/backup/pageChecksum.c -o command/backup/pageChecksum.o
 
 command/backup/protocol.o: command/backup/protocol.c build.auto.h command/backup/file.h command/backup/protocol.h common/assert.h common/crypto/common.h common/debug.h common/error.auto.h common/error.h common/io/filter/filter.h common/io/filter/group.h common/io/io.h common/io/read.h common/io/write.h common/lock.h common/log.h common/logLevel.h common/memContext.h common/stackTrace.h common/time.h common/type/buffer.h common/type/convert.h common/type/keyValue.h common/type/list.h common/type/param.h common/type/string.h common/type/stringList.h common/type/stringz.h common/type/variant.h common/type/variantList.h config/config.auto.h config/config.h config/define.auto.h config/define.h protocol/server.h storage/helper.h storage/info.h storage/read.h storage/storage.h storage/write.h
@@ -549,9 +548,6 @@ postgres/interface/v110.o: postgres/interface/v110.c build.auto.h common/assert.
 postgres/interface/v120.o: postgres/interface/v120.c build.auto.h common/assert.h common/debug.h common/error.auto.h common/error.h common/io/filter/filter.h common/io/filter/group.h common/io/read.h common/io/write.h common/logLevel.h common/memContext.h common/stackTrace.h common/time.h common/type/buffer.h common/type/convert.h common/type/keyValue.h common/type/list.h common/type/param.h common/type/string.h common/type/stringList.h common/type/stringz.h common/type/variant.h common/type/variantList.h postgres/interface.h postgres/interface/static.auto.h postgres/interface/version.auto.h postgres/interface/version.h postgres/interface/version.intern.h postgres/version.h storage/info.h storage/read.h storage/storage.h storage/write.h
 	$(CC) $(CPPFLAGS) $(CFLAGS) $(CMAKE) -c postgres/interface/v120.c -o postgres/interface/v120.o
 
-postgres/pageChecksum.o: postgres/pageChecksum.c build.auto.h common/assert.h common/debug.h common/error.auto.h common/error.h common/io/filter/filter.h common/io/filter/group.h common/io/read.h common/io/write.h common/log.h common/logLevel.h common/memContext.h common/stackTrace.h common/time.h common/type/buffer.h common/type/convert.h common/type/keyValue.h common/type/list.h common/type/param.h common/type/string.h common/type/stringList.h common/type/stringz.h common/type/variant.h common/type/variantList.h postgres/interface.h postgres/interface/static.auto.h storage/info.h storage/read.h storage/storage.h storage/write.h
-	$(CC) $(CPPFLAGS) $(CFLAGS) $(CMAKE) -c postgres/pageChecksum.c -o postgres/pageChecksum.o
-
 protocol/client.o: protocol/client.c build.auto.h common/assert.h common/debug.h common/error.auto.h common/error.h common/io/filter/filter.h common/io/filter/group.h common/io/read.h common/io/write.h common/log.h common/logLevel.h common/macro.h common/memContext.h common/object.h common/stackTrace.h common/time.h common/type/buffer.h common/type/convert.h common/type/json.h common/type/keyValue.h common/type/string.h common/type/stringz.h common/type/variant.h common/type/variantList.h protocol/client.h protocol/command.h version.h
 	$(CC) $(CPPFLAGS) $(CFLAGS) $(CMAKE) -c protocol/client.c -o protocol/client.o
 

diff --git a/src/command/backup/pageChecksum.c b/src/command/backup/pageChecksum.c
@@ -9,8 +9,8 @@ Page Checksum Filter
 #include "common/log.h"
 #include "common/memContext.h"
 #include "common/object.h"
-#include "postgres/pageChecksum.h"
 #include "postgres/interface.h"
+#include "postgres/interface/static.auto.h"
 
 /***********************************************************************************************************************************
 Filter type constant
@@ -100,12 +100,22 @@ pageChecksumProcess(THIS_VOID, const Buffer *input)
             // the data should be the same, but there's no question that some munging occurs.  Should we make a copy of the page
             // before passing it into pgPageChecksumTest()?
             unsigned char *pagePtr = bufPtr(input) + (pageIdx * PG_PAGE_SIZE_DEFAULT);
-            unsigned int pageNo = this->pageNoOffset + pageIdx;
-            size_t pageSize = this->align || pageIdx < pageTotal - 1 ? PG_PAGE_SIZE_DEFAULT : pageRemainder;
 
-            if (!pgPageChecksumTest(
-                    pagePtr, pageNo, (unsigned int)pageSize, (unsigned int)(this->lsnLimit >> 32),
-                    (unsigned int)(this->lsnLimit & 0xFFFFFFFF)))
+            // Get a pointer to the page header at the beginning of the page
+            const PageHeaderData *pageHeader = (const PageHeaderData *)pagePtr;
+
+            // Get the page lsn
+            uint64_t pageLsn = PageXLogRecPtrGet(pageHeader->pd_lsn);
+
+            // Block number relative to all segments in the relation
+            unsigned int blockNo = this->pageNoOffset + pageIdx;
+
+            if (// This is a new page so don't test checksum
+                !(pageHeader->pd_upper == 0 ||
+                // LSN is after the backup started so checksum is not tested because pages may be torn
+                pageLsn >= this->lsnLimit ||
+                // Checksum is valid if a full page
+                ((this->align || pageIdx < pageTotal - 1) && pageHeader->pd_checksum == pgPageChecksum(pagePtr, blockNo))))
             {
                 MEM_CONTEXT_BEGIN(this->memContext)
                 {
@@ -115,8 +125,8 @@ pageChecksumProcess(THIS_VOID, const Buffer *input)
 
                     // Add page number and lsn to the error list
                     VariantList *pair = varLstNew();
-                    varLstAdd(pair, varNewUInt(pageNo));
-                    varLstAdd(pair, varNewUInt64(pgPageLsn(pagePtr)));
+                    varLstAdd(pair, varNewUInt(blockNo));
+                    varLstAdd(pair, varNewUInt64(pageLsn));
                     varLstAdd(this->error, varNewVarLst(pair));
                 }
                 MEM_CONTEXT_END();

diff --git a/src/postgres/interface/static.auto.h b/src/postgres/interface/static.auto.h
@@ -20,6 +20,9 @@ which could have a large impact on dependencies.  Hopefully that won't happen of
 Note when adding new types it is safer to add them to version.auto.c unless they are needed for code that must be compatible across
 all versions of PostgreSQL supported by pgBackRest.
 ***********************************************************************************************************************************/
+#ifndef POSTGRES_INTERFACE_STATICAUTO_H
+#define POSTGRES_INTERFACE_STATICAUTO_H
+
 #include "common/assert.h"
 #include "postgres/interface.h"
 
@@ -45,6 +48,10 @@ typedef uint16_t uint16;
 // ---------------------------------------------------------------------------------------------------------------------------------
 typedef uint32_t uint32;
 
+// uint64 type
+// ---------------------------------------------------------------------------------------------------------------------------------
+typedef uint64_t uint64;
+
 // TransactionId type
 // ---------------------------------------------------------------------------------------------------------------------------------
 typedef uint32 TransactionId;
@@ -130,6 +137,11 @@ typedef struct
 	uint32		xrecoff;		/* low bits */
 } PageXLogRecPtr;
 
+// PageXLogRecPtrGet macro
+// ---------------------------------------------------------------------------------------------------------------------------------
+#define PageXLogRecPtrGet(val) \
+	((uint64) (val).xlogid << 32 | (val).xrecoff)
+
 // PageHeaderData type
 // ---------------------------------------------------------------------------------------------------------------------------------
 /*
@@ -204,3 +216,5 @@ typedef PageHeaderData *PageHeader;
  *		returns true if page has not been initialized (by PageInit)
  */
 #define PageIsNew(page) (((PageHeader) (page))->pd_upper == 0)
+
+#endif
diff --git a/src/postgres/interface/version.auto.h b/src/postgres/interface/version.auto.h
@@ -40,16 +40,6 @@ typedef int64_t int64;
 
 #endif
 
-// uint64 type
-// ---------------------------------------------------------------------------------------------------------------------------------
-#if PG_VERSION > PG_VERSION_MAX
-
-#elif PG_VERSION >= PG_VERSION_83
-
-typedef uint64_t uint64;
-
-#endif
-
 // MultiXactId type
 // ---------------------------------------------------------------------------------------------------------------------------------
 #if PG_VERSION > PG_VERSION_MAX

diff --git a/src/postgres/pageChecksum.c b/src/postgres/pageChecksum.c
diff --git a/src/postgres/pageChecksum.h b/src/postgres/pageChecksum.h
diff --git a/test/define.yaml b/test/define.yaml
@@ -316,17 +316,11 @@ unit:
 
       # ----------------------------------------------------------------------------------------------------------------------------
       - name: interface
-        total: 8
+        total: 9
 
         coverage:
           postgres/interface: full
-
-      # ----------------------------------------------------------------------------------------------------------------------------
-      - name: page-checksum
-        total: 2
-
-        coverage:
-          postgres/pageChecksum: full
+          postgres/interface/page: full
 
   # ********************************************************************************************************************************
   - name: config

diff --git a/test/src/module/command/backupCommonTest.c b/test/src/module/command/backupCommonTest.c
@@ -122,6 +122,39 @@ testRun(void)
             jsonFromVar(ioFilterGroupResult(ioWriteFilterGroup(write), PAGE_CHECKSUM_FILTER_TYPE_STR)),
             "{\"align\":true,\"valid\":true}", "all zero pages");
 
+        // Single valid page
+        // -------------------------------------------------------------------------------------------------------------------------
+        buffer = bufNew(PG_PAGE_SIZE_DEFAULT * 1);
+        bufUsedSet(buffer, bufSize(buffer));
+        memset(bufPtr(buffer), 0, bufSize(buffer));
+
+        // Page 0 has good checksum
+        *(PageHeaderData *)(bufPtr(buffer) + (PG_PAGE_SIZE_DEFAULT * 0x00)) = (PageHeaderData)
+        {
+            .pd_upper = 0x01,
+            .pd_lsn = (PageXLogRecPtr)
+            {
+                .xlogid = 0xF0F0F0F0,
+                .xrecoff = 0xF0F0F0F0,
+            },
+        };
+
+        ((PageHeaderData *)(bufPtr(buffer) + (PG_PAGE_SIZE_DEFAULT * 0x00)))->pd_checksum = pgPageChecksum(
+            bufPtr(buffer) + (PG_PAGE_SIZE_DEFAULT * 0x00), 0);
+
+        write = ioBufferWriteNew(bufferOut);
+
+        ioFilterGroupAdd(
+            ioWriteFilterGroup(write),
+            pageChecksumNewVar(varVarLst(jsonToVar(strNewFmt("[0,%u,%" PRIu64 "]", PG_SEGMENT_PAGE_DEFAULT, 0xFACEFACE00000000)))));
+        ioWriteOpen(write);
+        ioWrite(write, buffer);
+        ioWriteClose(write);
+
+        TEST_RESULT_STR_Z(
+            jsonFromVar(ioFilterGroupResult(ioWriteFilterGroup(write), PAGE_CHECKSUM_FILTER_TYPE_STR)),
+            "{\"align\":true,\"valid\":true}", "single valid page");
+
         // Single checksum error
         // -------------------------------------------------------------------------------------------------------------------------
         buffer = bufNew(PG_PAGE_SIZE_DEFAULT * 1);

diff --git a/test/src/module/postgres/interfaceTest.c b/test/src/module/postgres/interfaceTest.c
@@ -178,6 +178,19 @@ testRun(void)
         TEST_RESULT_STR_Z(pgXactPath(PG_VERSION_10), "pg_xact", "check pg_xact name");
     }
 
+    // *****************************************************************************************************************************
+    if (testBegin("pgPageChecksum()"))
+    {
+        unsigned char page[PG_PAGE_SIZE_DEFAULT];
+
+        // Checksum for 0xFF fill, page 0x00
+        memset(page, 0xFF, PG_PAGE_SIZE_DEFAULT);
+        TEST_RESULT_U16_HEX(pgPageChecksum(page, 0), 0x0E1C, "check for 0xFF filled page, block 0");
+
+        // Checksum for 0xFF fill, page 0xFF
+        TEST_RESULT_U16_HEX(pgPageChecksum(page, 999), 0x0EC3, "check for 0xFF filled page, block 999");
+    }
+
     // *****************************************************************************************************************************
     if (testBegin("pgWalFromBuffer() and pgWalFromFile()"))
     {