Re: type cache cleanup improvements

Teodor Sigaev <teodor@sigaev.ru>

From: Teodor Sigaev <teodor@sigaev.ru>
To: Michael Paquier <michael@paquier.xyz>
Cc: Aleksander Alekseev <aleksander@timescale.com>, Pgsql Hackers <pgsql-hackers@postgresql.org>, Tom Lane <tgl@sss.pgh.pa.us>
Date: 2024-03-13T13:40:38Z
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. Maintain RelIdToTypeIdCacheHash in TypeCacheOpcCallback()

  2. Fix concurrrently in typcache_rel_type_cache.sql

  3. Avoid looping over all type cache entries in TypeCacheRelCallback()

  4. Update header comment for lookup_type_cache()

  5. Revert: Avoid looping over all type cache entries in TypeCacheRelCallback()

  6. Introduce hash_search_with_hash_value() function

  7. Optimize InvalidateAttoptCacheCallback() and TypeCacheTypCallback()

  8. Refactor initial hash lookup in dynahash.c

  9. Rationalize and improve error messages for some jsonpath items

  10. Avoid race in RelationBuildDesc() affecting CREATE INDEX CONCURRENTLY.

  11. Represent Lists as expansible arrays, not chains of cons-cells.

Attachments

> 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@sigaev.ru
                                                    WWW: http://www.sigaev.ru/