Message-ID: <62fbf768-d2d0-4bb2-840d-a8d2880af82b@postgrespro.ru>
Date: Mon, 3 Feb 2025 10:45:49 +0300
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: Optimize scram_SaltedPassword performance
To: Zixuan Fu <r33s3n6@gmail.com>, pgsql-hackers@lists.postgresql.org
References: <Z6BsNAadbID+k2L4@bass1>
Content-Language: en-US
From: Yura Sokolov <y.sokolov@postgrespro.ru>
In-Reply-To: <Z6BsNAadbID+k2L4@bass1>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Archived-At: <https://www.postgresql.org/message-id/62fbf768-d2d0-4bb2-840d-a8d2880af82b%40postgrespro.ru>
Precedence: bulk

03.02.2025 10:11, Zixuan Fu пишет:
> Hi Hackers,  
> 
> While profiling a program with `perf`, I noticed that `scram_SaltedPassword`
> consumed more CPU time than expected. After some investigation, I found
> that the function performs many HMAC iterations (4096 rounds for
> SCRAM-SHA-256), and each iteration reinitializes the HMAC context, causing
> excessive overhead.
> 
> OpenSSL has an optimization for this case: when the key remains the
> same, the HMAC context can be reused with a lightweight state reset by
> passing NULL as the key. To take advantage of this, I introduced
> `pg_hmac_reuse()`, which replaces the key with NULL when OpenSSL is used.

Good catch.

Since pg_hmac_reuse is not `static`, I'd add some checks that key is
exactly same. At least there should be

   Assert(key == prev_key && len == prev_len && hash_bytes(key, len) ==
prev_hash);

Where `prev_key`, `prev_len` and `prev_hash` are static variables, filled
in `pg_hmac_init`.

I don't know, should it be `Assert`, or check that leads to `elog(ERROR)`.

`hash_bytes` is fast enough to not cause measurable slow down in production.

On the other hand, use cases are trivial enough to occasional misuses to be
caught using just `Assert`.

> With this change, the performance improves by approximately **4x** (reducing
> execution time from 4ms to 1ms). The built-in PostgreSQL HMAC implementation
> does not support context reuse, and modifying it would require substantial
> changes. Therefore, `pg_hmac_reuse()` simply calls `pg_hmac_init()` in that
> case, maintaining the existing logic.

-------

regards,
Yura