Replace binary search in fmgr_isbuiltin with a lookup array.

Turns out we have enough functions that the binary search is quite
noticeable in profiles.

Thus have Gen_fmgrtab.pl build a new mapping from a builtin function's
oid to an index in the existing fmgr_builtins array. That keeps the
additional memory usage at a reasonable amount.

Author: Andres Freund, with input from Tom Lane
Discussion: https://postgr.es/m/20170914065128.a5sk7z4xde5uy3ei@alap3.anarazel.de

diff --git a/src/backend/utils/Gen_fmgrtab.pl b/src/backend/utils/Gen_fmgrtab.pl
index 17851fe2a4f53bd66ca90d67583913a2077fc607..ee89d507849217ef46cee1a254073d4ab812e9ef 100644 (file)
--- a/src/backend/utils/Gen_fmgrtab.pl
+++ b/src/backend/utils/Gen_fmgrtab.pl
@@ -21,6 +21,8 @@ use warnings;
 # Collect arguments
 my $infile;    # pg_proc.h
 my $output_path = '';
+my @include_path;
+
 while (@ARGV)
 {
        my $arg = shift @ARGV;
@@ -32,6 +34,10 @@ while (@ARGV)
        {
                $output_path = length($arg) > 2 ? substr($arg, 2) : shift @ARGV;
        }
+       elsif ($arg =~ /^-I/)
+       {
+               push @include_path, length($arg) > 2 ? substr($arg, 2) : shift @ARGV;
+       }
        else
        {
                usage();
@@ -44,6 +50,13 @@ if ($output_path ne '' && substr($output_path, -1) ne '/')
        $output_path .= '/';
 }
 
+# Sanity check arguments.
+die "No input files.\n"                                     if !$infile;
+die "No include path; you must specify -I at least once.\n" if !@include_path;
+
+my $FirstBootstrapObjectId =
+       Catalog::FindDefinedSymbol('access/transam.h', \@include_path, 'FirstBootstrapObjectId');
+
 # Read all the data from the include/catalog files.
 my $catalogs = Catalog::Catalogs($infile);
 
@@ -176,6 +189,7 @@ qq|/*-------------------------------------------------------------------------
 
 #include "postgres.h"
 
+#include "access/transam.h"
 #include "utils/fmgrtab.h"
 #include "utils/fmgrprotos.h"
 
@@ -191,32 +205,71 @@ foreach my $s (sort { $a->{oid} <=> $b->{oid} } @fmgr)
        print $pfh "extern Datum $s->{prosrc}(PG_FUNCTION_ARGS);\n";
 }
 
-# Create the fmgr_builtins table
+# Create the fmgr_builtins table, collect data for fmgr_builtin_oid_index
 print $tfh "\nconst FmgrBuiltin fmgr_builtins[] = {\n";
 my %bmap;
 $bmap{'t'} = 'true';
 $bmap{'f'} = 'false';
+my @fmgr_builtin_oid_index;
+my $fmgr_count = 0;
 foreach my $s (sort { $a->{oid} <=> $b->{oid} } @fmgr)
 {
        print $tfh
-"  { $s->{oid}, \"$s->{prosrc}\", $s->{nargs}, $bmap{$s->{strict}}, $bmap{$s->{retset}}, $s->{prosrc} },\n";
+"  { $s->{oid}, \"$s->{prosrc}\", $s->{nargs}, $bmap{$s->{strict}}, $bmap{$s->{retset}}, $s->{prosrc} }";
+
+       $fmgr_builtin_oid_index[$s->{oid}] = $fmgr_count++;
+
+       if ($fmgr_count <= $#fmgr)
+       {
+               print $tfh ",\n";
+       }
+       else
+       {
+               print $tfh "\n";
+       }
+}
+print $tfh "};\n";
+
+print $tfh qq|
+const int fmgr_nbuiltins = (sizeof(fmgr_builtins) / sizeof(FmgrBuiltin));
+|;
+
+
+# Create fmgr_builtins_oid_index table.
+#
+# Note that the array has to be filled up to FirstBootstrapObjectId,
+# as we can't rely on zero initialization as 0 is a valid mapping.
+print $tfh qq|
+const uint16 fmgr_builtin_oid_index[FirstBootstrapObjectId] = {
+|;
+
+for (my $i = 0; $i < $FirstBootstrapObjectId; $i++)
+{
+       my $oid = $fmgr_builtin_oid_index[$i];
+
+       # fmgr_builtin_oid_index is sparse, map nonexistant functions to
+       # InvalidOidBuiltinMapping
+       if (not defined $oid)
+       {
+               $oid = 'InvalidOidBuiltinMapping';
+       }
+
+       if ($i + 1 == $FirstBootstrapObjectId)
+       {
+               print $tfh "  $oid\n";
+       }
+       else
+       {
+               print $tfh "  $oid,\n";
+       }
 }
+print $tfh "};\n";
+
 
 # And add the file footers.
 print $ofh "\n#endif /* FMGROIDS_H */\n";
 print $pfh "\n#endif /* FMGRPROTOS_H */\n";
 
-print $tfh
-qq|  /* dummy entry is easier than getting rid of comma after last real one */
-  /* (not that there has ever been anything wrong with *having* a
-     comma after the last field in an array initializer) */
-  { 0, NULL, 0, false, false, NULL }
-};
-
-/* Note fmgr_nbuiltins excludes the dummy entry */
-const int fmgr_nbuiltins = (sizeof(fmgr_builtins) / sizeof(FmgrBuiltin)) - 1;
-|;
-
 close($ofh);
 close($pfh);
 close($tfh);

