Thread: pgsql: Introduce hash_search_with_hash_value() function
Introduce hash_search_with_hash_value() function This new function iterates hash entries with given hash values. This function is designed to avoid full sequential hash search in the syscache invalidation callbacks. Discussion: https://postgr.es/m/5812a6e5-68ae-4d84-9d85-b443176966a1%40sigaev.ru Author: Teodor Sigaev Reviewed-by: Aleksander Alekseev, Tom Lane, Michael Paquier, Roman Zharkov Reviewed-by: Andrei Lepikhov Branch ------ master Details ------- https://git.postgresql.org/pg/commitdiff/d0f020037e19c33c74d683eb7e0c7cc5725294b4 Modified Files -------------- src/backend/utils/hash/dynahash.c | 38 ++++++++++++++++++++++++++++++++++++++ src/include/utils/hsearch.h | 5 +++++ 2 files changed, 43 insertions(+)
Hi
st 7. 8. 2024 v 6:08 odesílatel Alexander Korotkov <akorotkov@postgresql.org> napsal:
Introduce hash_search_with_hash_value() function
This new function iterates hash entries with given hash values. This function
is designed to avoid full sequential hash search in the syscache invalidation
callbacks.
Discussion: https://postgr.es/m/5812a6e5-68ae-4d84-9d85-b443176966a1%40sigaev.ru
Author: Teodor Sigaev
Reviewed-by: Aleksander Alekseev, Tom Lane, Michael Paquier, Roman Zharkov
Reviewed-by: Andrei Lepikhov
I tried to use hash_seq_init_with_hash_value in session variables patch, but it doesn't work there.
<-->if (!sessionvars)
<--><-->return;
elog(NOTICE, "%u", hashvalue);
<-->hash_seq_init(&status, sessionvars);
<-->while ((svar = (SVariable) hash_seq_search(&status)) != NULL)
<-->{
<--><-->if (hashvalue == 0 || svar->hashvalue == hashvalue)
<--><-->{
<--><--><-->elog(NOTICE, "FOUND OLD");
<--><--><-->svar->is_valid = false;
<--><-->}
<-->}
<-->/*
<--> * If the hashvalue is not specified, we have to recheck all currently
<--> * used session variables. Since we can't tell the exact session variable
<--> * from its hashvalue, we have to iterate over all items in the hash bucket.
<--> */
<-->if (hashvalue == 0)
<--><-->hash_seq_init(&status, sessionvars);
<-->else
<--><-->hash_seq_init_with_hash_value(&status, sessionvars, hashvalue);
<-->while ((svar = (SVariable) hash_seq_search(&status)) != NULL)
<-->{
<--><-->Assert(hashvalue == 0 || svar->hashvalue == hashvalue);
elog(NOTICE, "found");
<--><-->svar->is_valid = false;
<--><-->needs_validation = true;
<-->}
}
<--><-->return;
elog(NOTICE, "%u", hashvalue);
<-->hash_seq_init(&status, sessionvars);
<-->while ((svar = (SVariable) hash_seq_search(&status)) != NULL)
<-->{
<--><-->if (hashvalue == 0 || svar->hashvalue == hashvalue)
<--><-->{
<--><--><-->elog(NOTICE, "FOUND OLD");
<--><--><-->svar->is_valid = false;
<--><-->}
<-->}
<-->/*
<--> * If the hashvalue is not specified, we have to recheck all currently
<--> * used session variables. Since we can't tell the exact session variable
<--> * from its hashvalue, we have to iterate over all items in the hash bucket.
<--> */
<-->if (hashvalue == 0)
<--><-->hash_seq_init(&status, sessionvars);
<-->else
<--><-->hash_seq_init_with_hash_value(&status, sessionvars, hashvalue);
<-->while ((svar = (SVariable) hash_seq_search(&status)) != NULL)
<-->{
<--><-->Assert(hashvalue == 0 || svar->hashvalue == hashvalue);
elog(NOTICE, "found");
<--><-->svar->is_valid = false;
<--><-->needs_validation = true;
<-->}
}
Old methods found an entry, but new not.
What am I doing wrong?
Regards
Pavel
Branch
------
master
Details
-------
https://git.postgresql.org/pg/commitdiff/d0f020037e19c33c74d683eb7e0c7cc5725294b4
Modified Files
--------------
src/backend/utils/hash/dynahash.c | 38 ++++++++++++++++++++++++++++++++++++++
src/include/utils/hsearch.h | 5 +++++
2 files changed, 43 insertions(+)
Hi, Pavel! On Wed, Aug 7, 2024 at 9:35 AM Pavel Stehule <pavel.stehule@gmail.com> wrote: > st 7. 8. 2024 v 6:08 odesílatel Alexander Korotkov <akorotkov@postgresql.org> napsal: >> >> Introduce hash_search_with_hash_value() function >> >> This new function iterates hash entries with given hash values. This function >> is designed to avoid full sequential hash search in the syscache invalidation >> callbacks. >> >> Discussion: https://postgr.es/m/5812a6e5-68ae-4d84-9d85-b443176966a1%40sigaev.ru >> Author: Teodor Sigaev >> Reviewed-by: Aleksander Alekseev, Tom Lane, Michael Paquier, Roman Zharkov >> Reviewed-by: Andrei Lepikhov > > > I tried to use hash_seq_init_with_hash_value in session variables patch, but it doesn't work there. > > <-->if (!sessionvars) > <--><-->return; > > elog(NOTICE, "%u", hashvalue); > > > <-->hash_seq_init(&status, sessionvars); > > <-->while ((svar = (SVariable) hash_seq_search(&status)) != NULL) > <-->{ > <--><-->if (hashvalue == 0 || svar->hashvalue == hashvalue) > <--><-->{ > <--><--><-->elog(NOTICE, "FOUND OLD"); > <--><--><-->svar->is_valid = false; > <--><-->} > <-->} > > > > <-->/* > <--> * If the hashvalue is not specified, we have to recheck all currently > <--> * used session variables. Since we can't tell the exact session variable > <--> * from its hashvalue, we have to iterate over all items in the hash bucket. > <--> */ > <-->if (hashvalue == 0) > <--><-->hash_seq_init(&status, sessionvars); > <-->else > <--><-->hash_seq_init_with_hash_value(&status, sessionvars, hashvalue); > > <-->while ((svar = (SVariable) hash_seq_search(&status)) != NULL) > <-->{ > <--><-->Assert(hashvalue == 0 || svar->hashvalue == hashvalue); > > elog(NOTICE, "found"); > > <--><-->svar->is_valid = false; > <--><-->needs_validation = true; > <-->} > } > > Old methods found an entry, but new not. > > What am I doing wrong? I'm trying to check this. Applying this patch [1], but got conflicts. Could you please, rebase the patch, so I can recheck the issue? Links. 1. https://www.postgresql.org/message-id/flat/CAFj8pRD053CY_N4%3D6SvPe7ke6xPbh%3DK50LUAOwjC3jm8Me9Obg%40mail.gmail.com ------ Regards, Alexander Korotkov Supabase
st 7. 8. 2024 v 10:52 odesílatel Alexander Korotkov <aekorotkov@gmail.com> napsal:
Hi, Pavel!
On Wed, Aug 7, 2024 at 9:35 AM Pavel Stehule <pavel.stehule@gmail.com> wrote:
> st 7. 8. 2024 v 6:08 odesílatel Alexander Korotkov <akorotkov@postgresql.org> napsal:
>>
>> Introduce hash_search_with_hash_value() function
>>
>> This new function iterates hash entries with given hash values. This function
>> is designed to avoid full sequential hash search in the syscache invalidation
>> callbacks.
>>
>> Discussion: https://postgr.es/m/5812a6e5-68ae-4d84-9d85-b443176966a1%40sigaev.ru
>> Author: Teodor Sigaev
>> Reviewed-by: Aleksander Alekseev, Tom Lane, Michael Paquier, Roman Zharkov
>> Reviewed-by: Andrei Lepikhov
>
>
> I tried to use hash_seq_init_with_hash_value in session variables patch, but it doesn't work there.
>
> <-->if (!sessionvars)
> <--><-->return;
>
> elog(NOTICE, "%u", hashvalue);
>
>
> <-->hash_seq_init(&status, sessionvars);
>
> <-->while ((svar = (SVariable) hash_seq_search(&status)) != NULL)
> <-->{
> <--><-->if (hashvalue == 0 || svar->hashvalue == hashvalue)
> <--><-->{
> <--><--><-->elog(NOTICE, "FOUND OLD");
> <--><--><-->svar->is_valid = false;
> <--><-->}
> <-->}
>
>
>
> <-->/*
> <--> * If the hashvalue is not specified, we have to recheck all currently
> <--> * used session variables. Since we can't tell the exact session variable
> <--> * from its hashvalue, we have to iterate over all items in the hash bucket.
> <--> */
> <-->if (hashvalue == 0)
> <--><-->hash_seq_init(&status, sessionvars);
> <-->else
> <--><-->hash_seq_init_with_hash_value(&status, sessionvars, hashvalue);
>
> <-->while ((svar = (SVariable) hash_seq_search(&status)) != NULL)
> <-->{
> <--><-->Assert(hashvalue == 0 || svar->hashvalue == hashvalue);
>
> elog(NOTICE, "found");
>
> <--><-->svar->is_valid = false;
> <--><-->needs_validation = true;
> <-->}
> }
>
> Old methods found an entry, but new not.
>
> What am I doing wrong?
I'm trying to check this. Applying this patch [1], but got conflicts.
Could you please, rebase the patch, so I can recheck the issue?
I sent rebased patchset
Regards
Pavel
Links.
1. https://www.postgresql.org/message-id/flat/CAFj8pRD053CY_N4%3D6SvPe7ke6xPbh%3DK50LUAOwjC3jm8Me9Obg%40mail.gmail.com
------
Regards,
Alexander Korotkov
Supabase
On Wed, Aug 7, 2024 at 1:03 PM Pavel Stehule <pavel.stehule@gmail.com> wrote: > > st 7. 8. 2024 v 10:52 odesílatel Alexander Korotkov <aekorotkov@gmail.com> napsal: >> >> Hi, Pavel! >> >> On Wed, Aug 7, 2024 at 9:35 AM Pavel Stehule <pavel.stehule@gmail.com> wrote: >> > st 7. 8. 2024 v 6:08 odesílatel Alexander Korotkov <akorotkov@postgresql.org> napsal: >> >> >> >> Introduce hash_search_with_hash_value() function >> >> >> >> This new function iterates hash entries with given hash values. This function >> >> is designed to avoid full sequential hash search in the syscache invalidation >> >> callbacks. >> >> >> >> Discussion: https://postgr.es/m/5812a6e5-68ae-4d84-9d85-b443176966a1%40sigaev.ru >> >> Author: Teodor Sigaev >> >> Reviewed-by: Aleksander Alekseev, Tom Lane, Michael Paquier, Roman Zharkov >> >> Reviewed-by: Andrei Lepikhov >> > >> > >> > I tried to use hash_seq_init_with_hash_value in session variables patch, but it doesn't work there. >> > >> > <-->if (!sessionvars) >> > <--><-->return; >> > >> > elog(NOTICE, "%u", hashvalue); >> > >> > >> > <-->hash_seq_init(&status, sessionvars); >> > >> > <-->while ((svar = (SVariable) hash_seq_search(&status)) != NULL) >> > <-->{ >> > <--><-->if (hashvalue == 0 || svar->hashvalue == hashvalue) >> > <--><-->{ >> > <--><--><-->elog(NOTICE, "FOUND OLD"); >> > <--><--><-->svar->is_valid = false; >> > <--><-->} >> > <-->} >> > >> > >> > >> > <-->/* >> > <--> * If the hashvalue is not specified, we have to recheck all currently >> > <--> * used session variables. Since we can't tell the exact session variable >> > <--> * from its hashvalue, we have to iterate over all items in the hash bucket. >> > <--> */ >> > <-->if (hashvalue == 0) >> > <--><-->hash_seq_init(&status, sessionvars); >> > <-->else >> > <--><-->hash_seq_init_with_hash_value(&status, sessionvars, hashvalue); >> > >> > <-->while ((svar = (SVariable) hash_seq_search(&status)) != NULL) >> > <-->{ >> > <--><-->Assert(hashvalue == 0 || svar->hashvalue == hashvalue); >> > >> > elog(NOTICE, "found"); >> > >> > <--><-->svar->is_valid = false; >> > <--><-->needs_validation = true; >> > <-->} >> > } >> > >> > Old methods found an entry, but new not. >> > >> > What am I doing wrong? >> >> I'm trying to check this. Applying this patch [1], but got conflicts. >> Could you please, rebase the patch, so I can recheck the issue? > > I sent rebased patchset Thank you. Please see 40064a8ee1 takes special efforts to match HTAB hash function to syscache hash function. By default, their hash values don't match and you can't simply use syscache hash value to search HTAB. This probably should be mentioned in the header comment of hash_seq_init_with_hash_value(). ------ Regards, Alexander Korotkov Supabase
st 7. 8. 2024 v 12:22 odesílatel Alexander Korotkov <aekorotkov@gmail.com> napsal:
On Wed, Aug 7, 2024 at 1:03 PM Pavel Stehule <pavel.stehule@gmail.com> wrote:
>
> st 7. 8. 2024 v 10:52 odesílatel Alexander Korotkov <aekorotkov@gmail.com> napsal:
>>
>> Hi, Pavel!
>>
>> On Wed, Aug 7, 2024 at 9:35 AM Pavel Stehule <pavel.stehule@gmail.com> wrote:
>> > st 7. 8. 2024 v 6:08 odesílatel Alexander Korotkov <akorotkov@postgresql.org> napsal:
>> >>
>> >> Introduce hash_search_with_hash_value() function
>> >>
>> >> This new function iterates hash entries with given hash values. This function
>> >> is designed to avoid full sequential hash search in the syscache invalidation
>> >> callbacks.
>> >>
>> >> Discussion: https://postgr.es/m/5812a6e5-68ae-4d84-9d85-b443176966a1%40sigaev.ru
>> >> Author: Teodor Sigaev
>> >> Reviewed-by: Aleksander Alekseev, Tom Lane, Michael Paquier, Roman Zharkov
>> >> Reviewed-by: Andrei Lepikhov
>> >
>> >
>> > I tried to use hash_seq_init_with_hash_value in session variables patch, but it doesn't work there.
>> >
>> > <-->if (!sessionvars)
>> > <--><-->return;
>> >
>> > elog(NOTICE, "%u", hashvalue);
>> >
>> >
>> > <-->hash_seq_init(&status, sessionvars);
>> >
>> > <-->while ((svar = (SVariable) hash_seq_search(&status)) != NULL)
>> > <-->{
>> > <--><-->if (hashvalue == 0 || svar->hashvalue == hashvalue)
>> > <--><-->{
>> > <--><--><-->elog(NOTICE, "FOUND OLD");
>> > <--><--><-->svar->is_valid = false;
>> > <--><-->}
>> > <-->}
>> >
>> >
>> >
>> > <-->/*
>> > <--> * If the hashvalue is not specified, we have to recheck all currently
>> > <--> * used session variables. Since we can't tell the exact session variable
>> > <--> * from its hashvalue, we have to iterate over all items in the hash bucket.
>> > <--> */
>> > <-->if (hashvalue == 0)
>> > <--><-->hash_seq_init(&status, sessionvars);
>> > <-->else
>> > <--><-->hash_seq_init_with_hash_value(&status, sessionvars, hashvalue);
>> >
>> > <-->while ((svar = (SVariable) hash_seq_search(&status)) != NULL)
>> > <-->{
>> > <--><-->Assert(hashvalue == 0 || svar->hashvalue == hashvalue);
>> >
>> > elog(NOTICE, "found");
>> >
>> > <--><-->svar->is_valid = false;
>> > <--><-->needs_validation = true;
>> > <-->}
>> > }
>> >
>> > Old methods found an entry, but new not.
>> >
>> > What am I doing wrong?
>>
>> I'm trying to check this. Applying this patch [1], but got conflicts.
>> Could you please, rebase the patch, so I can recheck the issue?
>
> I sent rebased patchset
Thank you.
Please see 40064a8ee1 takes special efforts to match HTAB hash
function to syscache hash function. By default, their hash values
don't match and you can't simply use syscache hash value to search
HTAB. This probably should be mentioned in the header comment of
hash_seq_init_with_hash_value().
yes, enhancing doc should be great + maybe assert
Regards
Pavel
------
Regards,
Alexander Korotkov
Supabase
You may have already realized this, but the name of the function the patch adds is not the same as the name that appears in the commit message. ...Robert
On Wed, Aug 7, 2024 at 3:24 PM Robert Haas <robertmhaas@gmail.com> wrote: > You may have already realized this, but the name of the function the > patch adds is not the same as the name that appears in the commit > message. :sigh: I didn't realize that before your message. That would be another item for my checklist: ensure entities referenced from commit message and comments didn't change their names. ------ Regards, Alexander Korotkov Supabase
On Wed, Aug 7, 2024 at 11:55 AM Alexander Korotkov <aekorotkov@gmail.com> wrote: > On Wed, Aug 7, 2024 at 3:24 PM Robert Haas <robertmhaas@gmail.com> wrote: > > You may have already realized this, but the name of the function the > > patch adds is not the same as the name that appears in the commit > > message. > > :sigh: > I didn't realize that before your message. That would be another item > for my checklist: ensure entities referenced from commit message and > comments didn't change their names. I really wish there was some way to fix commit messages. I had a typo in mine today, too. -- Robert Haas EDB: http://www.enterprisedb.com
On Wed, Aug 7, 2024 at 7:30 PM Robert Haas <robertmhaas@gmail.com> wrote: > On Wed, Aug 7, 2024 at 11:55 AM Alexander Korotkov <aekorotkov@gmail.com> wrote: > > On Wed, Aug 7, 2024 at 3:24 PM Robert Haas <robertmhaas@gmail.com> wrote: > > > You may have already realized this, but the name of the function the > > > patch adds is not the same as the name that appears in the commit > > > message. > > > > :sigh: > > I didn't realize that before your message. That would be another item > > for my checklist: ensure entities referenced from commit message and > > comments didn't change their names. > > I really wish there was some way to fix commit messages. I had a typo > in mine today, too. +1, One of the scariest things that happened to me was forgetting to mention reviewers or even authors. People don't get credit for their work, and you can't fix that. ------ Regards, Alexander Korotkov Supabase
On Wed, Aug 7, 2024 at 1:34 PM Pavel Stehule <pavel.stehule@gmail.com> wrote: > st 7. 8. 2024 v 12:22 odesílatel Alexander Korotkov <aekorotkov@gmail.com> napsal: >> Thank you. >> Please see 40064a8ee1 takes special efforts to match HTAB hash >> function to syscache hash function. By default, their hash values >> don't match and you can't simply use syscache hash value to search >> HTAB. This probably should be mentioned in the header comment of >> hash_seq_init_with_hash_value(). > > > yes, enhancing doc should be great + maybe assert Please check the patch, which adds a caveat to the function header comment. I don't particularly like an assert here, because there could be use-cases besides syscache callbacks, which could legally use default hash function. ------ Regards, Alexander Korotkov Supabase
Attachment
st 7. 8. 2024 v 22:25 odesílatel Alexander Korotkov <aekorotkov@gmail.com> napsal:
On Wed, Aug 7, 2024 at 1:34 PM Pavel Stehule <pavel.stehule@gmail.com> wrote:
> st 7. 8. 2024 v 12:22 odesílatel Alexander Korotkov <aekorotkov@gmail.com> napsal:
>> Thank you.
>> Please see 40064a8ee1 takes special efforts to match HTAB hash
>> function to syscache hash function. By default, their hash values
>> don't match and you can't simply use syscache hash value to search
>> HTAB. This probably should be mentioned in the header comment of
>> hash_seq_init_with_hash_value().
>
>
> yes, enhancing doc should be great + maybe assert
Please check the patch, which adds a caveat to the function header
comment. I don't particularly like an assert here, because there
could be use-cases besides syscache callbacks, which could legally use
default hash function.
looks well
Regards
Pavel
------
Regards,
Alexander Korotkov
Supabase
On Thu, Aug 8, 2024 at 7:45 AM Pavel Stehule <pavel.stehule@gmail.com> wrote: > st 7. 8. 2024 v 22:25 odesílatel Alexander Korotkov <aekorotkov@gmail.com> napsal: >> >> On Wed, Aug 7, 2024 at 1:34 PM Pavel Stehule <pavel.stehule@gmail.com> wrote: >> > st 7. 8. 2024 v 12:22 odesílatel Alexander Korotkov <aekorotkov@gmail.com> napsal: >> >> Thank you. >> >> Please see 40064a8ee1 takes special efforts to match HTAB hash >> >> function to syscache hash function. By default, their hash values >> >> don't match and you can't simply use syscache hash value to search >> >> HTAB. This probably should be mentioned in the header comment of >> >> hash_seq_init_with_hash_value(). >> > >> > >> > yes, enhancing doc should be great + maybe assert >> >> Please check the patch, which adds a caveat to the function header >> comment. I don't particularly like an assert here, because there >> could be use-cases besides syscache callbacks, which could legally use >> default hash function. > > > looks well Thank you, pushed. ------ Regards, Alexander Korotkov Supabase