From 212e6f34d55c910505f87438d878698223d9a617 Mon Sep 17 00:00:00 2001 From: Andres Freund Date: Wed, 4 Oct 2017 00:22:38 -0700 Subject: [PATCH] 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 --- src/backend/utils/Gen_fmgrtab.pl | 79 ++++++++++++++++++++++++++------ src/backend/utils/Makefile | 2 +- src/backend/utils/fmgr/fmgr.c | 29 +++++------- src/include/utils/fmgrtab.h | 11 ++++- 4 files changed, 88 insertions(+), 33 deletions(-) diff --git a/src/backend/utils/Gen_fmgrtab.pl b/src/backend/utils/Gen_fmgrtab.pl index 17851fe2a4..ee89d50784 100644 --- 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 2e35ca58cc..efb8b53f49 100644 --- 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 919733517b..975c968e3b 100644 --- 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 6130ef8f9c..515a9d596a 100644 --- 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 */ -- 2.40.0