diff --git a/src/backend/utils/Makefile b/src/backend/utils/Makefile
index 2e35ca58cca5d25ef2459fde06a41e5cef0ceb06..efb8b53f4947dd83fcb12eb9c1d8e697e82a1218 100644 (file)
--- a/src/backend/utils/Makefile
+++ b/src/backend/utils/Makefile
@@ -25,7 +25,7 @@ fmgrprotos.h: fmgroids.h ;
 fmgroids.h: fmgrtab.c ;
 
 fmgrtab.c: Gen_fmgrtab.pl $(catalogdir)/Catalog.pm $(top_srcdir)/src/include/catalog/pg_proc.h
-       $(PERL) -I $(catalogdir) $< $(top_srcdir)/src/include/catalog/pg_proc.h
+       $(PERL) -I $(catalogdir) $< -I $(top_srcdir)/src/include/ $(top_srcdir)/src/include/catalog/pg_proc.h
 
 errcodes.h: $(top_srcdir)/src/backend/utils/errcodes.txt generate-errcodes.pl
        $(PERL) $(srcdir)/generate-errcodes.pl $< > $@

diff --git a/src/backend/utils/fmgr/fmgr.c b/src/backend/utils/fmgr/fmgr.c
index 919733517bda369f80f27e77bf0e6e2c9aa003e6..975c968e3b130bc40e692ec2038f1c938310bfd8 100644 (file)
--- a/src/backend/utils/fmgr/fmgr.c
+++ b/src/backend/utils/fmgr/fmgr.c
@@ -70,26 +70,21 @@ static Datum fmgr_security_definer(PG_FUNCTION_ARGS);
 static const FmgrBuiltin *
 fmgr_isbuiltin(Oid id)
 {
-       int                     low = 0;
-       int                     high = fmgr_nbuiltins - 1;
+       uint16          index;
+
+       /* fast lookup only possible if original oid still assigned */
+       if (id >= FirstBootstrapObjectId)
+               return NULL;
 
        /*
-        * Loop invariant: low is the first index that could contain target entry,
-        * and high is the last index that could contain it.
+        * Lookup function data. If there's a miss in that range it's likely a
+        * nonexistant function, returning NULL here will trigger an ERROR later.
         */
-       while (low <= high)
-       {
-               int                     i = (high + low) / 2;
-               const FmgrBuiltin *ptr = &fmgr_builtins[i];
-
-               if (id == ptr->foid)
-                       return ptr;
-               else if (id > ptr->foid)
-                       low = i + 1;
-               else
-                       high = i - 1;
-       }
-       return NULL;
+       index = fmgr_builtin_oid_index[id];
+       if (index == InvalidOidBuiltinMapping)
+               return NULL;
+
+       return &fmgr_builtins[index];
 }
 
 /*

diff --git a/src/include/utils/fmgrtab.h b/src/include/utils/fmgrtab.h
index 6130ef8f9c7418d78756adcaae4f7e68c546c6f5..515a9d596afe5cf913d5f52d94bbcc4bd56107b7 100644 (file)
--- a/src/include/utils/fmgrtab.h
+++ b/src/include/utils/fmgrtab.h
@@ -13,13 +13,13 @@
 #ifndef FMGRTAB_H
 #define FMGRTAB_H
 
+#include "access/transam.h"
 #include "fmgr.h"
 
 
 /*
  * This table stores info about all the built-in functions (ie, functions
- * that are compiled into the Postgres executable).  The table entries are
- * required to appear in Oid order, so that binary search can be used.
+ * that are compiled into the Postgres executable).
  */
 
 typedef struct
@@ -36,4 +36,11 @@ extern const FmgrBuiltin fmgr_builtins[];
 
 extern const int fmgr_nbuiltins;       /* number of entries in table */
 
+/*
+ * Mapping from a builtin function's oid to the index in the fmgr_builtins
+ * array.
+ */
+#define InvalidOidBuiltinMapping UINT16_MAX
+extern const uint16 fmgr_builtin_oid_index[FirstBootstrapObjectId];
+
 #endif                                                 /* FMGRTAB_H */