Re: Improve catcache/syscache performance.

On Thu, Sep 14, 2017 at 2:12 AM, Andres Freund <andres(at)anarazel(dot)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