Horton

Re: Change GUC hashtable to use simplehash?

Anton A. Melnikov <a.melnikov@postgrespro.ru>

View thread
From: "Anton A. Melnikov" <a.melnikov@postgrespro.ru>
To: John Naylor <johncnaylorls@gmail.com>
Cc: Tom Lane <tgl@sss.pgh.pa.us>, Jeff Davis <pgsql@j-davis.com>, Ants Aasma <ants.aasma@cybertec.at>, Heikki Linnakangas <hlinnaka@iki.fi>, Junwang Zhao <zhjwpku@gmail.com>, jian he <jian.universality@gmail.com>, Andres Freund <andres@anarazel.de>, Gurjeet Singh <gurjeet@singh.im>, pgsql-hackers@postgresql.org
Date: 2025-02-12T20:42:32Z
Lists: pgsql-hackers

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Silence warning in older versions of Valgrind

  2. Revert "Speed up tail processing when hashing aligned C strings, take two"

  3. Speed up tail processing when hashing aligned C strings, take two

  4. Teach fasthash_accum to use platform endianness for bytewise loads

  5. Add macro to disable address safety instrumentation

  6. Convert uses of hash_string_pointer to fasthash equivalent

  7. Speed up tail processing when hashing aligned C strings

  8. Add helper functions for dshash tables with string keys.

  9. Fix warnings in cpluspluscheck

  10. Further cosmetic review of hashfn_unstable.h

  11. Simplify initialization of incremental hash state

  12. Add optimized C string hashing

  13. Add inline incremental hash functions for in-memory use

  14. Make all Perl warnings fatal

Attachments

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