On 2017-09-27 15:50:05 -0400, Tom Lane wrote: > ISTM it shouldn't be that hard to get Gen_fmgrtab.pl to emit an index > array of the sort we're talking about, along with the FmgrBuiltin array > it already prints out. I'm the world's worst Perl programmer but > I'm happy to take a stab at it if you don't want to.
I might be worse than you... But anyway, here's a patch doing so. Looking at profiles, it turned out that having the integer limits as extern variables in a different TU isn't a great idea. So I moved what used to be fmgrtab.c to fmgrtab.h, and included it directly in fmgr.c. Is this roughly what you were thinking of? Greetings, Andres Freund
>From 11fc054fda92fafc8b6c1ac66f70ecf059c61a9d Mon Sep 17 00:00:00 2001 From: Andres Freund <and...@anarazel.de> Date: Tue, 26 Sep 2017 12:40:56 -0700 Subject: [PATCH] Speed up fmgr_isbuiltin() by keeping an oid -> builtin mapping. Turns out we have enough functions that the binary search is quite noticeable in profiles. To address that, have Gen_fmgrtab.pl build an oid -> fmgr_builtins index mapping. With that mapping fmgr_isbuiltin() just consists out of a comparison and two array lookups. To avoid having to reference extern variables, move the generated table to fmgrtab.h and include that from fmgr.c. Author: Andres Freund Discussion: https://postgr.es/m/20170914065128.a5sk7z4xde5uy...@alap3.anarazel.de --- src/backend/Makefile | 14 +++++-- src/backend/utils/Gen_fmgrtab.pl | 82 ++++++++++++++++++++++++++++++---------- src/backend/utils/Makefile | 7 ++-- src/backend/utils/fmgr/fmgr.c | 27 ++++++------- src/include/fmgr.h | 16 ++++++++ src/include/utils/fmgrtab.h | 39 ------------------- 6 files changed, 102 insertions(+), 83 deletions(-) delete mode 100644 src/include/utils/fmgrtab.h diff --git a/src/backend/Makefile b/src/backend/Makefile index aab676dbbd..f0eb5d8d78 100644 --- a/src/backend/Makefile +++ b/src/backend/Makefile @@ -142,6 +142,8 @@ utils/errcodes.h: utils/generate-errcodes.pl utils/errcodes.txt # see explanation in parser/Makefile utils/fmgrprotos.h: utils/fmgroids.h ; +utils/fmgrtab.h: utils/fmgroids.h ; + utils/fmgroids.h: utils/Gen_fmgrtab.pl catalog/Catalog.pm $(top_srcdir)/src/include/catalog/pg_proc.h $(MAKE) -C utils $(notdir $@) @@ -169,7 +171,7 @@ submake-schemapg: .PHONY: generated-headers -generated-headers: $(top_builddir)/src/include/parser/gram.h $(top_builddir)/src/include/catalog/schemapg.h $(top_builddir)/src/include/storage/lwlocknames.h $(top_builddir)/src/include/utils/errcodes.h $(top_builddir)/src/include/utils/fmgroids.h $(top_builddir)/src/include/utils/fmgrprotos.h $(top_builddir)/src/include/utils/probes.h +generated-headers: $(top_builddir)/src/include/parser/gram.h $(top_builddir)/src/include/catalog/schemapg.h $(top_builddir)/src/include/storage/lwlocknames.h $(top_builddir)/src/include/utils/errcodes.h $(top_builddir)/src/include/utils/fmgroids.h $(top_builddir)/src/include/utils/fmgrprotos.h $(top_builddir)/src/include/utils/fmgrtab.h $(top_builddir)/src/include/utils/probes.h $(top_builddir)/src/include/parser/gram.h: parser/gram.h prereqdir=`cd '$(dir $<)' >/dev/null && pwd` && \ @@ -201,6 +203,11 @@ $(top_builddir)/src/include/utils/fmgrprotos.h: utils/fmgrprotos.h cd '$(dir $@)' && rm -f $(notdir $@) && \ $(LN_S) "$$prereqdir/$(notdir $<)" . +$(top_builddir)/src/include/utils/fmgrtab.h: utils/fmgrtab.h + prereqdir=`cd '$(dir $<)' >/dev/null && pwd` && \ + cd '$(dir $@)' && rm -f $(notdir $@) && \ + $(LN_S) "$$prereqdir/$(notdir $<)" . + $(top_builddir)/src/include/utils/probes.h: utils/probes.h cd '$(dir $@)' && rm -f $(notdir $@) && \ $(LN_S) "../../../$(subdir)/utils/probes.h" . @@ -219,7 +226,7 @@ distprep: $(MAKE) -C catalog schemapg.h postgres.bki postgres.description postgres.shdescription $(MAKE) -C replication repl_gram.c repl_scanner.c syncrep_gram.c syncrep_scanner.c $(MAKE) -C storage/lmgr lwlocknames.h - $(MAKE) -C utils fmgrtab.c fmgroids.h fmgrprotos.h errcodes.h + $(MAKE) -C utils fmgroids.h fmgrprotos.h fmgrtab.h errcodes.h $(MAKE) -C utils/misc guc-file.c $(MAKE) -C utils/sort qsort_tuple.c @@ -312,6 +319,7 @@ clean: $(top_builddir)/src/include/storage/lwlocknames.h \ $(top_builddir)/src/include/utils/fmgroids.h \ $(top_builddir)/src/include/utils/fmgrprotos.h \ + $(top_builddir)/src/include/utils/fmgrtab.h \ $(top_builddir)/src/include/utils/probes.h ifeq ($(PORTNAME), cygwin) rm -f postgres.dll libpostgres.a @@ -341,7 +349,7 @@ maintainer-clean: distclean storage/lmgr/lwlocknames.h \ utils/fmgroids.h \ utils/fmgrprotos.h \ - utils/fmgrtab.c \ + utils/fmgrtab.h \ utils/errcodes.h \ utils/misc/guc-file.c \ utils/sort/qsort_tuple.c diff --git a/src/backend/utils/Gen_fmgrtab.pl b/src/backend/utils/Gen_fmgrtab.pl index 17851fe2a4..c09f8dedb1 100644 --- a/src/backend/utils/Gen_fmgrtab.pl +++ b/src/backend/utils/Gen_fmgrtab.pl @@ -2,7 +2,7 @@ #------------------------------------------------------------------------- # # Gen_fmgrtab.pl -# Perl script that generates fmgroids.h and fmgrtab.c from pg_proc.h +# Perl script that generates fmgroids.h and fmgrtab.h from pg_proc.h # # Portions Copyright (c) 1996-2017, PostgreSQL Global Development Group # Portions Copyright (c) 1994, Regents of the University of California @@ -79,7 +79,7 @@ foreach my $row (@$data) my $tmpext = ".tmp$$"; my $oidsfile = $output_path . 'fmgroids.h'; my $protosfile = $output_path . 'fmgrprotos.h'; -my $tabfile = $output_path . 'fmgrtab.c'; +my $tabfile = $output_path . 'fmgrtab.h'; open my $ofh, '>', $oidsfile . $tmpext or die "Could not open $oidsfile$tmpext: $!"; @@ -156,9 +156,12 @@ qq|/*------------------------------------------------------------------------- print $tfh qq|/*------------------------------------------------------------------------- * - * fmgrtab.c + * fmgrtab.h * The function manager's table of internal functions. * + * This should only be included once, by fmgr.c. Therefore no include guards + * are present. + * * Portions Copyright (c) 1996-2017, PostgreSQL Global Development Group * Portions Copyright (c) 1994, Regents of the University of California * @@ -174,10 +177,7 @@ qq|/*------------------------------------------------------------------------- *------------------------------------------------------------------------- */ -#include "postgres.h" - -#include "utils/fmgrtab.h" -#include "utils/fmgrprotos.h" +#include "fmgr.h" |; @@ -191,32 +191,72 @@ foreach my $s (sort { $a->{oid} <=> $b->{oid} } @fmgr) print $pfh "extern Datum $s->{prosrc}(PG_FUNCTION_ARGS);\n"; } -# Create the fmgr_builtins table -print $tfh "\nconst FmgrBuiltin fmgr_builtins[] = {\n"; +# Create the fmgr_builtins table, collect data for fmgr_builtin_oid_index +print $tfh "\nstatic const FmgrBuiltin fmgr_builtins[] = {\n"; my %bmap; $bmap{'t'} = 'true'; $bmap{'f'} = 'false'; +my $fmgr_builtin_max_oid = -1; +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} }"; + + if ($s->{oid} > $fmgr_builtin_max_oid) + { + $fmgr_builtin_max_oid = $s->{oid}; + } + + $fmgr_builtin_oid_index[$s->{oid}] = $fmgr_count++; + + if ($fmgr_count <= $#fmgr) + { + print $tfh ",\n"; + } + else + { + print $tfh "\n"; + } } +print $tfh "};"; + +print $tfh qq| +static const int fmgr_nbuiltins = (sizeof(fmgr_builtins) / sizeof(FmgrBuiltin)); + +static const Oid fmgr_builtin_max_oid = $fmgr_builtin_max_oid; + +|; + +# Create fmgr_builtins_oid_index table. +print $tfh "static const uint16 fmgr_builtin_oid_index[] = {\n"; +for (my $i = 0; $i <= $fmgr_builtin_max_oid; $i++) +{ + my $oid = $fmgr_builtin_oid_index[$i]; + + # fmgr_builtin_oid_index is sparse, map nonexistant functions to 0 + if (not defined $oid) + { + $oid = 0; + } + + if ($i == $fmgr_builtin_max_oid) + { + print $tfh " $oid\n"; + } + else + { + print $tfh " $oid,\n"; + } +} +print $tfh "};"; + # 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..b299d1fa45 100644 --- a/src/backend/utils/Makefile +++ b/src/backend/utils/Makefile @@ -8,7 +8,6 @@ subdir = src/backend/utils top_builddir = ../../.. include $(top_builddir)/src/Makefile.global -OBJS = fmgrtab.o SUBDIRS = adt cache error fmgr hash init mb misc mmgr resowner sort time # location of Catalog.pm @@ -22,9 +21,9 @@ $(SUBDIRS:%=%-recursive): fmgroids.h fmgrprotos.h # see explanation in ../parser/Makefile fmgrprotos.h: fmgroids.h ; -fmgroids.h: fmgrtab.c ; +fmgroids.h: fmgrtab.h ; -fmgrtab.c: Gen_fmgrtab.pl $(catalogdir)/Catalog.pm $(top_srcdir)/src/include/catalog/pg_proc.h +fmgrtab.h: 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 errcodes.h: $(top_srcdir)/src/backend/utils/errcodes.txt generate-errcodes.pl @@ -50,4 +49,4 @@ clean: rm -f probes.h maintainer-clean: clean - rm -f fmgroids.h fmgrprotos.h fmgrtab.c errcodes.h + rm -f fmgroids.h fmgrprotos.h fmgrtab.h errcodes.h diff --git a/src/backend/utils/fmgr/fmgr.c b/src/backend/utils/fmgr/fmgr.c index a7b07827e0..4fba2514f0 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 >= fmgr_builtin_max_oid) + 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]; + index = fmgr_builtin_oid_index[id]; + if (index == 0) + return NULL; - if (id == ptr->foid) - return ptr; - else if (id > ptr->foid) - low = i + 1; - else - high = i - 1; - } - return NULL; + return &fmgr_builtins[index]; } /* diff --git a/src/include/fmgr.h b/src/include/fmgr.h index b604a5c162..ff79ee3dd5 100644 --- a/src/include/fmgr.h +++ b/src/include/fmgr.h @@ -729,6 +729,22 @@ extern PGDLLIMPORT fmgr_hook_type fmgr_hook; #define FmgrHookIsNeeded(fn_oid) \ (!needs_fmgr_hook ? false : (*needs_fmgr_hook)(fn_oid)) + +/* + * Description of all the built-in functions (ie, functions that are compiled + * into the Postgres executable). + */ +typedef struct +{ + Oid foid; /* OID of the function */ + const char *funcName; /* C name of the function */ + short nargs; /* 0..FUNC_MAX_ARGS, or -1 if variable count */ + bool strict; /* T if function is "strict" */ + bool retset; /* T if function returns a set */ + PGFunction func; /* pointer to compiled function */ +} FmgrBuiltin; + + /* * !!! OLD INTERFACE !!! * diff --git a/src/include/utils/fmgrtab.h b/src/include/utils/fmgrtab.h deleted file mode 100644 index 6130ef8f9c..0000000000 --- a/src/include/utils/fmgrtab.h +++ /dev/null @@ -1,39 +0,0 @@ -/*------------------------------------------------------------------------- - * - * fmgrtab.h - * The function manager's table of internal functions. - * - * Portions Copyright (c) 1996-2017, PostgreSQL Global Development Group - * Portions Copyright (c) 1994, Regents of the University of California - * - * src/include/utils/fmgrtab.h - * - *------------------------------------------------------------------------- - */ -#ifndef FMGRTAB_H -#define FMGRTAB_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. - */ - -typedef struct -{ - Oid foid; /* OID of the function */ - const char *funcName; /* C name of the function */ - short nargs; /* 0..FUNC_MAX_ARGS, or -1 if variable count */ - bool strict; /* T if function is "strict" */ - bool retset; /* T if function returns a set */ - PGFunction func; /* pointer to compiled function */ -} FmgrBuiltin; - -extern const FmgrBuiltin fmgr_builtins[]; - -extern const int fmgr_nbuiltins; /* number of entries in table */ - -#endif /* FMGRTAB_H */ -- 2.14.1.536.g6867272d5b.dirty
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
