Hi Andres,

Another idea would be to have an array of FmgrBuiltin*, that we index by
> oid. That'd not be super small though, given that the space for function
> oids is sparse.
I totally agree here, as the oids are very much scattered having an
array is not feasible here.

> Thus what I've instead done is replacing the binary search in
> fmgr_isbuiltin() with a simplehash.h style hashtable. After that the
> lookup is still visible in the profile, but far less prominent.
> I'd like to move the hash creation out of fmgr_isbuiltin (to avoid
> having to check whether it's already created), but there's currently no
> convenient place to create the hash from.   Now that we don't rely on
> the sortedness of fmgrtab.c we could remove a few lines from
> Gen_fmgrtab.pl, but I don't quite see the advantage. If we were
> interested in a faster by-name lookup we could sort it by name, but
> that'd be better solved by another hashtable...

I looked into these patches.
Seems patch 004 is already committed, commit id:

About patch 0005:
The patch still applies cleanly.
There are no failures in ‘make check’

+ /* TODO: it'd be better if this were done separately */
+ if (unlikely(oid2builtins == NULL))
- int i = (high + low) / 2;
- const FmgrBuiltin *ptr = &fmgr_builtins[i];
+ int i;

- if (id == ptr->foid)
- return ptr;
- else if (id > ptr->foid)
- low = i + 1;
- else
- high = i - 1;
+ oid2builtins = oid2builtins_create(TopMemoryContext,
+   fmgr_nbuiltins,
+   NULL);
+ for (i = 0; i < fmgr_nbuiltins; i++)
+ {
+ const FmgrBuiltin *ptr = &fmgr_builtins[i];
+ bool found;
+ entry = oid2builtins_insert(oid2builtins, ptr->foid, &found);
+ Assert(!found);
+ entry->builtin = ptr;
+ }

As Andres has already pointed, may be we want to move above code in a
function, and just call that function here in case the hash is not already

Further I am thinking about doing some performance testing, Andres can you
point me how did you test it and what perf numbers you saw for this
particular patch(0005).

Jeevan Ladhe

Reply via email to