Re: Reduce the memcpy call from SearchCatCache

Tom Lane writes:
> Atsushi Ogawa <a_ogawa@hi-ho.ne.jp> writes:
> > Attached patch is reduce the memcpy calls from SearchCatCache
> > and SearchCatCacheList. This patch directly uses cache->cc_skey
> > in looking for hash table.
>
> How much did you test this patch?  I'm fairly sure it will break
> things.
> There are cases where cache lookups happen recursively.

I tested regression test and pgbench. However, I did not consider
recursive case. I revised a patch for safe recursive call.
But I cannot find test case in which recursive call happens.

In my understanding, recursive call at SearchCatCache does not happen
while looking for hash table. The recursive call happens while reading
the relation. If the cache->cc_skey is copied before read the relation,
I think it is safe.

best regards,

--- Atsushi Ogawa

*** ./src/backend/utils/cache/catcache.c.orig    2009-07-07 15:19:56.000000000 +0900
--- ./src/backend/utils/cache/catcache.c    2009-07-07 15:19:46.000000000 +0900
***************
*** 1124,1140 ****

      /*
       * initialize the search key information
       */
!     memcpy(cur_skey, cache->cc_skey, sizeof(cur_skey));
!     cur_skey[0].sk_argument = v1;
!     cur_skey[1].sk_argument = v2;
!     cur_skey[2].sk_argument = v3;
!     cur_skey[3].sk_argument = v4;

      /*
       * find the hash bucket in which to look for the tuple
       */
!     hashValue = CatalogCacheComputeHashValue(cache, cache->cc_nkeys, cur_skey);
      hashIndex = HASH_INDEX(hashValue, cache->cc_nbuckets);

      /*
--- 1124,1141 ----

      /*
       * initialize the search key information
+      * directly use cache->cc_skey while looking for hash table
       */
!     cache->cc_skey[0].sk_argument = v1;
!     cache->cc_skey[1].sk_argument = v2;
!     cache->cc_skey[2].sk_argument = v3;
!     cache->cc_skey[3].sk_argument = v4;

      /*
       * find the hash bucket in which to look for the tuple
       */
!     hashValue = CatalogCacheComputeHashValue(cache, cache->cc_nkeys,
!         cache->cc_skey);
      hashIndex = HASH_INDEX(hashValue, cache->cc_nbuckets);

      /*
***************
*** 1160,1166 ****
          HeapKeyTest(&ct->tuple,
                      cache->cc_tupdesc,
                      cache->cc_nkeys,
!                     cur_skey,
                      res);
          if (!res)
              continue;
--- 1161,1167 ----
          HeapKeyTest(&ct->tuple,
                      cache->cc_tupdesc,
                      cache->cc_nkeys,
!                     cache->cc_skey,
                      res);
          if (!res)
              continue;
***************
*** 1206,1211 ****
--- 1207,1218 ----
      }

      /*
+      * We need copy ScanKey data, because it is possible for recursive
+      * cache lookup.
+      */
+     memcpy(cur_skey, cache->cc_skey, sizeof(cur_skey));
+
+     /*
       * Tuple was not found in cache, so we have to try to retrieve it directly
       * from the relation.  If found, we will add it to the cache; if not
       * found, we will add a negative cache entry instead.
***************
*** 1371,1389 ****

      /*
       * initialize the search key information
       */
!     memcpy(cur_skey, cache->cc_skey, sizeof(cur_skey));
!     cur_skey[0].sk_argument = v1;
!     cur_skey[1].sk_argument = v2;
!     cur_skey[2].sk_argument = v3;
!     cur_skey[3].sk_argument = v4;

      /*
       * compute a hash value of the given keys for faster search.  We don't
       * presently divide the CatCList items into buckets, but this still lets
       * us skip non-matching items quickly most of the time.
       */
!     lHashValue = CatalogCacheComputeHashValue(cache, nkeys, cur_skey);

      /*
       * scan the items until we find a match or exhaust our list
--- 1378,1396 ----

      /*
       * initialize the search key information
+      * directly use cache->cc_skey while looking for hash table
       */
!     cache->cc_skey[0].sk_argument = v1;
!     cache->cc_skey[1].sk_argument = v2;
!     cache->cc_skey[2].sk_argument = v3;
!     cache->cc_skey[3].sk_argument = v4;

      /*
       * compute a hash value of the given keys for faster search.  We don't
       * presently divide the CatCList items into buckets, but this still lets
       * us skip non-matching items quickly most of the time.
       */
!     lHashValue = CatalogCacheComputeHashValue(cache, nkeys, cache->cc_skey);

      /*
       * scan the items until we find a match or exhaust our list
***************
*** 1410,1416 ****
          HeapKeyTest(&cl->tuple,
                      cache->cc_tupdesc,
                      nkeys,
!                     cur_skey,
                      res);
          if (!res)
              continue;
--- 1417,1423 ----
          HeapKeyTest(&cl->tuple,
                      cache->cc_tupdesc,
                      nkeys,
!                     cache->cc_skey,
                      res);
          if (!res)
              continue;
***************
*** 1440,1445 ****
--- 1447,1458 ----
      }

      /*
+      * We need copy ScanKey data, because it is possible for recursive
+      * cache lookup.
+      */
+     memcpy(cur_skey, cache->cc_skey, sizeof(cur_skey));
+
+     /*
       * List was not found in cache, so we have to build it by reading the
       * relation.  For each matching tuple found in the relation, use an
       * existing cache entry if possible, else build a new one.