MIME-Version: 1.0
Date: Mon, 20 Oct 2025 18:05:12 +0300
From: Oleg Tselebrovskiy <o.tselebrovskiy@postgrespro.ru>
To: Andrew Kim <tenistarkim@gmail.com>
Cc: John Naylor <johncnaylorls@gmail.com>,
 pgsql-hackers@lists.postgresql.org
Subject: Re: Proposal for enabling auto-vectorization for checksum
 calculations
In-Reply-To: 
 <CAK64mnd6VAtTbar=QS4sbWJpfFxjksjg2ERNkGw1tRwh_kc6mw@mail.gmail.com>
References: <20250911054220.3784-1-root@ip-172-31-36-228.ec2.internal>
 <CANWCAZYZQw-nzTXbx3Bk332VtY9_D7ksDsuMZ0A-iDZ53yG7Ng@mail.gmail.com>
 <CAK64mnfeWLBRbMfnOsag0vGTDnT84KJzpuei40nG0OHyw4SESw@mail.gmail.com>
 <CANWCAZa1b2rcvoK657SmcKwh2P2cgASQ1D-0JPj5d3LbfaAVgA@mail.gmail.com>
 <CAK64mneN20+sW5WhV+r7hMVo4Rd0z11B6=3L039rWMt1wK3nPg@mail.gmail.com>
 <d023881ae837e0e116d8645c89e319da@postgrespro.ru>
 <CAK64mnd6VAtTbar=QS4sbWJpfFxjksjg2ERNkGw1tRwh_kc6mw@mail.gmail.com>
Message-ID: <44d2472a978912ca352eb839a24b2176@postgrespro.ru>
Organization: Postgres Pro
Content-Type: text/plain; charset=UTF-8;
 format=flowed
Content-Transfer-Encoding: 8bit
Archived-At: 
 <https://www.postgresql.org/message-id/44d2472a978912ca352eb839a24b2176%40postgrespro.ru>
Precedence: bulk

Thanks for the new patch version!

Another round of review:

1) I think that changes to contrib/pageinspect/rawpage.c should be in 
the main patch, not in the benchmark patch. Also, without those chages 
the main patch can't compile using make world-bin

2) I still don't get why you check for working intrinsics in configure, 
config/c-compiler.m4 and meson.build, if your patch later uses them. 
I've gotten correct assembly code with this avx2_test function:
     #include <stdint.h>
     #if defined(__has_attribute) && __has_attribute (target)
     __attribute__((target("avx2")))
     static int avx2_test(void)
     {
		return 0;
     }
     #endif
Please, check if this works for you and consider using something similar

3) __builtin_cpu_supports doesn't work on Windows at all. We still have 
to use approach with __get_cpuid

4) Looks like you can safely remove "port/checksum_impl.h" from 
src/bin/pg_checksums/pg_checksums.c. It probably links with libpgport 
and/or libpgcommon, so it gets pg_checksum_page from there. Same with 
src/bin/pg_upgrade/file.c. Maybe those includes are "for clarity" and 
you don't need to remove them, but pg_checksums and pg_upgrade seem to 
work without them

5) You don't need #include <string.h> /* for memcpy */ in 
checksum_impl.h. At the very least, memcpy was used before your patch 
without string.h

6) Why did you remove Assert(sizeof(PGChecksummablePage) == BLCKSZ)? Is 
it always false?

7) Is reformatted variable declaration in pg_checksum_block_default_impl 
really needed? Is there a good reason for it? Or is it auto-formatting 
programm output?

8) Your patch removes one whitespace in this line - for (i = 0; i < 
(uint32)(BLCKSZ / (sizeof(uint32) * N_SUMS)); i++)
If you wish to fix formatting like that - please, do it in a separate 
patch. If this was done automatically by some formatting tool - please, 
revert this change

9) Unneeded empty string
     #define FNV_PRIME 16777619

     +
      /* Use a union so that this code is valid under strict aliasing */
      typedef union

10) You need one line with just /* at the beginning of the comment, look 
at other multiline comments in this file
+	/* For now, AVX2 implementation is identical to default
+	 * The compiler will auto-vectorize this with proper flags
+	 * Future versions could use explicit AVX2 intrinsics here
  	 */

11) Function pg_checksum_block_simple isn't used at all.

12) Why do you need those?
+#ifndef PG_CHECKSUM_EXTERNAL_INTERFACE
+#define PG_CHECKSUM_EXTERNAL_INTERFACE

13) Object files are added according to alphabetical order, not logical 
order (src/port/Makefile)
  	pg_popcount_aarch64.o \
  	pg_popcount_avx512.o \
+	checksum.o \
  	pg_strong_random.o \
  	pgcheckdir.o \

14) I still think that src/port/checksum.c needs to just include 
src/include/port/checksum_impl.h and have no other logic to keep 
checksum_impl.h's role as "header with full implementation"
Now checksum_impl.h doesn't have any mention of pg_checksum_page

15) Assembly for pg_checksum_block_choose now has full code of 
pg_checksum_block_default. This is probably a result of using inline 
functions
Don't know if this is bad, but it is at least strange

Also, some CFBot checks have failed. Two of them with this error/warning
   checksum.c:88:1: error: no previous prototype for ‘pg_checksum_page’ 
[-Werror=missing-prototypes]
      88 | pg_checksum_page(char *page, BlockNumber blkno)
         | ^~~~~~~~~~~~~~~~
Please, address those

Oleg Tselebrovskiy, PostgresPro