Re: Change GUC hashtable to use simplehash?

Hi!

On 29.01.2025 10:02, John Naylor wrote:
> This is done -- thanks for the report, and for testing.

It's good that this is done! But i still see the problem.

At ecb8226a in master with the same configure as in [1]
(with asserts) valgrind gives:
==00:00:00:23.937 285792== Conditional jump or move depends on uninitialised value(s)
==00:00:00:23.937 285792== at 0x3084A3: fasthash_accum (hashfn_unstable.h:142)
==00:00:00:23.937 285792== by 0x3084A3: fasthash_accum_cstring_aligned (hashfn_unstable.h:299)
==00:00:00:23.937 285792== by 0x3084A3: fasthash_accum_cstring (hashfn_unstable.h:323)
==00:00:00:23.937 285792== by 0x3084A3: spcachekey_hash (namespace.c:268)
==00:00:00:23.937 285792== by 0x308A3C: nsphash_lookup (simplehash.h:836)
==00:00:00:23.937 285792== by 0x308A3C: spcache_insert (namespace.c:394)
==00:00:00:23.937 285792== by 0x3095D9: cachedNamespacePath (namespace.c:4251)
==00:00:00:23.937 285792== by 0x3096C2: recomputeNamespacePath (namespace.c:4309)
==00:00:00:23.937 285792== by 0x3097B3: RelnameGetRelid (namespace.c:890)
==00:00:00:23.937 285792== by 0x30AF72: RangeVarGetRelidExtended (namespace.c:539)
==00:00:00:23.937 285792== by 0x2F0738: objectNamesToOids (aclchk.c:701)
==00:00:00:23.937 285792== by 0x2F5EFF: ExecuteGrantStmt (aclchk.c:425)
==00:00:00:23.937 285792== by 0x65E2E7: ProcessUtilitySlow (utility.c:1812)
==00:00:00:23.937 285792== by 0x65CCB2: standard_ProcessUtility (utility.c:969)
==00:00:00:23.937 285792== by 0x65CF84: ProcessUtility (utility.c:523)
==00:00:00:23.937 285792== by 0x65A426: PortalRunUtility (pquery.c:1152)

Please see backtrace at bt-with-asserts-Og.txt attached.

Without asserts it falls in similar way:
==00:00:00:23.391 271086== Conditional jump or move depends on uninitialised value(s)
==00:00:00:23.391 271086== at 0x2BE8FE: fasthash_accum (hashfn_unstable.h:180)
==00:00:00:23.391 271086== by 0x2BE8FE: fasthash_accum_cstring_aligned (hashfn_unstable.h:299)
==00:00:00:23.391 271086== by 0x2BE8FE: fasthash_accum_cstring (hashfn_unstable.h:323)
==00:00:00:23.391 271086== by 0x2BE8FE: spcachekey_hash (namespace.c:268)

See bt-wo-asserts-Og.txt

In addition the -O0 build with asserts gives an error in the
pg_rightmost_one_pos64(), not in the fasthash_accum():
==00:00:00:16.360 100422== at 0x3424D6: pg_rightmost_one_pos64 (pg_bitutils.h:148)
==00:00:00:16.360 100422== by 0x342909: fasthash_accum_cstring_aligned (hashfn_unstable.h:298)
==00:00:00:16.360 100422== by 0x3429AB: fasthash_accum_cstring (hashfn_unstable.h:323)
==00:00:00:16.360 100422== by 0x342AFC: spcachekey_hash (namespace.c:268)
==00:00:00:16.360 100422== by 0x3437C2: nsphash_lookup (simplehash.h:836)

See bt-with-asserts-O0.txt, please. It is clear as "word" contains 5 undefined bytes.
(Maybe compiler swallowed this line in -Og build.)
The previous two cases not so clear because valgrind decided that
the "remainder" in the fasthash_accum_cstring_aligned() was undefined.
It thinks that if the argument of __builtin_ctzl() contains some
undefined bytes then the result will be undefined although all rightmost
bits and the first non-zero bit are located in the defined bytes.
On the other hand the presence of the line "Assert(word != 0);"
in the pg_rightmost_one_pos64() already appears to be a
valid reason to use a valgrind-safe solution that will
not allow any undefined bits in its argument.

Besides i found that in the case described above it takes 214
asm instructions to perform "call 0xXXX <spcachekey_hash>" for
existing implementation in the master while the patch from [2]
was a bit faster - 211 instructions.
So i rebased it on the current master and kindly ask to take
it into account as well.

With the best regards,

--
Anton A. Melnikov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

[1] https://www.postgresql.org/message-id/a3a959f6-14b8-4819-ac04-eaf2aa2e868d%40postgrespro.ru

[2] https://www.postgresql.org/message-id/0647027b-9c9a-4f16-8f7c-3f9f3eb9451e%40postgrespro.ru