Re: Improve catcache/syscache performance.

On Thu, Sep 14, 2017 at 2:12 AM, Andres Freund <andres@anarazel.de> wrote:
> This patch gives me roughly 8% speedup in a workload that consists out
> of a fast query that returns a lot of columns.  If I apply a few
> other performance patches, this patch itself starts to make a bigger
> difference, of around 11%.

I did a read-through of this patch today.  I don't see anything really
serious to complain about here.  Somebody might question the use of
the no-inline stuff, but it seems sensible to me in this context.

+    /* not as performance critical & "complicated" */

This comment kinda sucks.  I don't think it will be clear to somebody
in 3 years what this means.  It's clear enough in context but later I
think it won't be.  I suggest dumping this altogether and expanding
the comment up above to encompass this:

Hash and equality functions for system types that are used as cache
key fields.  In some cases, we just call the regular SQL-callable
functions for the appropriate data type, but that tends to be a little
slow, and the speed of these functions is performance-critical.
Therefore, for data types that frequently occur as catcache keys, we
hard-code the logic here.  Avoiding the overhead of
DirectFunctionCallN(...) is a substantial win, and in certain cases
(like int4) we can adopt a faster hash algorithm as well.

+        {
+            return false;
+        }

Excess braces.

+ * The use of argument specific numbers is encouraged, they're faster, and
+ * insulates the caller from changes in the maximum number of keys.

s/, they're faster/. They're faster/

-    if (cache->cc_tupdesc == NULL)
+    if (unlikely(cache->cc_tupdesc == NULL))
         CatalogCacheInitializeCache(cache);

I don't think it's this patch's job to do it, but it seems like we
ought to just invent some early-initialization step where things like
this can happen, so that we don't have to branch here at all.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company