On Tue, 2013-04-23 at 01:08 +0300, Ants Aasma wrote:
> A slight delay, but here it is. I didn't lift the checksum part into a
> separate file as I didn't have a great idea what I would call it. The
> code is reasonably compact so I don't see a great need for this right
> now. It would be more worth the effort when/if we add non-generic
> variants. I'm not particularly attached to the method I used to mask
> out pd_checksum field, this could be improved if someone has a better
> idea how to structure the code.
Thank you. A few initial comments:
I have attached (for illustration purposes only) a patch on top of yours
that divides the responsibilities a little more cleanly.
* easier to move into a separate file, and use your recommended compiler
flags without affecting other routines in bufpage.c
* makes the checksum algorithm itself simpler
* leaves the data-page-specific aspects (mixing in the page number,
ignoring pd_checksum, reducing to 16 bits) to PageCalcChecksum16
* overall easier to review and understand
I'm not sure what we should call the separate file or where we should
put it, though. How about src/backend/utils/checksum/checksum_fnv.c? Is
there a clean way to override the compiler flags for a single file so we
don't need to put it in its own directory?
Regards,
Jeff Davis
*** a/src/backend/storage/page/bufpage.c
--- b/src/backend/storage/page/bufpage.c
***************
*** 23,28 **** static char pageCopyData[BLCKSZ]; /* for checksum calculation */
--- 23,29 ----
static Page pageCopy = pageCopyData;
static uint16 PageCalcChecksum16(Page page, BlockNumber blkno);
+ static uint32 checksum_fnv(char *data, uint32 size);
/* ----------------------------------------------------------------
* Page support functions
***************
*** 986,1017 **** static const uint32 checksumBaseOffsets[N_SUMS] = {
static uint16
PageCalcChecksum16(Page page, BlockNumber blkno)
{
! uint32 sums[N_SUMS];
! uint32 (*pageArr)[N_SUMS] = (uint32 (*)[N_SUMS]) page;
! uint32 result = blkno;
! int i, j;
! int pd_checksum_word = offsetof(PageHeaderData, pd_checksum)/sizeof(uint32);
! int pd_checksum_half = (offsetof(PageHeaderData, pd_checksum) % sizeof(uint32)) / sizeof(uint16);
/* only calculate the checksum for properly-initialized pages */
Assert(!PageIsNew(page));
/* initialize partial checksums to their corresponding offsets */
memcpy(sums, checksumBaseOffsets, sizeof(checksumBaseOffsets));
! /* first iteration needs to mask out pd_checksum field itself with zero */
! for (j = 0; j < N_SUMS; j++)
! {
! uint32 value = pageArr[0][j];
! if (j == pd_checksum_word)
! ((uint16*) &value)[pd_checksum_half] = 0;
! CHECKSUM_COMP(sums[j], value);
! }
!
! /* now add in the rest of the page */
! for (i = 1; i < BLCKSZ/sizeof(uint32)/N_SUMS; i++)
for (j = 0; j < N_SUMS; j++)
! CHECKSUM_COMP(sums[j], pageArr[i][j]);
/* finally add in one round of zeroes for one more layer of mixing */
for (j = 0; j < N_SUMS; j++)
--- 987,1035 ----
static uint16
PageCalcChecksum16(Page page, BlockNumber blkno)
{
! PageHeader phdr = (PageHeader) page;
! uint16 save_checksum;
! uint32 fnv_checksum;
/* only calculate the checksum for properly-initialized pages */
Assert(!PageIsNew(page));
+ /*
+ * Save pd_checksum and set it to zero, so that the checksum calculation
+ * isn't affected by the checksum stored on the page.
+ */
+ save_checksum = phdr->pd_checksum;
+ phdr->pd_checksum = 0;
+ fnv_checksum = checksum_fnv(page, BLCKSZ);
+ phdr->pd_checksum = save_checksum;
+
+ /* mix in the block number to detect transposed pages */
+ fnv_checksum ^= blkno;
+
+ /*
+ * Reduce to a uint16 (to fit in the pd_checksum field) with an offset of
+ * one. That avoids checksums of zero, which seems like a good idea.
+ */
+ return (fnv_checksum % 65535) + 1;
+ }
+
+ static uint32
+ checksum_fnv(char *data, uint32 size)
+ {
+ uint32 sums[N_SUMS];
+ uint32 (*dataArr)[N_SUMS] = (uint32 (*)[N_SUMS]) data;
+ uint32 result;
+ int i, j;
+
+ Assert((size % (sizeof(uint32)*N_SUMS)) == 0);
+
/* initialize partial checksums to their corresponding offsets */
memcpy(sums, checksumBaseOffsets, sizeof(checksumBaseOffsets));
! /* main checksum calculation */
! for (i = 0; i < size/sizeof(uint32)/N_SUMS; i++)
for (j = 0; j < N_SUMS; j++)
! CHECKSUM_COMP(sums[j], dataArr[i][j]);
/* finally add in one round of zeroes for one more layer of mixing */
for (j = 0; j < N_SUMS; j++)
***************
*** 1021,1026 **** PageCalcChecksum16(Page page, BlockNumber blkno)
for (i = 0; i < N_SUMS; i++)
result ^= sums[i];
! /* use mod mapping to map the value into 16 bits and offset from zero */
! return (result % 65535) + 1;
}
--- 1039,1043 ----
for (i = 0; i < N_SUMS; i++)
result ^= sums[i];
! return result;
}
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
