centralize CPU feature detection

Hello!

Seems like a file (checksum_block_internal.h) is missing from the patch?

For the entire src/include/port/pg_x86_feature.h:

Shouldn't it have an

+#if defined(USE_SSE2) || defined(__i386__)
...
#endif

block around the file, to skip everything on other platforms?

In src/include/port/pg_x86_feature.h:33

+
+extern PGDLLEXPORT bool X86Feature[];
+

Shouldn't that be PGDLLIMPORT?

+typedef enum X86FeatureId
+{
+ init,
+
+ PG_SSE4_2,
+ PG_POPCNT,

Shouldn't that be INIT?

On Tue, Feb 17, 2026 at 3:15 AM Zsolt Parragi <zsolt.parragi@percona.com> wrote:

Hello!

Seems like a file (checksum_block_internal.h) is missing from the patch?

Should work now, and thanks for looking.

For the entire src/include/port/pg_x86_feature.h:

Shouldn't it have an

+#if defined(USE_SSE2) || defined(__i386__)
...
#endif

block around the file, to skip everything on other platforms?

Done. I haven't tried Arm support yet, but now I realize the header
should be named generically, so it's now "pg_cpu.h". Then it can be
included everywhere.

I've also gone with "pg_cpu_x86.c" for better consistency within this
directory, and used the plural for the array name.

In src/include/port/pg_x86_feature.h:33

+
+extern PGDLLEXPORT bool X86Feature[];
+

Shouldn't that be PGDLLIMPORT?

Fixed.

+typedef enum X86FeatureId
+{
+ init,
+
+ PG_SSE4_2,
+ PG_POPCNT,

Shouldn't that be INIT?

I don't know. The instruction family names are conventionally all in
caps, but this is just our signal that we've populated the array. That
said, a less generic name would better for grep-ability.

I added some quick comments here where the instruction families are
split apart. I'm not sure what info is relevent, but it seemed good to
separate them.

--
John Naylor
Amazon Web Services

v3 removes some debug code that was causing CI to fail.

--
John Naylor
Amazon Web Services

Done. I haven't tried Arm support yet, but now I realize the header
should be named generically, so it's now "pg_cpu.h". Then it can be
included everywhere.

That makes sense, and simplifies the usage of the header. (However,
the include guard still refers to the old name)

I don't know. The instruction family names are conventionally all in
caps, but this is just our signal that we've populated the array. That
said, a less generic name would better for grep-ability.

Yes, that could work too. But reserving the lowercase "init" symbol in
a very generic header seems like a bad idea (especially for a use case
that isn't used globally), even if Postgres itself doesn't use the
symbol for anything else. "INIT" at least would be unlikely to
conflict with something else.

On Thu, Feb 19, 2026 at 1:47 AM Zsolt Parragi <zsolt.parragi@percona.com> wrote:

Done. I haven't tried Arm support yet, but now I realize the header
should be named generically, so it's now "pg_cpu.h". Then it can be
included everywhere.

That makes sense, and simplifies the usage of the header. (However,
the include guard still refers to the old name)

Oops, fixed.

I don't know. The instruction family names are conventionally all in
caps, but this is just our signal that we've populated the array. That
said, a less generic name would better for grep-ability.

Yes, that could work too. But reserving the lowercase "init" symbol in
a very generic header seems like a bad idea (especially for a use case
that isn't used globally), even if Postgres itself doesn't use the
symbol for anything else. "INIT" at least would be unlikely to
conflict with something else.

Still seems pretty generic, so I went with INIT_PG_X86.

I've also made a quick attempt at Arm support just to make sure I
didn't paint myself into a corner (v4-0005-6), and it compiles and
passes tests on a Debian aarch64 system with gcc 8.3. I'll put that
aside for later. v4-0001-3 are still the main focus now, and seem in
decent shape, maybe needs a bit more polish. (not to mention formal
commit messages)

--
John Naylor
Amazon Web Services

1-3 looks good to me, other than the need for the proper commit messages.

There's a typo in " Are ZMM registeres enabled?" in 3.

On Fri, Feb 20, 2026 at 3:26 PM Zsolt Parragi <zsolt.parragi@percona.com> wrote:

1-3 looks good to me, other than the need for the proper commit messages.

I've committed 0001.

There's a typo in " Are ZMM registeres enabled?" in 3.

Fixed. 0002 and 0003 are attached with draft commit messages. There
was also a cosmetic mistake in an enum member name, whose correction
was squashed in the wrong direction -- this has been fixed. I also
added the new typedef for pgindent and restored a lost comment for
pg_comp_crc32c_choose(). This seems committable, but will double-check
everything works correctly.

--
John Naylor
Amazon Web Services

2 and 3 looks good too, I only found two more typos:

+ return pg_comp_crc32c(crc, data, len);
+};

That semicolon is not needed

And in the commit message:

"it has been intialized and if"

That should be initialized

John Naylor <johncnaylorls@gmail.com> writes:

I've committed 0001.

BF animal rhinoceros isn't happy. I can reproduce that locally by
doing

$ ./configure ... USE_SLICING_BY_8_CRC32C=1
$ make
...
pg_cpu_x86.c: In function 'pg_comp_crc32c_choose':
pg_cpu_x86.c:85:3: error: 'pg_comp_crc32c' undeclared (first use in this function); did you mean 'pg_comp_crc32c_sb8'?
pg_comp_crc32c = pg_comp_crc32c_sse42;
^~~~~~~~~~~~~~
pg_comp_crc32c_sb8
pg_cpu_x86.c:85:3: note: each undeclared identifier is reported only once for each function it appears in
pg_cpu_x86.c:85:20: error: 'pg_comp_crc32c_sse42' undeclared (first use in this function); did you mean 'pg_comp_crc32c_sb8'?
pg_comp_crc32c = pg_comp_crc32c_sse42;
^~~~~~~~~~~~~~~~~~~~
pg_comp_crc32c_sb8
pg_cpu_x86.c:108:9: warning: implicit declaration of function 'pg_comp_crc32c'; did you mean 'pg_comp_crc32c_sb8'? [-Wimplicit-function-declaration]
return pg_comp_crc32c(crc, data, len);
^~~~~~~~~~~~~~
pg_comp_crc32c_sb8

It appears that if you want to build pg_cpu_x86.o unconditionally,
you need to make it more proof against the cases it wasn't getting
built in before.

regards, tom lane

On Wed, Feb 25, 2026 at 2:59 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

It appears that if you want to build pg_cpu_x86.o unconditionally,
you need to make it more proof against the cases it wasn't getting
built in before.

Thanks, I must have stopped watching the buildfarm too early. I've
pushed a fix which will get undone as part of v6-0002.

On Wed, Feb 25, 2026 at 2:57 AM Zsolt Parragi <zsolt.parragi@percona.com> wrote:

2 and 3 looks good too, I only found two more typos:

+ return pg_comp_crc32c(crc, data, len);
+};

That semicolon is not needed

And in the commit message:

"it has been intialized and if"

That should be initialized

Also fixed, thanks.

--
John Naylor
Amazon Web Services

Both look good to me.

This isn't part of the patch, and it seems harmless, but while
reviewing the CRC functions, I noticed that pg_crc32c.h is
inconsistent with its dllimport markers, pg_comp_crc32c has 3
different declarations, and only 1 of them is marked PGDLLIMPORT.

On Wed, Feb 25, 2026 at 8:09 PM Zsolt Parragi <zsolt.parragi@percona.com> wrote:

Both look good to me.

Pushed 0002 after making sure AVX-512 detection still worked, thanks
for the review! I think 0003 needs a link to the Intel manual for the
XCR symbol values, and I'll push shortly after I add that.

This isn't part of the patch, and it seems harmless, but while
reviewing the CRC functions, I noticed that pg_crc32c.h is
inconsistent with its dllimport markers, pg_comp_crc32c has 3
different declarations, and only 1 of them is marked PGDLLIMPORT.

Yeah, I think that crept in during development to keep Windows CI
building with a not-for-commit test module. I'll remove it soon.

--
John Naylor
Amazon Web Services