Re: type cache cleanup improvements

From: Teodor Sigaev <teodor(at)sigaev(dot)ru>
To: Michael Paquier <michael(at)paquier(dot)xyz>
Cc: Aleksander Alekseev <aleksander(at)timescale(dot)com>, Pgsql Hackers <pgsql-hackers(at)postgresql(dot)org>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Subject: Re: type cache cleanup improvements
Date: 2024-03-13 13:40:38
Message-ID: 4ed6a0d7-4297-4fda-8df9-aa28b2526e4a@sigaev.ru
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers


> I think that this patch should be split for clarity, as there are a
> few things that are independently useful. I guess something like
> that:
Done, all patches should be applied consequentially.

> - The typcache changes.
01-map_rel_to_type.v5.patch adds map relation to its type

> - Introduction of hash_initial_lookup(), that simplifies 3 places of
> dynahash.c where the same code is used. The routine should be
> inlined.
> - The split in hash_seq_search to force a different type of search is
> weird, complicating the dynahash interface by hiding what seems like a
> search mode. Rather than hasHashvalue that's hidden in the middle of
> HASH_SEQ_STATUS, could it be better to have an entirely different API
> for the search? That should be a patch on its own, as well.

02-hash_seq_init_with_hash_value.v5.patch - introduces a
hash_seq_init_with_hash_value() method. hash_initial_lookup() is marked as
inline, but I suppose, modern compilers are smart enough to inline it automatically.

Using separate interface for scanning hash with hash value will make scan code
more ugly in case when we need to use special value of hash value as it is done
in cache's scans. Look, instead of this simplified code:
if (hashvalue == 0)
hash_seq_init(&status, TypeCacheHash);
else
hash_seq_init_with_hash_value(&status, TypeCacheHash, hashvalue);
while ((typentry = hash_seq_search(&status)) != NULL) {
...
}
we will need to code something like that:
if (hashvalue == 0)
{
hash_seq_init(&status, TypeCacheHash);

while ((typentry = hash_seq_search(&status)) != NULL) {
...
}
}
else
{
hash_seq_init_with_hash_value(&status, TypeCacheHash, hashvalue);
while ((typentry = hash_seq_search_with_hash_value(&status)) != NULL) {
...
}
}
Or I didn't understand you.

I thought about integrate check inside existing loop in hash_seq_search() :
+ rerun:
if ((curElem = status->curEntry) != NULL)
{
/* Continuing scan of curBucket... */
status->curEntry = curElem->link;
if (status->curEntry == NULL) /* end of this bucket */
+ {
+ if (status->hasHashvalue)
+ hash_seq_term(status);

++status->curBucket;
+ }
+ else if (status->hasHashvalue && status->hashvalue !=
+ curElem->hashvalue)
+ goto rerun;
return (void *) ELEMENTKEY(curElem);
}

But for me it looks weird and adds some checks which will takes some CPU time.

03-att_with_hash_value.v5.patch - adds usage of previous patch.

--
Teodor Sigaev E-mail: teodor(at)sigaev(dot)ru
WWW: http://www.sigaev.ru/

Attachment Content-Type Size
03-att_with_hash_value.v5.patch text/x-patch 10.9 KB
02-hash_seq_init_with_hash_value.v5.patch text/x-patch 5.4 KB
01-map_rel_to_type.v5.patch text/x-patch 7.4 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Bharath Rupireddy 2024-03-13 14:02:37 Re: Add new error_action COPY ON_ERROR "log"
Previous Message Heikki Linnakangas 2024-03-13 13:34:15 Re: BitmapHeapScan streaming read user and prelim refactoring