public inbox for [email protected]help / color / mirror / Atom feed
[PATCH v7 5/7] Invalidate parent index cluster on attach 4+ messages / 4 participants [nested] [flat]
* [PATCH v7 5/7] Invalidate parent index cluster on attach @ 2020-11-06 01:11 Justin Pryzby <[email protected]> 0 siblings, 0 replies; 4+ messages in thread From: Justin Pryzby @ 2020-11-06 01:11 UTC (permalink / raw) --- src/backend/commands/indexcmds.c | 21 +++++++++++++++++++++ src/test/regress/expected/cluster.out | 14 ++++++++++++++ src/test/regress/sql/cluster.sql | 5 +++++ 3 files changed, 40 insertions(+) diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c index 4ca1ffbfa4..cc57c149ed 100644 --- a/src/backend/commands/indexcmds.c +++ b/src/backend/commands/indexcmds.c @@ -4117,6 +4117,27 @@ IndexSetParentIndex(Relation partitionIdx, Oid parentOid) /* set relispartition correctly on the partition */ update_relispartition(partRelid, OidIsValid(parentOid)); + /* + * If the attached index is not clustered, invalidate cluster mark on + * any parents + */ + if ((OidIsValid(parentOid) && get_index_isclustered(parentOid)) || + get_index_isclustered(partRelid)) + { + Relation indrel; + + /* Make relispartition visible */ + CommandCounterIncrement(); + + indrel = table_open(IndexGetRelation(partRelid, false), + ShareUpdateExclusiveLock); + mark_index_clustered(indrel, + get_index_isclustered(partRelid) ? partRelid : InvalidOid, + true); + table_close(indrel, ShareUpdateExclusiveLock); + + } + if (fix_dependencies) { /* diff --git a/src/test/regress/expected/cluster.out b/src/test/regress/expected/cluster.out index 6cba3cc4f9..e7f0889743 100644 --- a/src/test/regress/expected/cluster.out +++ b/src/test/regress/expected/cluster.out @@ -567,6 +567,20 @@ Indexes: "clstrpart_idx" btree (a) Number of partitions: 3 (Use \d+ to list them.) +-- Check that attaching an unclustered index marks the parent unclustered: +ALTER TABLE clstrpart CLUSTER ON clstrpart_idx; +CREATE TABLE clstrpart5 (LIKE clstrpart INCLUDING INDEXES); +ALTER TABLE clstrpart ATTACH PARTITION clstrpart5 FOR VALUES FROM (40)TO(50); +\d clstrpart + Partitioned table "public.clstrpart" + Column | Type | Collation | Nullable | Default +--------+---------+-----------+----------+--------- + a | integer | | | +Partition key: RANGE (a) +Indexes: + "clstrpart_idx" btree (a) +Number of partitions: 4 (Use \d+ to list them.) + -- Test CLUSTER with external tuplesorting create table clstr_4 as select * from tenk1; create index cluster_sort on clstr_4 (hundred, thousand, tenthous); diff --git a/src/test/regress/sql/cluster.sql b/src/test/regress/sql/cluster.sql index a1d132f288..3c8085c69e 100644 --- a/src/test/regress/sql/cluster.sql +++ b/src/test/regress/sql/cluster.sql @@ -239,6 +239,11 @@ CLUSTER clstrpart1 USING clstrpart1_idx_2; ALTER TABLE clstrpart CLUSTER ON clstrpart_idx; ALTER TABLE clstrpart1 SET WITHOUT CLUSTER; \d clstrpart +-- Check that attaching an unclustered index marks the parent unclustered: +ALTER TABLE clstrpart CLUSTER ON clstrpart_idx; +CREATE TABLE clstrpart5 (LIKE clstrpart INCLUDING INDEXES); +ALTER TABLE clstrpart ATTACH PARTITION clstrpart5 FOR VALUES FROM (40)TO(50); +\d clstrpart -- Test CLUSTER with external tuplesorting -- 2.17.0 --AA9g+nFNFPYNJKiL Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="v7-0006-Preserve-indisclustered-on-children-of-clustered-.patch" ^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: Optimize scram_SaltedPassword performance @ 2025-02-03 07:45 Yura Sokolov <[email protected]> 0 siblings, 1 reply; 4+ messages in thread From: Yura Sokolov @ 2025-02-03 07:45 UTC (permalink / raw) To: Zixuan Fu <[email protected]>; [email protected] 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 ^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: Optimize scram_SaltedPassword performance @ 2025-02-03 11:17 Daniel Gustafsson <[email protected]> parent: Yura Sokolov <[email protected]> 0 siblings, 1 reply; 4+ messages in thread From: Daniel Gustafsson @ 2025-02-03 11:17 UTC (permalink / raw) To: Zixuan Fu <[email protected]>; +Cc: Yura Sokolov <[email protected]>; PostgreSQL Hackers <[email protected]> > On 3 Feb 2025, at 08:45, Yura Sokolov <[email protected]> wrote: > > 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. While I don't disagree with speeding up this in general, the whole point of the SCRAM iterations is to take a lot of time as a way to combat brute forcing. Any attacker is likely to use a patched libpq (or not use libpq at all) so clientside it doesn't matter as much but if we make it 4x faster serverside we don't really achieve much other than making attacks more feasible. The relevant portion from RFC 7677 §4 is: The strength of this mechanism is dependent in part on the hash iteration-count, as denoted by "i" in [RFC5802]. As a rule of thumb, the hash iteration-count should be such that a modern machine will take 0.1 seconds to perform the complete algorithm; however, this is unlikely to be practical on mobile devices and other relatively low- performance systems. At the time this was written, the rule of thumb gives around 15,000 iterations required; however, a hash iteration- count of 4096 takes around 0.5 seconds on current mobile handsets. This computational cost can be avoided by caching the ClientKey (assuming the Salt and hash iteration-count is stable). Therefore, the recommendation of this specification is that the hash iteration- count SHOULD be at least 4096, but careful consideration ought to be given to using a significantly higher value, particularly where mobile use is less important. The numbers are quite outdated but the gist of it holds. If we speed it up serverside we need to counter that with a higher iteration count, and for a rogue client it's unlikely to matter since it wont use our code anyways. Speeding up HMAC for other usecases is a different story (but also likely to have less measurable performance impact). >> 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. > Where `prev_key`, `prev_len` and `prev_hash` are static variables, filled > in `pg_hmac_init`. Storing any part of a cryptograhic calculation, let alone a key, in a static variable doesn't seem entirely like a best practice, and it also wont be threadsafe. -- Daniel Gustafsson ^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: Optimize scram_SaltedPassword performance @ 2025-02-04 00:47 Michael Paquier <[email protected]> parent: Daniel Gustafsson <[email protected]> 0 siblings, 0 replies; 4+ messages in thread From: Michael Paquier @ 2025-02-04 00:47 UTC (permalink / raw) To: Daniel Gustafsson <[email protected]>; +Cc: Zixuan Fu <[email protected]>; Yura Sokolov <[email protected]>; PostgreSQL Hackers <[email protected]> On Mon, Feb 03, 2025 at 12:17:27PM +0100, Daniel Gustafsson wrote: > The numbers are quite outdated but the gist of it holds. If we speed it up > serverside we need to counter that with a higher iteration count, and for a > rogue client it's unlikely to matter since it wont use our code anyways. > Speeding up HMAC for other usecases is a different story (but also likely to > have less measurable performance impact). Thanks, I've managed to somewhat forget this point. Note that this has come up as well when adding the GUC scram_iterations in b577743000cd, where a lower count can be used for a faster connection. > Storing any part of a cryptograhic calculation, let alone a key, in a static > variable doesn't seem entirely like a best practice, and it also wont be > threadsafe. Agreed. By the way, something I have forgotten to mention yesterday.. If a fast-path gets implemented in these HMAC APIs (perhaps not), it sounds to me that it would be better to achieve the same for the fallback non-OpenSSL implementation in src/common/hmac.c, as it would reflect things properly in the interface. My 2c. -- Michael Attachments: [application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc) download ^ permalink raw reply [nested|flat] 4+ messages in thread
end of thread, other threads:[~2025-02-04 00:47 UTC | newest] Thread overview: 4+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2020-11-06 01:11 [PATCH v7 5/7] Invalidate parent index cluster on attach Justin Pryzby <[email protected]> 2025-02-03 07:45 Re: Optimize scram_SaltedPassword performance Yura Sokolov <[email protected]> 2025-02-03 11:17 ` Re: Optimize scram_SaltedPassword performance Daniel Gustafsson <[email protected]> 2025-02-04 00:47 ` Re: Optimize scram_SaltedPassword performance Michael Paquier <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox